RFR 8172190: Re-apply the fix for bugs 8062389, 8029459, 8061950
Peter Levart
peter.levart at gmail.com
Tue Jan 3 16:52:35 UTC 2017
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