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.efi...@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.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



Reply via email to