Re: RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Chris Hegarty]

> On 15 Jun 2016, at 19:33, Mark Sheppard  wrote:
> 
> Hi,
> please oblige and review the updated webrev:
> 
> http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev.01/

I’m happy with the code changes here.

Trivially, an alternative stylistic option for the method declaration:

private static Field getDeclaredField(final Class c,
  final String 
fieldName)
throws PrivilegedActionException, NoSuchFieldException, 
SecurityException
{

-Chris.

Re: JDK 9 RFR of JDK-8071859: AnnotationInvocationHandler.equals(Object) return true when apply to annotation

[Mandy Chung]

> On Jun 15, 2016, at 4:21 PM, Joseph D. Darcy  wrote:
> 
> Hello,
> 
> Please review the changes to address
> 
>JDK-8071859: AnnotationInvocationHandler.equals(Object) return true when 
> apply to annotation
>http://cr.openjdk.java.net/~darcy/8071859.0/

+1

Mandy

Re: JDK 9 RFR of JDK-8071859: AnnotationInvocationHandler.equals(Object) return true when apply to annotation

[Chris Hegarty]

On 16 Jun 2016, at 00:21, Joseph D. Darcy  wrote:
> 
> Hello,
> 
> Please review the changes to address
> 
>JDK-8071859: AnnotationInvocationHandler.equals(Object) return true when 
> apply to annotation
>http://cr.openjdk.java.net/~darcy/8071859.0/

This looks good to me Joe.

-Chris.

Re: RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Mark Sheppard]

thanks Roger ... I'll attend to them

regards
Mark

On 15/06/2016 21:57, Roger Riggs wrote:


Hi Mark,

Thanks for the test cleanup.

In the new security policy files, is it important to enumerate the 
individual permissions instead of using AllPermission?
If it is significant, the perhaps a comment is in order, otherwise, 
there will be doubt about why the long list instead of the simpler 
AllPermission.



ConcurrentHashMapTest, line 128/137: has some commented out code that 
can be removed (or explained).


HelloServer.java: line 36: more commented out code to be removed

Since _HelloImpl_Tie.java and _HelloInterface_Stub.java are being 
committed, please add the copyright header.



I didn't get the RmiIiopReturnValueTest to run both invocations; the 
second one died with could not initialize class javax.rmi.CORBA.Util.  
But I didn't take the time to debug it.  It probably works for you.


In RmiIiopReturnValueTest, there are a few unused imports, 
DataInputStream, File, TimeUnit, CountDownLatch that could be removed.


Thanks, Roger


On 6/15/2016 2:33 PM, Mark Sheppard wrote:

Hi,
 please oblige and review the updated webrev:

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev.01/
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev.01/

based on the feedback, the following amendments were made:

the scope of the tests reduced

the removal of the export fro module-info.java -
it transpires that the granting of all permissions to the corba 
module in the policy file, mitigates the

need for this addition

added check for SecurityManager and perform doPrivileged for 
getDeclaredField


cleanups suggestions from  Sean

regards
Mar

On 09/06/2016 23:27, Mark Sheppard wrote:

thanks Roger ... will reduce the focus of the test

regards
Mark

On 09/06/2016 15:58, Roger Riggs wrote:

Hi Mark,

With respect to the test,  it seems to do a lot more than just be 
the test for the current issue.
Perhaps some javadoc in the test could identify the the important 
parts of the test

as opposed to the supporting infrastructure.

If there is more code than is needed for this issue then perhaps 
the test should be whittled down
to just what is needed for this issue or should be promoted to a 
more general test with a regular name
(not in a subdirectory with the bug number).  I think this has been 
our direction anyway, to

write tests for the functionality and not only for a specific issue.

Thanks, Roger



On 6/9/2016 10:18 AM, Roger Riggs wrote:

Hi Mark,

IIOPInputStream.java:

- Does adding doPriv overhead to every field access have a 
noticeable impact on performance?
  If most of the fields are accessible or can easily be checked as 
being in the base module,
  I could see trying the access directly and catching  the 
security exception to retry with doPriv.


Roger

ps.

There is a newer version of webrev that provides convenient next 
and previous links.


