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

[Peter Levart]

Thanks Mandy, Claes,

This is for real now ;-)

Regards, Peter

On 01/05/2017 03:05 AM, Mandy Chung wrote:
> 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
>>>>