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 >>>> >>>> >> >