http://hg.openjdk.java.net/code-tools/webrev/raw-file/d26c194772db/webrev.ksh 





On 6/9/2016 8:57 AM, Seán Coffey wrote:

Hey Mark,

Looks fine. A few comments.


2291 Class declaredFieldClass = declaredClassField.getType();
2292
2293 if (declaredFieldClass == null) {
2294 continue;
2295 }

I'm not sure if this check is required. Can getType() return null ?

==


2833 Field classField;
2834 classField = c
2835 .getDeclaredField(fieldName);
2836 return classField;
Could we fold that code down to 'return 
c.getDeclaredField(fieldName);' ?


For the tests :
==
ConcurrentHashMapTest.java :
you might want to remove the old rmiServerProcess.destroy(), 
orbdProcess.destroyForcibly() comments.

==
HelloClient.java
line 157 : s/1000/ONE_SECOND

The use of static ports could be one to watch. If we encounter 
port issues, the test might have to be revisited.


Regards,
Sean.

On 08/06/2016 23:18, Mark Sheppard wrote:


Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev/

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev/

which address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8146975

the type checking in inputClassFields and other places failed to 
fully allowing for
the processing of return ValueTypes, and hence the 
getDeclaredField fails as
"application code" exist  on the call stack restricting access. 
This leads to a security exception,
which in turn leads to an IllegalArgumentExcetption, the 
processing of which failed to allow

for a null object value in the stream.
This has now been rectified, with the getDeclaredField wrapped 
in a doPrivileged call.


regards
Mark

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Joseph D. Darcy]

Steve,

In JarFile, please use methods not fields to return the new information. 
The information in question is not constant across versions. Using 
methods instead of fields avoid over-committing on a particular 
implementation, etc.


Cheers,

-Joe

On 6/15/2016 3:49 PM, Steve Drach wrote:

I’ve updated the webrev to address the issue of the constructor accepting 
values like Version.parse(“7.1”)

http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 



On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:


Please review the following changeset:

webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 

issue: https://bugs.openjdk.java.net/browse/JDK-8150680 


The issue calls for reconsidering the JarFile.Release enum.  A comment in the 
bug report suggests replacing JarFile.Release with Runtime.Version, and that’s 
what I did.  Specifically I removed the enum, changed the constructor to accept 
a Runtime.Version object instead of a JarFile.Release object, updated all 
places in the JDK that invoked the constructor and updated all tests.


Moving to Runtime.Version seems right but doesn't the javadoc for the constructor need to 
be updated to make it clear how it behavior when invoking with something like 
Version.parse("7.1") ? If I read the code correctly then this will be accepted 
and getVersion() will return 7.1.

Yes, it needs to be updated and it needs to be fixed.  Thanks for finding that.


Fields or methods is another discussion point for the base and runtime versions.

My thinking is, in this case fields and methods are equivalent, the method not 
giving any more flexibility than a field.  For example the method 
JarFile.baseVersion will just return the value contained in the private final 
static field BASE_VERSION.  Or the public final static field BASE_VERSION can 
be directly accessed.  I see no advantage of a method here.  But I’m willing to 
be enlightened.


-Alan.

JDK 9 RFR of JDK-8071859: AnnotationInvocationHandler.equals(Object) return true when apply to annotation

[Joseph D. Darcy]

Hello,

Please review the changes to address

JDK-8071859: AnnotationInvocationHandler.equals(Object) return true 
when apply to annotation

http://cr.openjdk.java.net/~darcy/8071859.0/

Thanks,

-Joe

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

I’ve updated the webrev to address the issue of the constructor accepting 
values like Version.parse(“7.1”)

http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 


> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
> 
>>> Please review the following changeset:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>>> 
>>> 
>>> The issue calls for reconsidering the JarFile.Release enum.  A comment in 
>>> the bug report suggests replacing JarFile.Release with Runtime.Version, and 
>>> that’s what I did.  Specifically I removed the enum, changed the 
>>> constructor to accept a Runtime.Version object instead of a JarFile.Release 
>>> object, updated all places in the JDK that invoked the constructor and 
>>> updated all tests.
>>> 
>> Moving to Runtime.Version seems right but doesn't the javadoc for the 
>> constructor need to be updated to make it clear how it behavior when 
>> invoking with something like Version.parse("7.1") ? If I read the code 
>> correctly then this will be accepted and getVersion() will return 7.1.
> 
> Yes, it needs to be updated and it needs to be fixed.  Thanks for finding 
> that.
> 
>> 
>> Fields or methods is another discussion point for the base and runtime 
>> versions.
> 
> My thinking is, in this case fields and methods are equivalent, the method 
> not giving any more flexibility than a field.  For example the method 
> JarFile.baseVersion will just return the value contained in the private final 
> static field BASE_VERSION.  Or the public final static field BASE_VERSION can 
> be directly accessed.  I see no advantage of a method here.  But I’m willing 
> to be enlightened.
> 
>> 
>> -Alan.
> 

Re: RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Roger Riggs]

fyi Mark,

The problem running the new test was a bad build on my end, sorry for 
the noise.


Roger


On 6/15/2016 4:57 PM, Roger Riggs wrote:

Hi Mark,

Thanks for the test cleanup.

In the new security policy files, is it important to enumerate the 
individual permissions instead of using AllPermission?
If it is significant, the perhaps a comment is in order, otherwise, 
there will be doubt about why the long list instead of the simpler 
AllPermission.



ConcurrentHashMapTest, line 128/137: has some commented out code that 
can be removed (or explained).


HelloServer.java: line 36: more commented out code to be removed

Since _HelloImpl_Tie.java and _HelloInterface_Stub.java are being 
committed, please add the copyright header.



I didn't get the RmiIiopReturnValueTest to run both invocations; the 
second one died with could not initialize class javax.rmi.CORBA.Util.  
But I didn't take the time to debug it.  It probably works for you.


In RmiIiopReturnValueTest, there are a few unused imports, 
DataInputStream, File, TimeUnit, CountDownLatch that could be removed.


Thanks, Roger


On 6/15/2016 2:33 PM, Mark Sheppard wrote:

Hi,
 please oblige and review the updated webrev:

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev.01/
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev.01/

based on the feedback, the following amendments were made:

the scope of the tests reduced

the removal of the export fro module-info.java -
it transpires that the granting of all permissions to the corba 
module in the policy file, mitigates the

need for this addition

added check for SecurityManager and perform doPrivileged for 
getDeclaredField


cleanups suggestions from  Sean

regards
Mar

On 09/06/2016 23:27, Mark Sheppard wrote:

thanks Roger ... will reduce the focus of the test

regards
Mark

On 09/06/2016 15:58, Roger Riggs wrote:

Hi Mark,

With respect to the test,  it seems to do a lot more than just be 
the test for the current issue.
Perhaps some javadoc in the test could identify the the important 
parts of the test

as opposed to the supporting infrastructure.

If there is more code than is needed for this issue then perhaps 
the test should be whittled down
to just what is needed for this issue or should be promoted to a 
more general test with a regular name
(not in a subdirectory with the bug number).  I think this has been 
our direction anyway, to

write tests for the functionality and not only for a specific issue.

Thanks, Roger



On 6/9/2016 10:18 AM, Roger Riggs wrote:

Hi Mark,

IIOPInputStream.java:

- Does adding doPriv overhead to every field access have a 
noticeable impact on performance?
  If most of the fields are accessible or can easily be checked as 
being in the base module,
  I could see trying the access directly and catching  the 
security exception to retry with doPriv.


Roger

ps.

There is a newer version of webrev that provides convenient next 
and previous links.


http://hg.openjdk.java.net/code-tools/webrev/raw-file/d26c194772db/webrev.ksh 





On 6/9/2016 8:57 AM, Seán Coffey wrote:

Hey Mark,

Looks fine. A few comments.


2291 Class declaredFieldClass = declaredClassField.getType();
2292
2293 if (declaredFieldClass == null) {
2294 continue;
2295 }

I'm not sure if this check is required. Can getType() return null ?

==


2833 Field classField;
2834 classField = c
2835 .getDeclaredField(fieldName);
2836 return classField;
Could we fold that code down to 'return 
c.getDeclaredField(fieldName);' ?


For the tests :
==
ConcurrentHashMapTest.java :
you might want to remove the old rmiServerProcess.destroy(), 
orbdProcess.destroyForcibly() comments.

