JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE

[Chris Hegarty]

This latest webrev looks good to me. If you haven't already got a 
sponsor, then I would be happy to sponsor this for you.

Since there are minor spec clarifications, to document long standing 
behavior, then a CCC request will need to be submitted, and approved, 
before integration.

-Chris.

On 08/10/2013 22:29, Brian Burkhalter wrote:
> I have posted an updated webrev http://cr.openjdk.java.net/~bpb/7179567/webrev.3/ which I hope addresses all the concerns expressed below.
>
> Please see also my comments in line.
>
> Once this looks acceptable I can do the CCC request.
>
> Thanks,
>
> Brian
>
> On Oct 7, 2013, at 9:28 PM, David Holmes wrote:
>
>> Aside: I'm confused about the relationship of this bug and JDK-6445180 - are they not duplicates? Seems to me this one should have been closed as a dup when it was reported.
>
> I think you are correct. I can adjust this once the webrev is approved.
>
>> On 5/10/2013 6:58 AM, Brian Burkhalter wrote:
>>> On Oct 3, 2013, at 5:38 PM, Brian Burkhalter wrote:
>>>
>>>> On Oct 3, 2013, at 5:35 PM, Alan Bateman wrote:
>>>>
>>>>> On 03/10/2013 16:10, Brian Burkhalter wrote:
>>>>>> Please review and comment at your convenience.
>>>>>>
>>>>>> Issue:	https://bugs.openjdk.java.net/browse/JDK-7179567
>>>>>> Webrev:	http://cr.openjdk.java.net/~bpb/7179567/
>>>
>>> An updated webrev which I hope adequately addresses the expressed concerns may be found at:
>>>
>>> http://cr.openjdk.java.net/~bpb/7179567.2/
>>
>> URLClassLoader.java: please delete the commented out line:
>>
>> +         //Objects.requireNonNull(urls);
>
> I already fixed that but had not updated the webrev.
>
>> SecureClassLoader.java:
>>
>> 186      * @throws SecurityException if the {@code ClassLoader} instance is not
>> 187      * initialized.
>>
>> should read "this classloader instance" as it refers to the current instance. Also this may be bringing the spec into line with the implementation but "initialization" here is purely an implementation detail not part of the specification for this class so it should not be mentioned explicitly - and this change still needs a CCC (which might help determine exactly what should be said here - I'm unclear how you can try to call getPermissions if this is uninitialized as it would need 'this' to escape from the constructor - which perhaps it can do via a third-party security manager?).
>
> I've removed this @throws clause.
>
>> NoCallStackClassLoader.java: the comment is far too verbose, we don't write such explanatory kinds of comments for this kind of thing (else the code would be overrun with commentary!).
>
> Shortened.
>
>> Aside: if you are a JDK Author you don't need a Contributed-by line in the commit comments.
>
>
> Removed.
>
> On Oct 8, 2013, at 4:08 AM, Alan Bateman wrote:
>
>> This looks much better.
>>
>> If I read the code correctly then the long standing behavior of URLClassLoader.getPermissions(null) was to throw a NPE, now it is changed to return the empty collection in order to match the super type (SecureClassLoader). I can't think of anything that would break so this is probably okay, just maybe a bit surprising.
>>
>> My previous comment on @throws vs. @exception was just to keep things consistent as this is old code and would look odd for a method to use both. Our preference is to switch to @throws whenever the opportunity arises. The comment was also on the alignment/style. Take URLCLassLoader(URL[], ClassLoader) for example, the existing SecurityException has its description aligned under SecurityException whereas the new NullPointerException wraps around. So my overall comment here was to avoid mixing styles (it doesn't matter too much which style is used, just keep it consistent for future maintainers).
>>
>> In the test then it might be better to change the @summary line to only include the second paragraph (the bug summary is not interesting, especially when it references a test that is not in OpenJDK).
>
>
> Updated.
>
> On Oct 8, 2013, at 8:53 AM, Chris Hegarty wrote:
>
>> Yes, this is much nicer.
>>
>> I'd be tempted to write another private constructor, taking all args, and checking params with checkForNullURLs before invoking, similar to ThreadGroup [1] L117, for example. But that is just clean up to remove some duplication. No need to do this.
>>
>>> If I read the code correctly then the long standing behavior of
>>> URLClassLoader.getPermissions(null) was to throw a NPE, now it is
>>> changed to return the empty collection in order to match the super type
>>> (SecureClassLoader). I can't think of anything that would break so this
>>> is probably okay, just maybe a bit surprising.
>>
>> Agreed.
>>
>>> My previous comment on @throws vs. @exception was just to keep things
>>> consistent as this is old code and would look odd for a method to use
>>> both. Our preference is to switch to @throws whenever the opportunity
>>> arises. The comment was also on the alignment/style. Take
>>> URLCLassLoader(URL[], ClassLoader) for example, the existing
>>> SecurityException has its description aligned under SecurityException
>>> whereas the new NullPointerException wraps around. So my overall comment
>>> here was to avoid mixing styles (it doesn't matter too much which style
>>> is used, just keep it consistent for future maintainers).
>>
>> Right, these classes are prime for stylistic clean up early in 9. For now, keep things locally consistent  ( even if that is old style ).
>>
>>
>> -Chris.
>>
>> [1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/9f57d2774603/src/share/classes/java/lang/ThreadGroup.java
>