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

[Roger Riggs]

Looks fine  +1

Thanks, Roger


On 2/8/2018 10:13 AM, Jason Mehrens wrote:
> Aleksei,
>
> Looks good to me.
>
> Jason
> ________________________________________
> From: Aleks Efimov <aleksej.efimov at oracle.com>
> Sent: Wednesday, February 7, 2018 7:24 PM
> To: Roger Riggs; Jason Mehrens
> Cc: core-libs-dev at openjdk.java.net
> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception
>
> 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
>>>>>
>>>>>