==
HelloClient.java
line 157 : s/1000/ONE_SECOND

The use of static ports could be one to watch. If we encounter 
port issues, the test might have to be revisited.


Regards,
Sean.

On 08/06/2016 23:18, Mark Sheppard wrote:


Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev/

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev/

which address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8146975

the type checking in inputClassFields and other places failed to 
fully allowing for
the processing of return ValueTypes, and hence the 
getDeclaredField fails as
"application code" exist  on the call stack restricting access. 
This leads to a security exception,
which in turn leads to an IllegalArgumentExcetption, the 
processing of which failed to allow

for a null object value in the stream.
This has now been rectified, with the getDeclaredField wrapped 
in a doPrivileged call.


regards
Mark

Review Request: JDK-8159524 jdeps -jdkinternals throws NPE when no replacement is known

[Mandy Chung]

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8159524/webrev.00/index.html

Simple fix in jdeps -jdkinternals to handle properly when no replacement is 
known.

Mandy

Re: RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Roger Riggs]

Hi Mark,

Thanks for the test cleanup.

In the new security policy files, is it important to enumerate the 
individual permissions instead of using AllPermission?
If it is significant, the perhaps a comment is in order, otherwise, 
there will be doubt about why the long list instead of the simpler 
AllPermission.



ConcurrentHashMapTest, line 128/137: has some commented out code that 
can be removed (or explained).


HelloServer.java: line 36: more commented out code to be removed

Since _HelloImpl_Tie.java and _HelloInterface_Stub.java are being 
committed, please add the copyright header.



I didn't get the RmiIiopReturnValueTest to run both invocations; the 
second one died with could not initialize class javax.rmi.CORBA.Util.  
But I didn't take the time to debug it.  It probably works for you.


In RmiIiopReturnValueTest, there are a few unused imports, 
DataInputStream, File, TimeUnit, CountDownLatch that could be removed.


Thanks, Roger


On 6/15/2016 2:33 PM, Mark Sheppard wrote:

Hi,
 please oblige and review the updated webrev:

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev.01/
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev.01/

based on the feedback, the following amendments were made:

the scope of the tests reduced

the removal of the export fro module-info.java -
it transpires that the granting of all permissions to the corba module 
in the policy file, mitigates the

need for this addition

added check for SecurityManager and perform doPrivileged for 
getDeclaredField


cleanups suggestions from  Sean

regards
Mar

On 09/06/2016 23:27, Mark Sheppard wrote:

thanks Roger ... will reduce the focus of the test

regards
Mark

On 09/06/2016 15:58, Roger Riggs wrote:

Hi Mark,

With respect to the test,  it seems to do a lot more than just be 
the test for the current issue.
Perhaps some javadoc in the test could identify the the important 
parts of the test

as opposed to the supporting infrastructure.

If there is more code than is needed for this issue then perhaps the 
test should be whittled down
to just what is needed for this issue or should be promoted to a 
more general test with a regular name
(not in a subdirectory with the bug number).  I think this has been 
our direction anyway, to

write tests for the functionality and not only for a specific issue.

Thanks, Roger



On 6/9/2016 10:18 AM, Roger Riggs wrote:

Hi Mark,

IIOPInputStream.java:

- Does adding doPriv overhead to every field access have a 
noticeable impact on performance?
  If most of the fields are accessible or can easily be checked as 
being in the base module,
  I could see trying the access directly and catching  the security 
exception to retry with doPriv.


Roger

ps.

There is a newer version of webrev that provides convenient next 
and previous links.


http://hg.openjdk.java.net/code-tools/webrev/raw-file/d26c194772db/webrev.ksh 





On 6/9/2016 8:57 AM, Seán Coffey wrote:

Hey Mark,

Looks fine. A few comments.


2291 Class declaredFieldClass = declaredClassField.getType();
2292
2293 if (declaredFieldClass == null) {
2294 continue;
2295 }

I'm not sure if this check is required. Can getType() return null ?

==


2833 Field classField;
2834 classField = c
2835 .getDeclaredField(fieldName);
2836 return classField;
Could we fold that code down to 'return 
c.getDeclaredField(fieldName);' ?


