RFR 8172190: Re-apply the fix for bugs 8062389, 8029459, 8061950

Wed Jan 4 16:30:22 UTC 2017

Hi,

no new failures - seems good to go unless someone objects to the
NPE behavior change.

Thanks!

/Claes

On 2017-01-03 18:15, Claes Redestad wrote:
> 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
>>>>>>>>>>
>>>>>>
>>>>>
>>>
>>
>