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

[Mandy Chung]

I reviewed webrev.09to10.  The NPE check is fine.  I also ran the core tests and no surprise found.

Mandy

> On Jan 4, 2017, at 8:30 AM, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> 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
>>>