For the tests :
==
ConcurrentHashMapTest.java :
you might want to remove the old rmiServerProcess.destroy(), 
orbdProcess.destroyForcibly() comments.

==
HelloClient.java
line 157 : s/1000/ONE_SECOND

The use of static ports could be one to watch. If we encounter 
port issues, the test might have to be revisited.


Regards,
Sean.

On 08/06/2016 23:18, Mark Sheppard wrote:


Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev/

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev/

which address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8146975

the type checking in inputClassFields and other places failed to 
fully allowing for
the processing of return ValueTypes, and hence the 
getDeclaredField fails as
"application code" exist  on the call stack restricting access. 
This leads to a security exception,
which in turn leads to an IllegalArgumentExcetption, the 
processing of which failed to allow

for a null object value in the stream.
This has now been rectified, with the getDeclaredField wrapped in 
a doPrivileged call.


regards
Mark

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

[Alex Strange]

> On Jun 13, 2016, at 1:58 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review this Mac-only fix:
> 
> http://cr.openjdk.java.net/~bchristi/7131356/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-7131356
> 
> Thanks go to Gerard Ziemski for the thorough investigation and detailed 
> write-up in the bug report.
> 
> The symptoms:
> 
> On those OS X machines where the default system Java is not installed, any 
> attempt to instantiate JVM from a local JDK (for example via JNI as shown in 
> the bug) presents "No Java installed. Do you want to install Java?" dialog 
> and refuses to run. Clearly, a valid local JDK should be able to run in this 
> case.
> 
> The problem:
> 
> In java_props_macosx.c, there is code that dynamically looks up 4 methods in 
> JavaRuntimeSupport.framework (JRS) and calls into them to find out localized 
> OS version, default locale and the preferred language, but JRS checks for a 
> system available Java and refuses to run if none found.
> 
> Background:
> 
> JavaRuntimeSupport.framework (JRS) is a framework implemented and provided by 
> Apple for use by Java.  It is a "black box" that wraps SPIs required by the 
> Apple-provided implementation of JDK, exposing them to the JDK as APIs.
> 
> Now that the JDK implementation ownership has changed from Apple to Oracle, 
> we have the issue that we don't control the JRS implementation, but rely on 
> Apple to support it for our JDK to continue to run. Depending on a private 
> framework provided by a third party for our code to continue to run doesn't 
> seem like a good long term bet (see also 8024281).
> 
> Proposed solution:
> 
> Apple suggested changing the JDK initialization order so any access to JRS 
> happens only after the JLI_MemAlloc symbol is available.  We believe a better 
> solution is to re-implement the JSR APIs in question, as a move toward 
> eventually dropping reliance on JSR altogether.  The three main changes are:
> 
> 1. In "getMacOSXLocale", re-implement "JRSCopyPrimaryLanguage" using 
> "CFLocaleCopyPreferredLanguages", which gives us the preferred language as 
> set in "System Preferences"->"Language & Text"->"Language" in the form of 
> "xx" (ie. just the language code - ex. "en", "fr", etc.).  The original Apple 
> code then calls into "JRSCopyCanonicalLanguageForPrimaryLanguage" to map 
> "language" into "language_REGION" (ex. "en"-->"en_US", "fr"-->"fr_FR", etc.), 
> though this appears to be unnecessary.  Following the call to 
> setupMacOSXLocale() in ParseLocale()[1], mapLookup() is called to map the 
> language to a default country, per this comment:
> 
> * If the locale name (without .encoding@variant, if any) matches
> * any of the names in the locale_aliases list, map it to the
> * corresponding full locale name.  Most of the entries in the
> * locale_aliases list are locales that include a language name but
> * no country name, and this facility is used to map each language
> * to a default country if that's possible.
> 
> I tried out a few locales, for English (en_US, en_GB), German (de_DE, de_CH) 
> and French (fr_FR, fr_CA).  Language/country system properties behave the 
> same before and after this change, as does the java.util.Locale test attached 
> to the bug.

I think JRSCopyCanonicalLanguageForPrimaryLanguage() may be related to app 
bundles which don't have localizations in the user's language (for instance a 
Japanese-only app run on en_US). But I don't have an example of such an app or 
remember what the right behavior would be.

