RFR 8172190: Re-apply the fix for bugs 8062389, 8029459, 8061950
Claes Redestad
claes.redestad at oracle.com
Tue Jan 3 17:15:40 UTC 2017
Hi Peter,
letting you know I've submitted a build and test job for this to run
over night.
Thanks!
/Claes
On 01/03/2017 05:52 PM, Peter Levart wrote:
> Hi,
>
> Now that initialization cycle has been broken by Claes' fix for:
>
> https://bugs.openjdk.java.net/browse/JDK-8172048
>
> I prepared a revised fix for issues described in the following bugs:
>
> https://bugs.openjdk.java.net/browse/JDK-8062389
> https://bugs.openjdk.java.net/browse/JDK-8029459
> https://bugs.openjdk.java.net/browse/JDK-8061950
>
> ...now tracked by bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8172190
>
> The revised webrev is here:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.10/
>
>
> The incremental change from webrev.09 is the following:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.09to10/
>
>
>
> Besides fixing one failing test (StarInheritance) that now has to
> account for new behavior of Class.getMethods(), some tests also failed
> because of the changed behavior (not throwing NPE) when passing null
> 'name' parameter to Class.get[Declared][Method|Field] methods. This
> was a straight bug introduced by me and caught by tests which I
> haven't re-run after introducing it. I apologize for that and promise
> to be more careful in the future. In original code the NPE was thrown
> by private Class.searchFields and searchMethods in the part of code
> where the 'name' parameter was interned. I replaced interning and
> reference comparison with equality comparison, which is faster and
> does not trash the String interning cache with names that are later
> not found as members of classes, but I forgot to introduce explicit
> nonNull check for the 'name' argument.
>
> I now intentionally inserted the nonNull checks in front of the
> security checks in public-facing methods although this introduces a
> little behavioral change. I choose that because:
>
> - The javadocs list NullPointerException before the SecurityException
> (for example in getField):
>
> * @throws NullPointerException if {@code name} is {@code null}
> * @throws SecurityException
> * If a security manager, <i>s</i>, is present and
> * the caller's class loader is not the same as or an
> * ancestor of the class loader for the current class and
> * invocation of {@link SecurityManager#checkPackageAccess
> * s.checkPackageAccess()} denies access to the package
> * of this class.
>
> - I think it is more correct for methods to 1st check parameters
> before throwing any other state related exceptions.
>
> But if you think this behavioral change is not justified, I can
> reverse the order of checks and prepare new webrev.
>
> I have successfully executed java/lang tests and all tier 1 tests that
> failed when webrev.09 was pushed, mentioned in:
>
> https://bugs.openjdk.java.net/browse/JDK-8171988
>
>
> But don't take my word for it. Can someone please run webrev.10
> through the tests on Oracle side before confirming the change?
>
>
> Thanks,
>
> Peter Levart
>
>
> On 12/26/2016 07:31 PM, Peter Levart wrote:
>> Hi,
>>
>> I see there remaining failures are not trivial to fix in a hurry. So
>> I think it's better to just backout this change and than prepare new
>> fix...
>>
>> I'm pushing this backout now.
>>
>> Regards, Peter
>>
>>
>> On 12/26/2016 06:55 PM, Claes Redestad wrote:
>>> Hi,
>>>
>>> many of the tier 1 failures listed seems to fail due to a cyclic
>>> bootstrap dependency on the new PublicMethods -> Policy.isSet() ->
>>> ... -> PublicMethods that appears in all tests which install a
>>> security manager.
>>>
>>> It turns out the dependency is only there (Policy.isSet) to ensure the
>>> VM has booted to a state where System.getProperty will return actual
>>> values to feed into sun.security.util.Debug (the state of which does
>>> not in any way vary with Policy), and could be replaced by the new
>>> VM.isBooted() (or VM.initLevel() > 1):
>>>
>>> http://cr.openjdk.java.net/~redestad/scratch/8171988.01/
>>>
>>> With this most test failures disappear, but there are still 5 tests
>>> in java/lang/Class which fail with a message more directly related to
>>> the changes:
>>>
>>> expected java.lang.NullPointerException for null -- FAILED
>>>
>>> Perhaps problem list these 5 together with the above change as a fix to
>>> this issue?
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>> On 2016-12-26 18:30, Chris Hegarty wrote:
>>>>
>>>>> On 26 Dec 2016, at 16:26, joe darcy <joe.darcy at oracle.com> wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> Assuming we'll want to revisit this work at some point, there are
>>>>> some advantages to anti-delta-ing the code changes now, but just
>>>>> problem listing the tests in terms of making a less confusing
>>>>> history.
>>>>
>>>> My preference is to anti-delta. There are just too many tests
>>>> failing, ~35 across all platforms and tiers.
>>>>
>>>> Peter,
>>>> Let me know if you need any help pushing this.
>>>>
>>>> -Chris.
>>>>
>>>>
>>>>> Thanks,
>>>>>
>>>>> -Joe
>>>>>
>>>>>
>>>>> On 12/26/2016 1:58 AM, Chris Hegarty wrote:
>>>>>>> On 26 Dec 2016, at 09:35, Peter Levart <peter.levart at gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Jeff,
>>>>>>>
>>>>>>> I've been told that the latest change I pushed causes some tests
>>>>>>> to fail, so I prepared a backout patch for 8062389, 8029459,
>>>>>>> 8061950:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/backout.09/webrev.01/
>>>>>>> <http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/backout.09/webrev.01/>
>>>>>>>
>>>>>> I just grabbed the webrev patch, applied it to a local repo, then
>>>>>> compared that against a repo that had been updated to the
>>>>>> change prior to your push. They are identical, so this appears
>>>>>> to be an accurate anti-delta.
>>>>>>
>>>>>> Maybe file a new bug, or just make it clear in the synopsis of
>>>>>> 8171988 that it is an anti-delta.
>>>>>>
>>>>>>
>>>>>>> From the stacktrace of the bug report, it seems an early
>>>>>>> initialization issue with VarHandle(s) involved. Can you shed
>>>>>>> some light into what tests are failing?
>>>>>> I’ll post a few comments in 8171988 with sample failures.
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>> But first let us backout that change.
>>>>>>>
>>>>>>> Regards, Peter
>>>>>>>
>>>>>>>> On 12/26/2016 10:09 AM, Peter Levart wrote:
>>>>>>>> Hi Jeff,
>>>>>>>>
>>>>>>>> I'm taking a look at this...
>>>>>>>>
>>>>>>>> Regards, Peter
>>>>>>>>
>>>>>>>>> On 12/26/2016 06:14 AM, Jeff Dinkins wrote:
>>>>>>>>> Hi Peter -
>>>>>>>>>
>>>>>>>>> I just received mail from out SQE manager, saying that your
>>>>>>>>> last changeset has caused our test harness to hiccup. I don’t
>>>>>>>>> have much more detail besides the below bug, but I’m wondering
>>>>>>>>> if you could do us a huge favor and roll your change back for
>>>>>>>>> now while it’s debugged (and so we can get our automated tests
>>>>>>>>> going again).
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8171988
>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8171988>
>>>>>>>>>
>>>>>>>>> thanks!
>>>>>>>>>
>>>>>>>>> -jeff
>>>>>>>>>
>>>>>
>>>>
>>
>
More information about the core-libs-dev
mailing list