Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 16, 2013, at 4:46 AM, Alan Bateman wrote: > This version looks okay too. For JDK-8026517 then you could add a note to say > that the tests for it are in the NullURLTest.java test case, alternatively > remove the commented-out tests and leave it to when JDK-8026517 is > re-examined. I'll update 8026517 with a note about the test and integrate this change after the CCC request has been approved. Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 15/10/2013 21:58, Brian Burkhalter wrote: Pursuant to comments on the submitted CCC request, the patch previously approved for this issue has been revised: http://cr.openjdk.java.net/~bpb/7179567/webrev.6/ The difference is that the URLClassLoader constructors and newInstance() methods no longer throw a NPE if the URL array parameter is non-null but contains at least one null element. The javadoc and test have been adjusted accordingly. A new issue https://bugs.openjdk.java.net/browse/JDK-8026517 has been filed to track adding a test for a null element in the URL array parameter. This latter issue is not presently scheduled to be addressed in JDK 8. Thanks, Brian This version looks okay too. For JDK-8026517 then you could add a note to say that the tests for it are in the NullURLTest.java test case, alternatively remove the commented-out tests and leave it to when JDK-8026517 is re-examined. -Alan.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Pursuant to comments on the submitted CCC request, the patch previously approved for this issue has been revised: http://cr.openjdk.java.net/~bpb/7179567/webrev.6/ The difference is that the URLClassLoader constructors and newInstance() methods no longer throw a NPE if the URL array parameter is non-null but contains at least one null element. The javadoc and test have been adjusted accordingly. A new issue https://bugs.openjdk.java.net/browse/JDK-8026517 has been filed to track adding a test for a null element in the URL array parameter. This latter issue is not presently scheduled to be addressed in JDK 8. Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 11, 2013, at 8:42 AM, Alan Bateman wrote: > On 11/10/2013 16:15, Brian Burkhalter wrote: >> Thanks. >> >> Any further comments from anyone else? >> >> Brian >> > I'm happy to see getPermissions changed to just specify long standing > behavior (the original proposal was unlikely to break anyone but it was > surprising). So I think what you have is fine now. Great! Thanks everyone for the comments. I'll file the CCC request today. Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 11/10/2013 16:15, Brian Burkhalter wrote: Thanks. Any further comments from anyone else? Brian I'm happy to see getPermissions changed to just specify long standing behavior (the original proposal was unlikely to break anyone but it was surprising). So I think what you have is fine now. -Alan.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Looks fine to me. Michael On 11/10/13 16:15, Brian Burkhalter wrote: Thanks. Any further comments from anyone else? Brian On Oct 10, 2013, at 9:04 PM, David Holmes wrote: Ship it! :) Thanks, David On 11/10/2013 5:24 AM, Brian Burkhalter wrote: On Oct 10, 2013, at 11:21 AM, Brian Burkhalter wrote: On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote: Nit: In the test there are a few places where you have t on a line by itself: 65t); but it can go on the previous line and not exceed the length of other lines nearby. Also "+ should be " + I don't see what you are referring to. Oh, sorry, now I do. Will update. I hope that as of now all nits have been picked. http://cr.openjdk.java.net/~bpb/7179567/webrev.5/ Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 11/10/2013 05:04, David Holmes wrote: Ship it! :) +1 -Chris. Thanks, David On 11/10/2013 5:24 AM, Brian Burkhalter wrote: On Oct 10, 2013, at 11:21 AM, Brian Burkhalter wrote: On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote: Nit: In the test there are a few places where you have t on a line by itself: 65 t); but it can go on the previous line and not exceed the length of other lines nearby. Also "+ should be " + I don't see what you are referring to. Oh, sorry, now I do. Will update. I hope that as of now all nits have been picked. http://cr.openjdk.java.net/~bpb/7179567/webrev.5/ Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Thanks. Any further comments from anyone else? Brian On Oct 10, 2013, at 9:04 PM, David Holmes wrote: > Ship it! :) > > Thanks, > David > > On 11/10/2013 5:24 AM, Brian Burkhalter wrote: >> >> On Oct 10, 2013, at 11:21 AM, Brian Burkhalter wrote: >> >>> On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote: >>> > Nit: In the test there are a few places where you have t on a line by > itself: > > 65t); > > but it can go on the previous line and not exceed the length of other > lines nearby. Also "+ should be " + I don't see what you are referring to. >>> >>> Oh, sorry, now I do. Will update. >> >> I hope that as of now all nits have been picked. >> >> http://cr.openjdk.java.net/~bpb/7179567/webrev.5/ >> >> Thanks, >> >> Brian >>
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Ship it! :) Thanks, David On 11/10/2013 5:24 AM, Brian Burkhalter wrote: On Oct 10, 2013, at 11:21 AM, Brian Burkhalter wrote: On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote: Nit: In the test there are a few places where you have t on a line by itself: 65t); but it can go on the previous line and not exceed the length of other lines nearby. Also "+ should be " + I don't see what you are referring to. Oh, sorry, now I do. Will update. I hope that as of now all nits have been picked. http://cr.openjdk.java.net/~bpb/7179567/webrev.5/ Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 10, 2013, at 11:21 AM, Brian Burkhalter wrote: > On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote: > >>> Nit: In the test there are a few places where you have t on a line by >>> itself: >>> >>> 65t); >>> >>> but it can go on the previous line and not exceed the length of other lines >>> nearby. Also "+ should be " + >> >> I don't see what you are referring to. > > Oh, sorry, now I do. Will update. I hope that as of now all nits have been picked. http://cr.openjdk.java.net/~bpb/7179567/webrev.5/ Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote: >> Nit: In the test there are a few places where you have t on a line by itself: >> >> 65t); >> >> but it can go on the previous line and not exceed the length of other lines >> nearby. Also "+ should be " + > > I don't see what you are referring to. Oh, sorry, now I do. Will update. Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 8:32 PM, David Holmes wrote:
> In the text:
>
> * @exception NullPointerException if {@code urls} or any of its elements
> * is {@code null}
>
> I'm unsure if it should be "is null" or "are null" given the initial subject
> is singular and latter subject is plural. Existing similar phrases seem to be
> split roughly 50-50.
I have changed these to
* @exception NullPointerException if {@code urls} or at least one of its
* elements is {@code null}.
Update: http://cr.openjdk.java.net/~bpb/7179567/webrev.5/
> Nit: In the test there are a few places where you have t on a line by itself:
>
> 65t);
>
> but it can go on the previous line and not exceed the length of other lines
> nearby. Also "+ should be " +
I don't see what you are referring to.
Thanks,
Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 10/10/2013 2:07 PM, John Rose wrote: On Oct 9, 2013, at 8:32 PM, David Holmes wrote: I'm unsure if it should be "is null" or "are null" given the initial subject is singular and latter subject is plural. The English major in me surfaces briefly to note that "any" can be either singular or plural: "Pick a card, any card" (singular) vs. "any takers?" (plural). Brian's works well enough as a singular. Etymologically, "any" (like "an") is a cognate of the word "one". It isn't the "any" that introduces the plural but "elements". David — John
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 8:32 PM, David Holmes wrote: > I'm unsure if it should be "is null" or "are null" given the initial subject > is singular and latter subject is plural. The English major in me surfaces briefly to note that "any" can be either singular or plural: "Pick a card, any card" (singular) vs. "any takers?" (plural). Brian's works well enough as a singular. Etymologically, "any" (like "an") is a cognate of the word "one". — John
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Hi Brian,
In the text:
* @exception NullPointerException if {@code urls} or any of its elements
* is {@code null}
I'm unsure if it should be "is null" or "are null" given the initial
subject is singular and latter subject is plural. Existing similar
phrases seem to be split roughly 50-50.
Nit: In the test there are a few places where you have t on a line by
itself:
65t);
but it can go on the previous line and not exceed the length of other
lines nearby. Also "+ should be " +
Thanks,
David
On 10/10/2013 6:01 AM, Brian Burkhalter wrote:
On Oct 9, 2013, at 3:06 AM, Michael McMahon wrote:
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.
I'm still not sure about that change. Its effect in sub-classes will be to
continue on in the sub-class implementation
of getPermissions() with a null codesource parameter, where currently NPE would
be thrown.
Well, upon re-examination I actually agree with you, especially considering the
class hierarchy:
AppletClassLoader :: URLClassLoader :: SecureClassLoader :: ClassLoader
I have posted an updated webrev here:
http://cr.openjdk.java.net/~bpb/7179567/webrev.4/
The differences with respect to the previous webrev [1] are:
1) the javadoc change of SecureClassLoader has been reverted;
2) these statements
665 if (codesource == null) {
666 return perms;
667 }
668
in URLClassLoader.getPermissions() have been reverted;
3) the javadoc of getPermissions() in URLClassLoader and AppletClassLoader has been
updated to add "@exception NullPointerException if {@code codesource} is {@code
null}."
The CCC request to be associated with this change would indicate that "@exception
NullPointerException" has been added to the javadoc of:
A) all public URLClassLoader constructors;
B) URLClassLoader.findClass();
C) URLClassLoader.getPermissions();
D) both URLClassLoader.newInstance() methods;
E) AppletClassLoader.getPermissions().
The only actual *code* changes are in URLClassLoader.
Thanks,
Brian
[1] http://cr.openjdk.java.net/~bpb/7179567/webrev.3/
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 3:06 AM, Michael McMahon wrote:
>> 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.
>>
>
> I'm still not sure about that change. Its effect in sub-classes will be to
> continue on in the sub-class implementation
> of getPermissions() with a null codesource parameter, where currently NPE
> would be thrown.
Well, upon re-examination I actually agree with you, especially considering the
class hierarchy:
AppletClassLoader :: URLClassLoader :: SecureClassLoader :: ClassLoader
I have posted an updated webrev here:
http://cr.openjdk.java.net/~bpb/7179567/webrev.4/
The differences with respect to the previous webrev [1] are:
1) the javadoc change of SecureClassLoader has been reverted;
2) these statements
665 if (codesource == null) {
666 return perms;
667 }
668
in URLClassLoader.getPermissions() have been reverted;
3) the javadoc of getPermissions() in URLClassLoader and AppletClassLoader has
been updated to add "@exception NullPointerException if {@code codesource} is
{@code null}."
The CCC request to be associated with this change would indicate that
"@exception NullPointerException" has been added to the javadoc of:
A) all public URLClassLoader constructors;
B) URLClassLoader.findClass();
C) URLClassLoader.getPermissions();
D) both URLClassLoader.newInstance() methods;
E) AppletClassLoader.getPermissions().
The only actual *code* changes are in URLClassLoader.
Thanks,
Brian
[1] http://cr.openjdk.java.net/~bpb/7179567/webrev.3/
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 1:56 AM, Chris Hegarty wrote: > 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. Thanks. > Since there are minor spec clarifications, to document long standing > behavior, then a CCC request will need to be submitted, and approved, before > integration. Yes, I intend to do that. I was hoping to get a consensus on the review first but I will file the request this week in any case. Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 08/10/13 12:08, Alan Bateman wrote: On 04/10/2013 21:58, Brian Burkhalter wrote: : An updated webrev which I hope adequately addresses the expressed concerns may be found at: http://cr.openjdk.java.net/~bpb/7179567.2/ 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. I'm still not sure about that change. Its effect in sub-classes will be to continue on in the sub-class implementation of getPermissions() with a null codesource parameter, where currently NPE would be thrown. Michael
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
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 mix
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
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 pri
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 10/08/2013 12:08 PM, Alan Bateman wrote: On 04/10/2013 21:58, Brian Burkhalter wrote: : An updated webrev which I hope adequately addresses the expressed concerns may be found at: http://cr.openjdk.java.net/~bpb/7179567.2/ This looks much better. 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 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). -Alan.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 04/10/2013 21:58, Brian Burkhalter wrote: : An updated webrev which I hope adequately addresses the expressed concerns may be found at: http://cr.openjdk.java.net/~bpb/7179567.2/ 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). -Alan.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Hi Brian,
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.
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);
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?).
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!).
Aside: if you are a JDK Author you don't need a Contributed-by line in
the commit comments.
David
-
Will you be adding tests for these cases to the webrev?
As needed once the concept in general is accepted.
The foregoing webrev includes a test of the affected public methods.
Thanks,
Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 7, 2013, at 2:47 PM, Mike Duigou wrote:
>> An updated webrev which I hope adequately addresses the expressed concerns
>> may be found at:
>>
>> http://cr.openjdk.java.net/~bpb/7179567.2/
>
> Looks good to me.
>
> Does the addition of "If {@code codesource} is {@code null} the returned
> {@code PermissionCollection} is empty." constitute a spec change or just a
> clarification? I see the URClassLoader change @@ -625,10 +661,14 @@ but am
> unsure.
To me it's a clarification but it would be helpful for someone else to weight
in on the question.
Thanks,
Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 4 2013, at 13:58 , 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/
Looks good to me.
Does the addition of "If {@code codesource} is {@code null} the returned {@code
PermissionCollection} is empty." constitute a spec change or just a
clarification? I see the URClassLoader change @@ -625,10 +661,14 @@ but am
unsure.
Mike
>
>>
>>> Will you be adding tests for these cases to the webrev?
>>
>> As needed once the concept in general is accepted.
>
> The foregoing webrev includes a test of the affected public methods.
>
> Thanks,
>
> Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
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/ > >> Will you be adding tests for these cases to the webrev? > > As needed once the concept in general is accepted. The foregoing webrev includes a test of the affected public methods. Thanks, Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On 04/10/13 01:35, 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/ Summary * Document and throw IAE from URLClassLoader constructors. If urls is null then it looks like the long standing behavior is to throw NPE. If so then would be simpler to just specify that rather than switching to IAE? I agree. Documenting NPE seems reasonable to me. I'm not convinced of the need for any behavior changes in the class. Michael. : * Clarify specification of SecureClassLoader.getPermissions. A minor comment on SecureClassLoader is that you might want to align the @throws to match the other @exception usages in the class. Will you be adding tests for these cases to the webrev? -Alan.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
+1 on NPE rather than IAE - this is the common approach used elsewhere. It also means some explicit checks can be removed. BUT these explicit null URL checks could be relatively expensive. The same for the findClass(String name) change. It just bugs me that we have to slow down correct code with explicit null checks. For findClass if you just let NPE be thrown then we should trigger it without need for an explicit check. No comment on getPermissions. (This seems to be a mixed bag of issues). David On 4/10/2013 10:38 AM, 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/ Summary * Document and throw IAE from URLClassLoader constructors. If urls is null then it looks like the long standing behavior is to throw NPE. If so then would be simpler to just specify that rather than switching to IAE? It would be. I think the main point is to catch the null parameter as soon as possible. I can easily do the s/IAE/NPE/ change in the revision. * Clarify specification of SecureClassLoader.getPermissions. A minor comment on SecureClassLoader is that you might want to align the @throws to match the other @exception usages in the class. I saw that difference but am used to using "@throws." Is there any preference of one over the other in new code? Will you be adding tests for these cases to the webrev? As needed once the concept in general is accepted. Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
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/ >> >> Summary >> * Document and throw IAE from URLClassLoader constructors. > If urls is null then it looks like the long standing behavior is to throw > NPE. If so then would be simpler to just specify that rather than switching > to IAE? It would be. I think the main point is to catch the null parameter as soon as possible. I can easily do the s/IAE/NPE/ change in the revision. >> * Clarify specification of SecureClassLoader.getPermissions. >> > A minor comment on SecureClassLoader is that you might want to align the > @throws to match the other @exception usages in the class. I saw that difference but am used to using "@throws." Is there any preference of one over the other in new code? > Will you be adding tests for these cases to the webrev? As needed once the concept in general is accepted. Brian
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
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/ Summary * Document and throw IAE from URLClassLoader constructors. If urls is null then it looks like the long standing behavior is to throw NPE. If so then would be simpler to just specify that rather than switching to IAE? : * Clarify specification of SecureClassLoader.getPermissions. A minor comment on SecureClassLoader is that you might want to align the @throws to match the other @exception usages in the class. Will you be adding tests for these cases to the webrev? -Alan.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 3 2013, at 17:12 , Brian Burkhalter wrote: > So if s/IAE/NPE/ then the rest appears within reason? Yes. I would also changes the CNFE to an NPE. I have no comment (and total ignorance) on the SecureClassLoader changes. I couldn't approve this change without more research. Mike > Brian > > On Oct 3, 2013, at 4:53 PM, Mike Duigou wrote: > >> I would normally suggest throwing NPE rather than other exceptions for >> detected invalid nulls. You can also then use Objects.requireNonNull() >> >> Mike >> >> On Oct 3 2013, at 16:10 , Brian Burkhalter wrote: >> >>> >>> Summary >>> * Document and throw IAE from URLClassLoader constructors. >>> * Throw ClassNotFoundException if null is passed to findClass method of a >>> ClassLoader. >>> * Clarify specification of SecureClassLoader.getPermissions. >
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
So if s/IAE/NPE/ then the rest appears within reason? Brian On Oct 3, 2013, at 4:53 PM, Mike Duigou wrote: > I would normally suggest throwing NPE rather than other exceptions for > detected invalid nulls. You can also then use Objects.requireNonNull() > > Mike > > On Oct 3 2013, at 16:10 , Brian Burkhalter wrote: > >> >> Summary >> * Document and throw IAE from URLClassLoader constructors. >> * Throw ClassNotFoundException if null is passed to findClass method of a >> ClassLoader. >> * Clarify specification of SecureClassLoader.getPermissions.
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
I would normally suggest throwing NPE rather than other exceptions for detected invalid nulls. You can also then use Objects.requireNonNull() Mike On Oct 3 2013, at 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/ > > Summary > * Document and throw IAE from URLClassLoader constructors. > * Throw ClassNotFoundException if null is passed to findClass method of a > ClassLoader. > * Clarify specification of SecureClassLoader.getPermissions. > > This change would require an approved CCC request and JCK test changes. > > Thanks, > > Brian