> 
> 2. In "setupMacOSXLocale" we simply drop the call to 
> "JRSSetDefaultLocalization" as it appears to be a NOP. According to Apple, 
> that API sets up native bundle locale, so that any access to native Cocoa UI 
> (like FileOpenChooser) uses localized strings. Testing shows that this does 
> not seem to work even in Apple's own JDK (ie. JDK 6), so dropping the call to 
> this SPI here does not result in a regression.  An issue was filed to 
> investigate further (8024279, a dup of 8019464) which has since been closed 
> as, "Not an Issue".

This was added a very long time ago so that 'java -jar x.jar' would show 
properly localized menus in the menubar, instead of English menus, on a 
non-English system. It might no longer be a problem.

> 
> 3. In "setOSNameAndVersion", re-implement JRSCopyOSVersion using 
> [NSProcessInfo operatingSystemVersion].  (Use of JRSCopyOSName was already 
> removed by 7178922).

You shouldn't need to use objc_msgSend_stret here. If you're not getting a 
warning when you use @selector in the line above, you should just be able to 
call -operationgSystemVersion directly inside the if.

If you are getting a warning, it'd be best to declare the selector yourself 
somewhere higher up:

typedef struct {
NSInteger majorVersion;
NSInteger minorVersion;
NSInteger patchVersion;
} OSVerStruct;

@interface NSProcessInfo ()
- (OSVerStruct)operatingSystemVersion;
@end

> 
> 
> Th

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Peter Levart]

Hi Steve,


On 06/15/2016 05:56 PM, Steve Drach wrote:

>Fields or methods is another discussion point for the base and runtime 
versions.

My thinking is, in this case fields and methods are equivalent, the method not 
giving any more flexibility than a field.  For example the method 
JarFile.baseVersion will just return the value contained in the private final 
static field BASE_VERSION.  Or the public final static field BASE_VERSION can 
be directly accessed.  I see no advantage of a method here.  But I’m willing to 
be enlightened.



If you make the static final field part of public API, then you 
constrain yourself to initializing it in class initializer. If you keep 
it private and expose it via a static method now, then you keep the 
freedom to initialize it lazily if it ever happens to be needed in the 
future because of various reasons such as initialization circularity. 
But I don't know if this applies in your case. Would you ever need to 
use part of JarFile class early in bootup sequence before those fields 
can be initialized?


Regards, Peter

Re: RFR: 8065831: Ensure the pack200/unpack200 help is consistent with man page

[Mandy Chung]

> On Jun 15, 2016, at 11:34 AM, Kumar Srinivasan 
>  wrote:
> 
> 
> Oops, I missed making changes to the unpack200 executable's
> help strings, no changes to DriverResource.java.
> 
> http://cr.openjdk.java.net/~ksrini/8065831/webrev.02/

+1

Mandy

Re: Review request: JDK-8068764 java/lang/ClassLoader/ExtDirs.java failed with Exception java.lang.IllegalThreadStateException: process hasn't exited

[Alan Bateman]

On 15/06/2016 19:17, Mandy Chung wrote:

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8068764/webrev.00/

The ExtDirs and EndorsedDirs tests might fail due to the insufficient wait time 
on fastdebug build on a slow machine.

This patch converts the tests to use the test library ProcessTools that will 
wait for the child process to finish or destroy it when timeout.


This looks okay to me.

-Alan

Re: RFR: 8065831: Ensure the pack200/unpack200 help is consistent with man page

[Kumar Srinivasan]

Oops, I missed making changes to the unpack200 executable's
help strings, no changes to DriverResource.java.

http://cr.openjdk.java.net/~ksrini/8065831/webrev.02/

Just finished verifying with jprt.

Thanks
Kumar


On Jun 14, 2016, at 6:31 PM, Kumar Srinivasan  
wrote:

Thanks for looking at this. I wrapped all of them.

http://cr.openjdk.java.net/~ksrini/8065831/webrev.01/



Looks fine.  Disclaimer - I didn’t validate with the current implementation :)

Mandy

Re: RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Mark Sheppard]

Hi,
 please oblige and review the updated webrev:

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev.01/
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev.01/

