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

Fri, 18 Nov 2016 15:23:40 -0800 


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>
*Cc:* 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