RFR: 8169772: [JAXP] XALAN: Transformation of DOM with null valued text node causes NPE

[Joe Wang]

On 11/18/16, 12:17 PM, Langer, Christoph wrote:
>
> Hi Joe,
>
> Yep, I'll conduct the testing before pushing.
>
> With the "Expected Result", you mean some comment for the test method, 
> right?
>

Yes, some comment for the test method would do. But I see that you've 
added assertions? That would be even better. Comments help in this case 
since without the patch, the process would have thrown an NPE.

Once a bug is fixed, SQE will take the new test and verify the fix. 
Knowing the success/fail (before and after fix) conditions is helpful 
for them.

Thanks,
Joe

> Thanks
>
> Christoph
>
> *From:*Joe Wang [mailto:huizhe.wang at oracle.com]
> *Sent:* Freitag, 18. November 2016 20:00
> *To:* Langer, Christoph <christoph.langer at sap.com>
> *Cc:* core-libs-dev at openjdk.java.net
> *Subject:* Re: RFR: 8169772: [JAXP] XALAN: Transformation of DOM with 
> null valued text node causes NPE
>
> Looks good, Christoph.
>
> I assume you'll do an all-test run (all in jaxp/test) before pushing.  
> No need for further review, but it'd be nice to add an "Expected 
> result" for the new test testBug8169772, with/without the fix (e.g. NPE).
>
> Best regards,
> Joe
>
> On 11/18/16, 4:38 AM, Langer, Christoph wrote:
>
>     Hi Joe,
>
>     thanks for the feedback.
>
>     I've created a new version of the webrev working in your
>     suggestions, adding some further formatting cleanups in the files
>     and I also moved a small refactoring in TransformerTest.java to
>     this changeset.
>
>     http://cr.openjdk.java.net/~clanger/webrevs/8169772.1/
>     <http://cr.openjdk.java.net/%7Eclanger/webrevs/8169772.1/>
>
>     From my end this one is ready for pushing -- waiting for your
>     final go.
>
>     Best regards
>
>     Christoph
>
>     *From:*Joe Wang [mailto:huizhe.wang at oracle.com]
>     *Sent:* Freitag, 18. November 2016 07:36
>     *To:* Langer, Christoph <christoph.langer at sap.com>
>     <mailto:christoph.langer at sap.com>
>     *Cc:* core-libs-dev at openjdk.java.net
>     <mailto:core-libs-dev at openjdk.java.net>
>     *Subject:* Re: RFR: 8169772: [JAXP] XALAN: Transformation of DOM
>     with null valued text node causes NPE
>
>     Looks fine.
>
>     License header: in general, don't change / add Year if there's no
>     material change, removing the legacy $Id field in
>     EmptySerializer.java for example, does not constitute a change to
>     the code, so just keep the original year (see below).
>
>          The initial years for the classes:
>                 EmptySerializer.java 2012
>                 SerializerBase.java 2012
>                 ToSAXHandler.java none (meaning if you make changes to
>     this class, just add 2016)
>                 ToStream.java 2006
>                 ToUnknownStream.java 2007
>                 XSLOutputAttributes.java none (so keep the original
>     "DO NOT REMOVE OR ALTER!" block)
>
>     Thanks,
>     Joe
>
>     On 11/16/16, 6:22 AM, Langer, Christoph wrote:
>
>     Hi,
>
>       
>
>     please review another XALAN fix.
>
>       
>
>     The Serializer should be able to handle text nodes with null input. There has already been some discussion here:http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-November/044567.html
>
>       
>
>     Bug:https://bugs.openjdk.java.net/browse/JDK-8169772
>
>     Webrev:http://cr.openjdk.java.net/~clanger/webrevs/8169772.0/  <http://cr.openjdk.java.net/%7Eclanger/webrevs/8169772.0/>
>
>       
>
>     The actual fix is in ToUnknownStream.java, method "public void characters(String chars) throws SAXException". I don't know if one should even directly return after chars being null or of size() 0. The rest of this change is cleanups and a test case.
>
>       
>
>     Thanks for reviewing.
>
>       
>
>     Best regards
>
>     Christoph
>
>           
>