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

Tue, 22 Nov 2016 14:02:20 -0800 

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.w...@oracle.com]
*Sent:* Samstag, 19. November 2016 00:23
*To:* Langer, Christoph <christoph.lan...@sap.com>
*Cc:* core-libs-dev@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.w...@oracle.com]
    *Sent:* Freitag, 18. November 2016 20:00
    *To:* Langer, Christoph <christoph.lan...@sap.com>
    <mailto:christoph.lan...@sap.com>
    *Cc:* core-libs-dev@openjdk.java.net
    <mailto:core-libs-dev@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.w...@oracle.com]
        *Sent:* Freitag, 18. November 2016 07:36
        *To:* Langer, Christoph <christoph.lan...@sap.com>
        <mailto:christoph.lan...@sap.com>
        *Cc:* core-libs-dev@openjdk.java.net
        <mailto:core-libs-dev@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

Reply via email to