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.efi...@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-boun...@openjdk.java.net> on
behalf of Aleks Efimov <aleksej.efi...@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