based on the feedback, the following amendments were made:

the scope of the tests reduced

the removal of the export fro module-info.java -
it transpires that the granting of all permissions to the corba module 
in the policy file, mitigates the

need for this addition

added check for SecurityManager and perform doPrivileged for 
getDeclaredField


cleanups suggestions from  Sean

regards
Mar

On 09/06/2016 23:27, Mark Sheppard wrote:

thanks Roger ... will reduce the focus of the test

regards
Mark

On 09/06/2016 15:58, Roger Riggs wrote:

Hi Mark,

With respect to the test,  it seems to do a lot more than just be the 
test for the current issue.
Perhaps some javadoc in the test could identify the the important 
parts of the test

as opposed to the supporting infrastructure.

If there is more code than is needed for this issue then perhaps the 
test should be whittled down
to just what is needed for this issue or should be promoted to a more 
general test with a regular name
(not in a subdirectory with the bug number).  I think this has been 
our direction anyway, to

write tests for the functionality and not only for a specific issue.

Thanks, Roger



On 6/9/2016 10:18 AM, Roger Riggs wrote:

Hi Mark,

IIOPInputStream.java:

- Does adding doPriv overhead to every field access have a 
noticeable impact on performance?
  If most of the fields are accessible or can easily be checked as 
being in the base module,
  I could see trying the access directly and catching  the security 
exception to retry with doPriv.


Roger

ps.

There is a newer version of webrev that provides convenient next and 
previous links.


http://hg.openjdk.java.net/code-tools/webrev/raw-file/d26c194772db/webrev.ksh 





On 6/9/2016 8:57 AM, Seán Coffey wrote:

Hey Mark,

Looks fine. A few comments.


2291 Class declaredFieldClass = declaredClassField.getType();
2292
2293 if (declaredFieldClass == null) {
2294 continue;
2295 }

I'm not sure if this check is required. Can getType() return null ?

==


2833 Field classField;
2834 classField = c
2835 .getDeclaredField(fieldName);
2836 return classField;
Could we fold that code down to 'return 
c.getDeclaredField(fieldName);' ?


For the tests :
==
ConcurrentHashMapTest.java :
you might want to remove the old rmiServerProcess.destroy(), 
orbdProcess.destroyForcibly() comments.

==
HelloClient.java
line 157 : s/1000/ONE_SECOND

The use of static ports could be one to watch. If we encounter port 
issues, the test might have to be revisited.


Regards,
Sean.

On 08/06/2016 23:18, Mark Sheppard wrote:


Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev/

http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev/

which address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8146975

the type checking in inputClassFields and other places failed to 
fully allowing for
the processing of return ValueTypes, and hence the 
getDeclaredField fails as
"application code" exist  on the call stack restricting access. 
This leads to a security exception,
which in turn leads to an IllegalArgumentExcetption, the 
processing of which failed to allow

for a null object value in the stream.
This has now been rectified, with the getDeclaredField wrapped in 
a doPrivileged call.


regards
Mark

Review request: JDK-8068764 java/lang/ClassLoader/ExtDirs.java failed with Exception java.lang.IllegalThreadStateException: process hasn't exited

[Mandy Chung]

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8068764/webrev.00/

The ExtDirs and EndorsedDirs tests might fail due to the insufficient wait time 
on fastdebug build on a slow machine.

This patch converts the tests to use the test library ProcessTools that will 
wait for the child process to finish or destroy it when timeout.

Mandy

Re: [9] RFR: 8043387: java/time/test/java/util/TestFormatter.java failed.

[Roger Riggs]

Looks fine,

Roger


On 6/10/2016 5:52 PM, Naoto Sato wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8043387

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8043387/webrev.00/

The issue only is reproducible when the underlying OS's default time 
zone is "GMT+XX:XX". In that case, the test case removes "GMT" and 
retains it as the expected formatted time zone name, which is not 
correct.


Naoto

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

