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@oracle.com> Sent: Wednesday, February 7, 2018 7:24 PM To: Roger Riggs; Jason Mehrens Cc: core-libs-dev@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@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/xpat...
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@openjdk.java.net> on behalf of Aleks Efimov <aleksej.efimov@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