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

[huizhe wang]

On 7/25/2013 1:06 AM, Chris Hegarty wrote:
> On 07/25/2013 08:39 AM, Daniel Fuchs wrote:
>> Hi Joe,
>>
>> Looks good.
> +1
>
>>
>> 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.
> +1
>
> Additionally, and don't feel obliged either, the (fSecurityPropertyMgr 
> != null) is no longer needed, since this can never be the case. Though 
> I do understand that you may not want to take any chances ;-)

That was exactly what went through my mind :-)  Two bugs reported 
against the update to jaxp 1.5, both related to NPE.

>
> For JDK8, in the future, I think you should remove this constructor. 
> NetBeans can do the right thing to support a new major release.

I haven't heard from NetBeans regarding this. But yes, I'll create a bug 
report and ccc request to change the inner class to private and remove 
the constructor.

-Joe

>
> -Chris.
>>
>> 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
>>>>
>>>
>>