>> Please review the following changeset:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>> 
>> 
>> The issue calls for reconsidering the JarFile.Release enum.  A comment in 
>> the bug report suggests replacing JarFile.Release with Runtime.Version, and 
>> that’s what I did.  Specifically I removed the enum, changed the constructor 
>> to accept a Runtime.Version object instead of a JarFile.Release object, 
>> updated all places in the JDK that invoked the constructor and updated all 
>> tests.
>> 
> Moving to Runtime.Version seems right but doesn't the javadoc for the 
> constructor need to be updated to make it clear how it behavior when invoking 
> with something like Version.parse("7.1") ? If I read the code correctly then 
> this will be accepted and getVersion() will return 7.1.

Yes, it needs to be updated and it needs to be fixed.  Thanks for finding that.

> 
> Fields or methods is another discussion point for the base and runtime 
> versions.

My thinking is, in this case fields and methods are equivalent, the method not 
giving any more flexibility than a field.  For example the method 
JarFile.baseVersion will just return the value contained in the private final 
static field BASE_VERSION.  Or the public final static field BASE_VERSION can 
be directly accessed.  I see no advantage of a method here.  But I’m willing to 
be enlightened.

> 
> -Alan.

Re: [9] RFR: 8043387: java/time/test/java/util/TestFormatter.java failed.

[Xueming Shen]

+1

On 6/10/16 2:52 PM, Naoto Sato wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8043387

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8043387/webrev.00/

The issue only is reproducible when the underlying OS's default time 
zone is "GMT+XX:XX". In that case, the test case removes "GMT" and 
retains it as the expected formatted time zone name, which is not 
correct.


Naoto

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Alan Bateman]

On 15/06/2016 00:17, Steve Drach wrote:

Hi,

Please review the following changeset:

webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 

issue: https://bugs.openjdk.java.net/browse/JDK-8150680 


The issue calls for reconsidering the JarFile.Release enum.  A comment in the 
bug report suggests replacing JarFile.Release with Runtime.Version, and that’s 
what I did.  Specifically I removed the enum, changed the constructor to accept 
a Runtime.Version object instead of a JarFile.Release object, updated all 
places in the JDK that invoked the constructor and updated all tests.

Moving to Runtime.Version seems right but doesn't the javadoc for the 
constructor need to be updated to make it clear how it behavior when 
invoking with something like Version.parse("7.1") ? If I read the code 
correctly then this will be accepted and getVersion() will return 7.1.


Fields or methods is another discussion point for the base and runtime 
versions.


-Alan.

RFR: 8159590: Remove deprecated methods from jdk.internal.misc.VM

[Claes Redestad]

Hi,

after VM.java was encapsulated and moved from sun.misc to 
jdk.internal.misc, the rationale for keeping a number of deprecated 
methods and constants no longer applies and these methods should be removed:


Webrev: http://cr.openjdk.java.net/~redestad/8159590/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8159590

This falls somewhere between an enhancement and a bug fix, so feel free 
to object to or endorse treating this as a bug. :-)


/Claes

Re: Create java.util.stream.Stream from Iterator / Enumeration

[Patrick Reinhart]

Almost:

- you can use Enumeration.asIterator() rather than creating your own.


Right, for JDK 9 that will the right way. In the meantime under JDK 8
I will have to write my own ;-)


- I don’t think you can assume the Iterator has an encounter order
(even though there is a form of order related to class loader
hierarchy, i.e. you cannot assume resources from a particular class
loader are presented in any particular order, it might depend on how
the zip/jar was created or the order in which resources are presented
in the JDK image, which IIRC the order might optimized for booting
up).


So in that case only IMMUTABLE will be appropriate, possibly also 
NONNULL

as far I understand the getResources() method documentation.


I had marked ClassLoader as an area to use Stream (we went through a
bunch of areas that return Enumeration and add Stream-based methods
e.g. NetworkInterface) but we held off because Jigsaw was doing a lot
of plumbing work.


I did already some hacking on the Jigsaw stuff and I liked it quit a 
lot.

The biggest problem I see is the time it takes to have all required
libraries being converted to Jigsaw too. All in all you all did a great
job there and I hope this will be appreciated in the end...


It might be possible to revisit, it’s the type of enhancement we could
get a Feature Complete (FC) extension for. I cannot promise anything
here, but if you are looking for something to contribute that may be a
good area of focus on now Jigsaw is settling down.


So, what do you suggest that should do now? Should I open a enhancement
Issue for that?

Patrick