RFR: JAXP in JDK8/7u40 : 8021148 Regression in SAXParserImpl in 7u40 b34 (NPE)

[huizhe wang]

Thanks for the quick review and the tip.  I've started a build and am 
still trying to fix some test issues on the other patch. So for now I'll 
keep it that way.

-Joe

On 7/25/2013 12:39 AM, Daniel Fuchs wrote:
> Hi Joe,
>
> Looks good.
>
> Small nit: you could keep fSecurityPropertyManager final by using:
>
> if (spm != null) {
>    fSecurityPropertyManager = spm;
> } else {
>     fSecurityPropertyManager = new ...
>     ...
> }
>
> Don't feel obliged to do it though - your fix is good as it is.
>
> best regards,
>
> -- daniel
>
> On 7/25/13 8:30 AM, huizhe wang wrote:
>> I received test from the NetBeans team. The NetBeans did reference 
>> the internal JAXPSAXParser directly. This is not a usage supported by 
>> the spec. I would suggest them remove it and use the default parser 
>> instead. I'm asking the NetBeans team what are the features they rely 
>> on that the default parser couldn't provide. In the meantime, we're 
>> making this fix so that existing NetBeans continue to work properly 
>> when 7u40 is used.
>>
>> Here's an updated webrev (test included):
>> http://cr.openjdk.java.net/~joehw/7u40/8021148/webrev/
>>
>> Thanks,
>> Joe
>>
>> On 7/24/2013 3:41 AM, Lance Andersen - Oracle wrote:
>>> Agree with the change and making fSecurityPropertyMgr final
>>>
>>> Best
>>> Lance
>>> On Jul 24, 2013, at 5:04 AM, chris Hegarty wrote:
>>>
>>>> Joe,
>>>>
>>>> I can see in SAXParserImpl constructor, setFeature0 could throw, 
>>>> leaving the fSecurityPropertyMgr uninitialized. There my be other 
>>>> code paths too.
>>>>
>>>> I agree with this change, and Daniel's comments to make both 
>>>> fSecurityPropertyMgr fields final. Consider it reviewed ( at least 
>>>> on my side ).
>>>>
>>>> -Chris.
>>>>
>>>> On 24/07/2013 07:59, huizhe wang wrote:
>>>>> On 7/23/2013 11:00 PM, Daniel Fuchs wrote:
>>>>>> Hi Joe,
>>>>>>
>>>>>> This looks reasonable.
>>>>>> Out of curiosity - could it be that it was fSAXParser that was null,
>>>>>> and not fSecurityPropertyMgr?
>>>>>> JAXPSAXParser has a no arg public constructor that could have 
>>>>>> lead to
>>>>>> that...
>>>>> That was my suspicion as well. I thought NetBeans was referencing the
>>>>> internal class directly since the new JAXPSAXParser(this) inside
>>>>> SAXParserImpl was the only call in the entire jaxp code. I was 
>>>>> therefore
>>>>> thinking it really should have been a private class. Of course, once
>>>>> NetBeans bugzilla became accessible (it was down at the time), I was
>>>>> able to get the error stacktrace.
>>>>>
>>>>> There is still something strange since XMLReaderManager.getXMLReader
>>>>> calls XMLReaderFactory which should have returned SAXParser since 
>>>>> it's
>>>>> hardcoded default. In a manual test, I could only get a 
>>>>> JAXPSAXParser if
>>>>> I intentionally set "org.xml.sax.driver" to a "bogus parser". I'm 
>>>>> asking
>>>>> the NetBeans reporter and haven't heard from him yet.
>>>>>
>>>>>> I have only one remark:
>>>>>>
>>>>>> It looks as if fSecurityPropertyMgr could be declared final in both
>>>>>> classes - and I think it
>>>>>> would be better if it were: it would make it clear that it's never
>>>>>> replaced in fSAXParser
>>>>>> and that therefore your new code is strictly equivalent to the 
>>>>>> old in
>>>>>> that respect.
>>>>> Make sense.
>>>>>
>>>>> Thanks,
>>>>> Joe
>>>>>
>>>>>> best regards,
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>> On 7/24/13 4:01 AM, huizhe wang wrote:
>>>>>>> Hi Lance, Chris,
>>>>>>>
>>>>>>> Looking at the affected class [1], and the error stack trace [2] 
>>>>>>> , it
>>>>>>> appeared that in SAXParserImpl$JAXPSAXParser , line 545,
>>>>>>> fSAXParser.fSecurityPropertyMgr is null when setProperty is called.
>>>>>>> fSecurityPropertyMgr was instantiated in SAXParserImpl's 
>>>>>>> constructor
>>>>>>> after JAXPSAXParser was. I can see a chance where the NetBeans 
>>>>>>> got a
>>>>>>> copy of JAXPSAXParser instance with fSecurityPropertyMgr not
>>>>>>> initialized. The fix is to remove the reference of
>>>>>>> fSecurityPropertyMgr in JAXPSAXParser, and pass it in when
>>>>>>> JAXPSAXParser is created.
>>>>>>>
>>>>>>> Here is the webrev:
>>>>>>> http://cr.openjdk.java.net/~joehw/7u40/8021148/webrev/
>>>>>>>
>>>>>>> [1]
>>>>>>> http://cr.openjdk.java.net/~joehw/7u40/8021148/webrev/src/com/sun/org/apache/xerces/internal/jaxp/SAXParserImpl.java.html 
>>>>>>>
>>>>>>>
>>>>>>> [2]
>>>>>>>
>>>>>>> Caused by: java.lang.NullPointerException
>>>>>>> at
>>>>>>> com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl.access$300(SAXParserImpl.java:69) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser.setProperty(SAXParserImpl.java:545) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xml.internal.utils.XMLReaderManager.getXMLReader(XMLReaderManager.java:161) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xml.internal.dtm.ref.DTMManagerDefault.getXMLReader(DTMManagerDefault.java:613) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xalan.internal.xsltc.dom.XSLTCDTMManager.getDTM(XSLTCDTMManager.java:401) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xalan.internal.xsltc.dom.XSLTCDTMManager.getDTM(XSLTCDTMManager.java:252) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.getDOM(TransformerImpl.java:559) 
>>>>>>>
>>>>>>>
>>>>>>> ... 43 more
>>>>>>> ---------
>>>>>>> javax.xml.transform.TransformerException: 
>>>>>>> java.lang.NullPointerException
>>>>>>> at
>>>>>>> com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.getDOM(TransformerImpl.java:581) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:742) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:353) 
>>>>>>>
>>>>>>>
>>>>>>> at
>>>>>>> org.netbeans.spi.project.support.ant.GeneratedFilesHelper$1$1.run(GeneratedFilesHelper.java:332) 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Joe
>>>>>>>
>>>
>>>
>>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>> Oracle Java Engineering
>>> 1 Network Drive
>>> Burlington, MA 01803
>>> Lance.Andersen at oracle.com
>>>
>>
>