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

[Joe Wang]

Hi Christoph,

That looks good to me.

Best,
Joe

On 11/22/16, 1:26 PM, Langer, Christoph wrote:
>
> Hi Joe,
>
> as my jtreg issues are solved now, I'm finalizing the patches.
>
> For this one I've updated the test method: 
> http://cr.openjdk.java.net/~clanger/webrevs/8169772.2/ 
> <http://cr.openjdk.java.net/%7Eclanger/webrevs/8169772.2/>
>
> Could you please quickly check if my new test method 
> "testBug8169772()" looks as you want it? I've added some more detailed 
> comments for the method. The point is that without the fix you'd 
> encounter an NPE. Or should there be some special assertion around it?
>
> If it's ok for you, I'd push tomorrow.
>
> Thanks & Best regards
>
> Christoph
>
> *From:*Joe Wang [mailto:huizhe.wang at oracle.com]
> *Sent:* Samstag, 19. November 2016 00:23
> *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
>
>
>
> 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>
>     <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 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
>
>               
>