RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Aleks Efimov]

Hi Jason, Roger,

Thanks for your comments.
The new webrev with suggested/recommended edits can be viewed at this 
location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/03/

Thanks,
Aleksei

PS: Also configured my IDE to use explicit imports =)

On 02/06/2018 03:31 PM, Roger Riggs wrote:
> Hi Aleksei,
>
> SAXException.java x 2, SAXExceptionInitCause:
>   Please use explicit imports (not java.io.*).
>
> SAXEXception.java:145: I would have expected:
> if (cause instanceof Exception) {...}
>
> SAXExceptionInitCause.java:
>   Great to see all the test cases.
>
>   Generally, we recommend using try-with-resources but in this case it 
> does not add much because
>   exceptions can't occur when using BAOS.
>
> Thanks, Roger
>
> On 2/5/2018 9:46 PM, Aleks Efimov wrote:
>> Thank you for your comments, Jason!
>> New webrev can be found at this location:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/02/
>>
>> The list of changes:
>> - this.initCause() -> super.initCause()
>> - 'readObject' has been modified to convert IllegalStateException to 
>> InvalidClassException. The test case that triggers 
>> IllegalStateException in SAXException::readObject has been added to 
>> 'testReadObjectIllegalStateException' test method.
>>
>> Best Regards,
>> Aleksei
>>
>> On 02/05/2018 05:09 PM, Jason Mehrens wrote:
>>> Aleksei,
>>>
>>> Looks good.  We should avoid this.initCause in readObject and prefer 
>>> super.initCause so subclasses can't alter the deserialization behavior.
>>> While I can't think of a case off the top of my head, I would prefer 
>>> that we trap and convert IllegalStateException into a 
>>> InvalidClassException in readObject.
>>> That keeps the readObject contract intact in case something 
>>> malicious is going on.  That is just my preference though.
>>>
>>> Jason
>>> ________________________________________
>>> From: Aleks Efimov <aleksej.efimov at oracle.com>
>>> Sent: Monday, February 5, 2018 10:51 AM
>>> To: Jason Mehrens; 'Roger Riggs'
>>> Cc: core-libs-dev
>>> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does 
>>> not correctly set Exception
>>>
>>> Hi Roger, Jason,
>>>
>>> I've tried to avoid doing the same stuff as I did for XPathException to
>>> minimize the changes to the SAXException class. But I was naive to 
>>> think so:
>>> Tried to keep the exception field in place to avoid modifications
>>> required to keep serial form intact (similar as it was done in
>>> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' 
>>> method
>>> (spotted by Roger, thank you!) that can make cause/exception values
>>> inconsistent. To avoid the API modification and for the sake of clarity
>>> I proceeded with removal of the 'exception' field. The new version of
>>> the fix:
>>> - Removes 'exception' field
>>> - Maintains the serial form of the SAXException class
>>> - Handles the difference in SAXException:exception and Throwable:cause
>>> types 'SAXException::getException', i.e. SAXException:exception is
>>> Exception typed and Throwable:cause is Throwable typed.
>>> - Avoids any changes to API and serial form of the class, but
>>> maintaining it similar to JDK-8009581)
>>> - There is a copy of SAXException class in java.base module that is
>>> required for j.u.Properties::loadFromXML, it has been update with the
>>> same changes.
>>>
>>> New regression test has been added to test:
>>> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
>>> - usages of SAXException::initCause/getException
>>>
>>> Webrev with the fix and new regression:
>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>>>
>>> Testing shows no JCK/regression tests failures with the proposed fix.
>>>
>>> Best Regards,
>>> Aleksei
>>>
>>>
>>> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>>>> Aleksei,
>>>>
>>>> You have to override all of the constructors to always disable 
>>>> initCause.  Actually a similar issue was covered in:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html 
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html 
>>>>
>>>>
>>>> Pretty much everything was hashed out in those threads.
>>>>
>>>> Here is the final webrev for that:
>>>>
>>>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html 
>>>>
>>>>
>>>> That should be a good cookbook for changes and tests required for 
>>>> this issue.  Note that it gets rid of the Exception field and 
>>>> maintains serial compatibility.
>>>> Looking back on that change, I would have changed it so the 
>>>> InvalidClassException added the double cause as suppressed exceptions.
>>>>
>>>> Jason
>>>>
>>>> ________________________________________
>>>> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> on 
>>>> behalf of Aleks Efimov <aleksej.efimov at oracle.com>
>>>> Sent: Thursday, December 21, 2017 11:27 AM
>>>> To: core-libs-dev
>>>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does 
>>>> not correctly set Exception
>>>>
>>>> Hello,
>>>>
>>>> Please, help to review the fix for jaxp bug that fixes SAXException to
>>>> correctly set exception cause with 'setCause' method:
>>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
>>>> I've tried to keep the fix miminal with respect to serial form of this
>>>> API class, i.e. kept private 'exception' field.
>>>>
>>>> The new test case was added to IssueTracker56Test unit test. Testing
>>>> showed no related JCK/JTREG failures.
>>>>
>>>> With Best Regards,
>>>> Aleksei
>>>>
>>>>
>>
>