RFR JDK-8155616: java/util/zip/TestLocalTime.java fails intermittently with storing mtime failed

[Xueming Shen]

Hi Amy,

I finally got to the bottom of this failure. It appears the timestamps that
trigger the failure are those that (1) < 1980 and (2) within the 
daylight saving

time gap, which means the local date-time is actually not valid, for example
1968-04-28/2:51:25, this ldt should be adjusted to 3:51:25 when switched
to summer time. This is really a corner case that should not have any real
impact to real world use scenario. The implementation itself is correct. 
Just the

test case need adjusted. Please help review.

issue: https://bugs.openjdk.java.net/browse/JDK-8155616
webrev: http://cr.openjdk.java.net/~sherman/8155616/webrev

Thanks,
Sherman

RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Frank Yuan]

> -Original Message-
> From: Amy Lu [mailto:amy...@oracle.com]
> Sent: Monday, July 18, 2016 5:42 PM
> To: Frank Yuan; 'core-libs-dev'
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit 
> tests
> 
> On 7/18/16 5:32 PM, Frank Yuan wrote:
> > Btw, I moved internaltest into unittest because it's unnecessary to 
> > separate them.
> 
> Maybe you'd like to regenerate the webrev with hg move for those files?
> 
Yes, I will. Thank you very much!

Frank

> Thanks,
> Amy

Re: 8161129 Unsafe::getUnsafe should allow the platform class loader to access it

[John Rose]

+1 to Martin and Remi.  The call to registerMethodsToFilter is very old,
way out of date, and accomplishes nothing useful today.

Modular access checks for reflective method calls is our main line of defense,
if the security manager is turned off.  There are other things we might try in 
the
future that are analogous to registerMethodsToFilter, but it doesn't add 
anything
today.

— John

On Jul 18, 2016, at 7:07 PM, Martin Buchholz  wrote:
> 
> Recent discussion here:
> 
> http://markmail.org/search/?q=core-libs-dev%20ConcurrentHashMap%20jdk.internal.misc.unsafe#query:core-libs-dev%20ConcurrentHashMap%20jdk.internal.misc.unsafe+page:1+mid:iviymmlotcuasv6t+state:results
> 
> There are too many places where reflective code can get its hands on a
> jdk.internal.misc.Unsafe.  They can even call its toString method, but
> (perhaps!) not the dangerous methods.
> 
> On Mon, Jul 18, 2016 at 10:29 AM, Remi Forax  wrote:
> 
>> Hi Paul,
>> 
>> - Mail original -
>>> De: "Paul Sandoz" 
>>> À: "Core-Libs-Dev" 
>>> Envoyé: Lundi 18 Juillet 2016 18:06:44
>>> Objet: 8161129 Unsafe::getUnsafe should allow the platform class loader
>> toaccess it
>>> 
>>> Hi,
>>> 
>>> Please review the patch below.
>>> 
>> 
>> [...]
>> 
>>> I also took the opportunity to hide the “theInternalUnsafe” field in the
>>> sun.misc.Unsafe. That just avoids any futile attempts to obtain the
>> field’s
>>> value after which any reflective access on that value will fail.
>> 
>> I see 3 good reasons to not do that,
>> - as you said it is futile to try to get a reference to
>> jdk.internal.misc.Unsafe because you will never be able to call a method on
>> it.
>> - the code you add contains a string that reference a field name which is
>> erro prone when doing a refactoring
>> - jdk.internal.misc.Unsafe is stored in static fields of a lot of exported
>> classes, so why trying to 'protect' only sun.misc.Unsafe.
>> 
>>> 
>>> Paul.
>> 
>> Rémi
>> 
>> 
>>> diff -r 4f5f82c457af
>> src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
>>> --- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java  Mon Jul 18
>>> 13:13:52 2016 +0800
>>> +++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java  Mon Jul 18
>>> 17:50:20 2016 +0200
>>> @@ -56,6 +56,7 @@
>>> 
>>> static {
>>> Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
>>> +Reflection.registerFieldsToFilter(Unsafe.class,
>>> "theInternalUnsafe");
>>> }
>>> 
>>> private Unsafe() {}
>>> 
>> 

Re: Change of the toString implementation for annotations

[joe darcy]

Hello Rafael,

On 7/18/2016 5:43 PM, Rafael Winterhalter wrote:

Hello,
I recognized a failing test on Java 9 caused by a changed return value
returned by toString on an annotation with a class-property.

When calling toString on an annotation: @interface Foo { Class value();
} instantiated as @Foo(Bar.class) with Java 8 it would be printed as:

@Foo(class Bar)

while in Java 9 it is printed as:

@Foo(Bar.class)

Is this change intended? I do not see a big benefit of this implementation
change and it could break code. In my case, the problem is not that big, it
is an easy fix but still, I found it a bit surprising.



I pushed the change your test noticed; it was done as part of

JDK-5040830: (ann) please improve toString() for annotations 
containing exception proxies

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

The basic rationale for the change is that "Foo.class" is the syntax 
that appears when annotations are in source code so the toString() form 
should generally reflect that.


Thanks for running your project against JDK 9 builds; HTH,

-Joe

Re: 8161129 Unsafe::getUnsafe should allow the platform class loader to access it

[Martin Buchholz]

Recent discussion here:

http://markmail.org/search/?q=core-libs-dev%20ConcurrentHashMap%20jdk.internal.misc.unsafe#query:core-libs-dev%20ConcurrentHashMap%20jdk.internal.misc.unsafe+page:1+mid:iviymmlotcuasv6t+state:results

There are too many places where reflective code can get its hands on a
jdk.internal.misc.Unsafe.  They can even call its toString method, but
(perhaps!) not the dangerous methods.

On Mon, Jul 18, 2016 at 10:29 AM, Remi Forax  wrote:

> Hi Paul,
>
> - Mail original -
> > De: "Paul Sandoz" 
> > À: "Core-Libs-Dev" 
> > Envoyé: Lundi 18 Juillet 2016 18:06:44
> > Objet: 8161129 Unsafe::getUnsafe should allow the platform class loader
> toaccess it
> >
> > Hi,
> >
> > Please review the patch below.
> >
>
> [...]
>
> > I also took the opportunity to hide the “theInternalUnsafe” field in the
> > sun.misc.Unsafe. That just avoids any futile attempts to obtain the
> field’s
> > value after which any reflective access on that value will fail.
>
> I see 3 good reasons to not do that,
> - as you said it is futile to try to get a reference to
> jdk.internal.misc.Unsafe because you will never be able to call a method on
> it.
> - the code you add contains a string that reference a field name which is
> erro prone when doing a refactoring
> - jdk.internal.misc.Unsafe is stored in static fields of a lot of exported
> classes, so why trying to 'protect' only sun.misc.Unsafe.
>
> >
> > Paul.
>
> Rémi
>
>
> > diff -r 4f5f82c457af
> src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
> > --- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java  Mon Jul 18
> > 13:13:52 2016 +0800
> > +++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java  Mon Jul 18
> > 17:50:20 2016 +0200
> > @@ -56,6 +56,7 @@
> >
> >  static {
> >  Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
> > +Reflection.registerFieldsToFilter(Unsafe.class,
> > "theInternalUnsafe");
> >  }
> >
> >  private Unsafe() {}
> >
>

Re: JDK 9 RFR of 8161402: Provide a javadoc description for java.prefs module

[joe darcy]

Hi Brian,

As as grammatical comment, I might start with "The {@code java.prefs} 
module defines ...", but otherwise, the change looks fine.


Thanks,

-Joe


On 7/18/2016 5:45 PM, Brian Burkhalter wrote:

Please review at your convenience.

Issue:  https://bugs.openjdk.java.net/browse/JDK-8161402
Patch:  [1]

Thanks,

Brian

[1] diff

--- a/src/java.prefs/share/classes/module-info.java
+++ b/src/java.prefs/share/classes/module-info.java
@@ -23,6 +23,10 @@
   * questions.
   */
  
+/**

+ * java.prefs defines and exports the preferences APIs of the Java SE platform.
+ */
+
  module java.prefs {
  requires java.xml;

Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Steve Drach]

>> Please review the following:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
>> 
>> 
>> This changeset adds a multi-release jar validator to jar tool.  After the 
>> jar tool builds a multi-release jar, the potential resultant jar file is 
>> passed to the validator to assure that the jar file meets the minimal 
>> standards of a multi-release jar, in particular that versioned classes have 
>> the same api as base classes they override.  There are other checks, for 
>> example warning if two classes are identical.  If the jar file is invalid, 
>> it is not kept, so —create will not produce a jar file and —update will keep 
>> the input jar file.

I’ve updated the webrev to address the issues raised — 
http://cr.openjdk.java.net/~sdrach/8158295/webrev.02/index.html 


> 
> jar/Main.java
> 
> 284 // This code would be repeated in the create and update 
> sections.
> 285 // In order not to do that, it's here as a consumer.
> 286 Consumer validateAndClose = tmpfile -> {
> Why does it need to be Consumer rather than just a method?

I changed it to a method.

> 
> Then i think you don’t need to rethow, but you can still keep the try block 
> and use finally for File.deleteIfExist since it will not complain for the 
> case where you move.

Done.

> 
> 
> 558 int i1 = s1.indexOf('/', n);
> 559 int i2 = s1.indexOf('/', n);
> 560 if (i1 == -1) throw new NumberFormatException(s1);
> 561 if (i2 == -2) throw new NumberFormatException(s2);
> 
> I think you are trying to reject entries directly under the versioned area so 
> it’s not about numbers (same in the validator, so there is some redundancy?).

A couple things here.  The most obvious is it should be “if (i2 == -1)”.  I 
replaced NumberFormatException with a new InvalidJarException.  I added a 
comment that
this code is used to sort the version numbers as string representations of 
numbers, therefore “9” goes before “10”, not usual for string sorts.

> 
> 
> 588 AtomicBoolean validHolder = new AtomicBoolean();
> 589 validHolder.set(valid);
> 
> Pass valid to the constructor

Done.

> 
> 
> Validator/FingerPrint.java
> 
> It would be useful to have some comments on what is checked in terms of API 
> compatibility. e.g. describe what FingerPrint collects and how Validator uses 
> it.

Added some commentary to FingerPrint.

> 
> 
> FingerPrint.java
> 
> 205 private final Map fields = new HashMap<>();
> 206 private final Map methods = new HashMap<>();
> 
> Not sure you need to use a Map, why not use a Set?

Not sure why I did it with Maps instead of Sets, but I wanted to keep the 
method and field names and maybe that made sense at one time.  It doesn’t now, 
so Sets they are.

> 
> Change Method to be literally a representation of a single method rather than 
> a consolidation that is lossy for the set of exceptions.

Done.

JDK 9 RFR of 8161402: Provide a javadoc description for java.prefs module

[Brian Burkhalter]

Please review at your convenience.

Issue:  https://bugs.openjdk.java.net/browse/JDK-8161402
Patch:  [1]

Thanks,

Brian

[1] diff

--- a/src/java.prefs/share/classes/module-info.java
+++ b/src/java.prefs/share/classes/module-info.java
@@ -23,6 +23,10 @@
  * questions.
  */
 
+/**
+ * java.prefs defines and exports the preferences APIs of the Java SE platform.
+ */
+
 module java.prefs {
 requires java.xml;

Change of the toString implementation for annotations

[Rafael Winterhalter]

Hello,
I recognized a failing test on Java 9 caused by a changed return value
returned by toString on an annotation with a class-property.

When calling toString on an annotation: @interface Foo { Class value();
} instantiated as @Foo(Bar.class) with Java 8 it would be printed as:

@Foo(class Bar)

while in Java 9 it is printed as:

@Foo(Bar.class)

Is this change intended? I do not see a big benefit of this implementation
change and it could break code. In my case, the problem is not that big, it
is an easy fix but still, I found it a bit surprising.

Thank you.
Best regards, Rafael

Re: JDK 9 RFR of JDK-8161567: Mark java/util/concurrent/forkjoin/FJExceptionTableLeak.java as intermittently failing

[Martin Buchholz]

Okay, we're all agreed.  Amy can demote this test, and I'll try to think
hard about how to improve the test, with my newfound insight.

On Mon, Jul 18, 2016 at 5:00 PM, Joseph D. Darcy 
wrote:

> Hello,
>
> While the underlying issues are being investigated, I think it is
> worthwhile to tag the test as intermittently failing to aid those having to
> QA any if its failures.
>
> Cheers,
>
> -Joe
>
>
> On 7/18/2016 4:56 PM, David Holmes wrote:
>
>> Hi Martin,
>>
>> On 19/07/2016 12:38 AM, Martin Buchholz wrote:
>>
>>> Like I always say, I'm unhappy with the increased memory requirements.
>>> Java programs that used to run in 4m now need more than 8m. Hotspot
>>> should
>>> be fixed!
>>>
>>
>> I've updated the comments in
>>
>> https://bugs.openjdk.java.net/browse/JDK-8151158
>>
>> I agree further investigation is needed in this case.
>>
>> If this is dependent on the particular GC, maybe there's a way to require
>>> not having that gc.
>>>
>>
>> GC algorithm itself should not be causing OOME.
>>
>> David
>> -
>>
>> I see:
>>>
>>> ./java/lang/management/MemoryMXBean/LowMemoryTest2.sh:28:# @requires
>>> vm.gc=="null"
>>> ./java/lang/management/MemoryMXBean/PendingAllGC.sh:29:# @requires
>>> vm.gc=="null"
>>> ./java/lang/management/MemoryMXBean/MemoryManagementConcMarkSweepGC.sh:29:#
>>>
>>> @requires vm.gc=="ConcMarkSweep" | vm.gc=="null"
>>> ./java/lang/management/MemoryMXBean/MemoryTestAllGC.sh:29:# @requires
>>> vm.gc=="Parallel" | vm.gc=="null"
>>> ./java/lang/management/MemoryMXBean/MemoryManagementParallelGC.sh:29:#
>>> @requires vm.gc=="Parallel" | vm.gc=="null"
>>> ./java/lang/management/MemoryMXBean/MemoryManagementSerialGC.sh:29:#
>>> @requires vm.gc=="Serial" | vm.gc=="null"
>>>
>>> Maybe we can make this test pass in a similar way.
>>>
>>> Bug reports should be clearer about which VM flags actually cause the
>>> failure.
>>>
>>>
>>> On Sun, Jul 17, 2016 at 11:46 PM, Amy Lu  wrote:
>>>
>>> java/util/concurrent/forkjoin/FJExceptionTableLeak.java

 Test was ever fixed in JDK-8144990, but still failing intermittently as
 reported in JDK-8151158.

 This patch is to add back @key intermittent to the test. (and keep it in
 tier2 till issue resolved.)
 Please review.

 bug: https://bugs.openjdk.java.net/browse/JDK-8161567
 webrev: http://cr.openjdk.java.net/~amlu/8161567/webrev.00/

 Thanks,
 Amy

 --- old/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
 2016-07-18 14:37:25.0 +0800
 +++ new/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
 2016-07-18 14:37:25.0 +0800
 @@ -35,6 +35,7 @@
   * @test
   * @author Doug Lea
   * @bug 8004138
 + * @key intermittent
   * @summary Check if ForkJoinPool table leaks thrown exceptions.
   * @run main/othervm -Xmx8m
 -Djava.util.concurrent.ForkJoinPool.common.parallelism=4
 FJExceptionTableLeak
   */




>

Re: JDK 9 RFR of JDK-8161567: Mark java/util/concurrent/forkjoin/FJExceptionTableLeak.java as intermittently failing

[Joseph D. Darcy]

Hello,

While the underlying issues are being investigated, I think it is 
worthwhile to tag the test as intermittently failing to aid those having 
to QA any if its failures.


Cheers,

-Joe

On 7/18/2016 4:56 PM, David Holmes wrote:

Hi Martin,

On 19/07/2016 12:38 AM, Martin Buchholz wrote:

Like I always say, I'm unhappy with the increased memory requirements.
Java programs that used to run in 4m now need more than 8m. Hotspot 
should

be fixed!


I've updated the comments in

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

I agree further investigation is needed in this case.

If this is dependent on the particular GC, maybe there's a way to 
require

not having that gc.


GC algorithm itself should not be causing OOME.

David
-


I see:

./java/lang/management/MemoryMXBean/LowMemoryTest2.sh:28:# @requires
vm.gc=="null"
./java/lang/management/MemoryMXBean/PendingAllGC.sh:29:# @requires
vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementConcMarkSweepGC.sh:29:# 


@requires vm.gc=="ConcMarkSweep" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryTestAllGC.sh:29:# @requires
vm.gc=="Parallel" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementParallelGC.sh:29:#
@requires vm.gc=="Parallel" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementSerialGC.sh:29:#
@requires vm.gc=="Serial" | vm.gc=="null"

Maybe we can make this test pass in a similar way.

Bug reports should be clearer about which VM flags actually cause the
failure.


On Sun, Jul 17, 2016 at 11:46 PM, Amy Lu  wrote:


java/util/concurrent/forkjoin/FJExceptionTableLeak.java

Test was ever fixed in JDK-8144990, but still failing intermittently as
reported in JDK-8151158.

This patch is to add back @key intermittent to the test. (and keep 
it in

tier2 till issue resolved.)
Please review.

bug: https://bugs.openjdk.java.net/browse/JDK-8161567
webrev: http://cr.openjdk.java.net/~amlu/8161567/webrev.00/

Thanks,
Amy

--- old/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
2016-07-18 14:37:25.0 +0800
+++ new/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
2016-07-18 14:37:25.0 +0800
@@ -35,6 +35,7 @@
  * @test
  * @author Doug Lea
  * @bug 8004138
+ * @key intermittent
  * @summary Check if ForkJoinPool table leaks thrown exceptions.
  * @run main/othervm -Xmx8m
-Djava.util.concurrent.ForkJoinPool.common.parallelism=4
FJExceptionTableLeak
  */

Re: JDK 9 RFR of JDK-8161567: Mark java/util/concurrent/forkjoin/FJExceptionTableLeak.java as intermittently failing

[David Holmes]

Hi Martin,

On 19/07/2016 12:38 AM, Martin Buchholz wrote:

Like I always say, I'm unhappy with the increased memory requirements.
Java programs that used to run in 4m now need more than 8m.  Hotspot should
be fixed!


I've updated the comments in

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

I agree further investigation is needed in this case.


If this is dependent on the particular GC, maybe there's a way to require
not having that gc.


GC algorithm itself should not be causing OOME.

David
-


I see:

./java/lang/management/MemoryMXBean/LowMemoryTest2.sh:28:# @requires
vm.gc=="null"
./java/lang/management/MemoryMXBean/PendingAllGC.sh:29:# @requires
vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementConcMarkSweepGC.sh:29:#
@requires vm.gc=="ConcMarkSweep" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryTestAllGC.sh:29:# @requires
vm.gc=="Parallel" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementParallelGC.sh:29:#
@requires vm.gc=="Parallel" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementSerialGC.sh:29:#
@requires vm.gc=="Serial" | vm.gc=="null"

Maybe we can make this test pass in a similar way.

Bug reports should be clearer about which VM flags actually cause the
failure.


On Sun, Jul 17, 2016 at 11:46 PM, Amy Lu  wrote:


java/util/concurrent/forkjoin/FJExceptionTableLeak.java

Test was ever fixed in JDK-8144990, but still failing intermittently as
reported in JDK-8151158.

This patch is to add back @key intermittent to the test. (and keep it in
tier2 till issue resolved.)
Please review.

bug: https://bugs.openjdk.java.net/browse/JDK-8161567
webrev: http://cr.openjdk.java.net/~amlu/8161567/webrev.00/

Thanks,
Amy

--- old/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
2016-07-18 14:37:25.0 +0800
+++ new/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
2016-07-18 14:37:25.0 +0800
@@ -35,6 +35,7 @@
  * @test
  * @author Doug Lea
  * @bug 8004138
+ * @key intermittent
  * @summary Check if ForkJoinPool table leaks thrown exceptions.
  * @run main/othervm -Xmx8m
-Djava.util.concurrent.ForkJoinPool.common.parallelism=4
FJExceptionTableLeak
  */

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

>> It’s used in two places, moving it to jarPackages won’t work, I think.  I 
>> also put a check in to see if it’s a multi-release jar.
> I meant to move the method down to where jarPackages is located. I would 
> probably adjust the comment too but we can do that once your changes are in 
> jdk9/dev as we have several patches that will change this code. So I think 
> what you have is okay.

Okay.  Will do.

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Alan Bateman]

On 18/07/2016 19:06, Steve Drach wrote:



It’s used in two places, moving it to jarPackages won’t work, I think. 
 I also put a check in to see if it’s a multi-release jar.
I meant to move the method down to where jarPackages is located. I would 
probably adjust the comment too but we can do that once your changes are 
in jdk9/dev as we have several patches that will change this code. So I 
think what you have is okay.


-Alan

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Steve Drach]

>> Please review this change to JarFile that reverts JarFile::stream and 
>> JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
>> except line 504 in JarEntryIterator that now uses a different constructor to 
>> create a JarFileEntry.  I also added some implementation notes explaining 
>> how to create a versionedStream and a versionedEntries method.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8157524 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 
>> 

I put an updated webrev at 
http://cr.openjdk.java.net/~sdrach/8157524/webrev.01/index.html 



> For @implNote then you can put the example in {@code ...} and that will allow 
> to use angled brackets (no need for < etc).

Okay.  I doesn’t seem to allow nested @, so I just dropped the @Override 
annotations on the example

> 
> In ModulePath then it would be good to move versionedStream to jarPackages.

It’s used in two places, moving it to jarPackages won’t work, I think.  I also 
put a check in to see if it’s a multi-release jar.

> A comment on the method would be useful too although we have other changes 
> coming to this code so we can do it then.

I put a comment in

> 
> You should use an @apiNote, as this is presenting some interesting 
> information on how to use this method and not some particular characteristics 
> of the implementation.

Okay

> 
> I would be inclined to drop the note for Enumeration and add a @see tag 
> referencing the Stream returned method.

I actually had to use the versionedEntries method for jdeps.  Because of that 
and because I don’t see the harm with leaving it in, I left it in.

Re: RFR (JAXP): 8021787: javax.xml.datatype.XMLGregorianCalendar.getMonth() return is documented wrong

[huizhe wang]

Hi Svetlana,

Thanks for working on these bugs. The change is correct. It would be 
better to say: "Returns the month of this calendar", in both the returns 
tag and the description (not number of month).


While we are in this class, it would be good to at least fix the missing 
@returns tags in a number of methods, e.g. getDay, getTimezone, getHour, 
getMinute, toGregorianCalendar. Also, it @Override equals, hashCode, 
toString, and clone.


Thanks,

Joe

On 7/18/2016 8:49 AM, Svetlana Nikandrova wrote:

Hello all,

please review this javadoc fix for 
javax.xml.datatype.XMLGregorianCalendar.getMonth()

Webrev:
http://cr.openjdk.java.net/~snikandrova/8021787/webrev.00/ 


JBS:
https://bugs.openjdk.java.net/browse/JDK-8021787

Thank you,
Svetlana

Re: 8161129 Unsafe::getUnsafe should allow the platform class loader to access it

[Remi Forax]

Hi Paul,

- Mail original -
> De: "Paul Sandoz" 
> À: "Core-Libs-Dev" 
> Envoyé: Lundi 18 Juillet 2016 18:06:44
> Objet: 8161129 Unsafe::getUnsafe should allow the platform class loader to
> access it
> 
> Hi,
> 
> Please review the patch below.
> 

[...]

> I also took the opportunity to hide the “theInternalUnsafe” field in the
> sun.misc.Unsafe. That just avoids any futile attempts to obtain the field’s
> value after which any reflective access on that value will fail.

I see 3 good reasons to not do that,
- as you said it is futile to try to get a reference to 
jdk.internal.misc.Unsafe because you will never be able to call a method on it.
- the code you add contains a string that reference a field name which is erro 
prone when doing a refactoring
- jdk.internal.misc.Unsafe is stored in static fields of a lot of exported 
classes, so why trying to 'protect' only sun.misc.Unsafe. 

> 
> Paul.

Rémi


> diff -r 4f5f82c457af src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
> --- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java  Mon Jul 18
> 13:13:52 2016 +0800
> +++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java  Mon Jul 18
> 17:50:20 2016 +0200
> @@ -56,6 +56,7 @@
> 
>  static {
>  Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
> +Reflection.registerFieldsToFilter(Unsafe.class,
> "theInternalUnsafe");
>  }
> 
>  private Unsafe() {}
> 

Re: JDK 9 RFR of JDK-8161500: Use getTypeName and StringJoiner in core reflection toString methods

[joe darcy]

Hi Claes,


On 7/15/2016 4:55 PM, Claes Redestad wrote:

Looks OK, although I find:

StringJoiner sj = StringJoiner(delimiter, before, after);
...
sb.append(sj.toString());

to be a messier read than:

sb.append(before);
StringJoiner sj = new StringJoiner(delimiter);
...
sb.append(sj.toString())
  .append(after);


For the usagse in Executable.java where the "(" and ")" are always 
issued, I switched back to appending those characters directly to the 
string builder.




Any of these are ever used in a performance sensitive manner? I assume 
no, but still curious. :-)


I would be shocked if these were used in a performance sensitive context :-)

Thanks for the review,

-Joe



/Claes

On 2016-07-16 01:17, Joseph D. Darcy wrote:

Hello,

Please review this straightforward cleanup of some of the core
reflection implementation classes:

 JDK-8161500: Use getTypeName and StringJoiner in core reflection
toString methods
 http://cr.openjdk.java.net/~darcy/8161500.1/

All java/lang/Class and java/lang/reflect regression tests pass with
this patch.

(If the fix for JDK-8054213: "Class name repeated in output of
Type.toString()" is pushed before this fix, a simple merge will be 
needed.)


Thanks,

-Joe

Re: 8161129 Unsafe::getUnsafe should allow the platform class loader to access it

[Alan Bateman]

On 18/07/2016 17:33, Peter Levart wrote:


Hi Paul,

Just curious, what was the purpose of hiding a public method in a 
public class in an exported package from reflection? I'm talking about 
sun.misc.Unsafe::getUnsafe ...


Regards, Peter
As I recall, it was an effort in JDK 6 (and prior to OpenJDK) to make it 
less tempting to use directly. It didn't work :-)


-Alan

Re: 8161129 Unsafe::getUnsafe should allow the platform class loader to access it

[Peter Levart]

Hi Paul,

Just curious, what was the purpose of hiding a public method in a public 
class in an exported package from reflection? I'm talking about 
sun.misc.Unsafe::getUnsafe ...


Regards, Peter

On 07/18/2016 06:06 PM, Paul Sandoz wrote:

Hi,

Please review the patch below.

Since Jigsaw encapsulates jdk.internal.misc.Unsafe there is no need to perform 
any runtime checks such as:

- hiding the getUnsafe method from reflection; and
- checking the class loader of the calling class for invocation of getUnsafe.

This more cleanly enables qualified modules to utilise the internal Unsafe 
without reflection gymnastics, even though we should be reviewing that usage 
and to reduce it where possible.

I also took the opportunity to hide the “theInternalUnsafe” field in the 
sun.misc.Unsafe. That just avoids any futile attempts to obtain the field’s 
value after which any reflective access on that value will fail.

Paul.

diff -r 4f5f82c457af src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java Mon Jul 18 
13:13:52 2016 +0800
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java Mon Jul 18 
17:50:20 2016 +0200
@@ -26,8 +26,6 @@
  package jdk.internal.misc;

  import jdk.internal.HotSpotIntrinsicCandidate;
-import jdk.internal.reflect.CallerSensitive;
-import jdk.internal.reflect.Reflection;
  import jdk.internal.vm.annotation.ForceInline;

  import java.lang.reflect.Field;
@@ -57,7 +55,6 @@
  private static native void registerNatives();
  static {
  registerNatives();
-Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
  }

  private Unsafe() {}
@@ -87,16 +84,8 @@
   * }}
   *
   * (It may assist compilers to make the local variable {@code final}.)
- *
- * @throws  SecurityException if the class loader of the caller
- *  class is not in the system domain in which all permissions
- *  are granted.
   */
-@CallerSensitive
  public static Unsafe getUnsafe() {
-Class caller = Reflection.getCallerClass();
-if (!VM.isSystemDomainLoader(caller.getClassLoader()))
-throw new SecurityException("Unsafe");
  return theUnsafe;
  }

diff -r 4f5f82c457af src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
--- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.javaMon Jul 18 
13:13:52 2016 +0800
+++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.javaMon Jul 18 
17:50:20 2016 +0200
@@ -56,6 +56,7 @@

  static {
  Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
+Reflection.registerFieldsToFilter(Unsafe.class, "theInternalUnsafe");
  }

  private Unsafe() {}

Re: [9] RFR: 8159214: jlink --include-locales problems

[Jim Laskey (Oracle)]

+1

> On Jul 18, 2016, at 12:54 PM, Naoto Sato  wrote:
> 
> Ping.
> 
> On 7/14/16 5:42 AM, Naoto Sato wrote:
>> Hello,
>> 
>> Please review the fix to the following issue:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8159214
>> 
>> The fix is located at:
>> 
>> http://cr.openjdk.java.net/~naoto/8159214/webrev.04/
>> 
>> Naoto

8161129 Unsafe::getUnsafe should allow the platform class loader to access it

[Paul Sandoz]

Hi,

Please review the patch below.

Since Jigsaw encapsulates jdk.internal.misc.Unsafe there is no need to perform 
any runtime checks such as:

- hiding the getUnsafe method from reflection; and
- checking the class loader of the calling class for invocation of getUnsafe.

This more cleanly enables qualified modules to utilise the internal Unsafe 
without reflection gymnastics, even though we should be reviewing that usage 
and to reduce it where possible.

I also took the opportunity to hide the “theInternalUnsafe” field in the 
sun.misc.Unsafe. That just avoids any futile attempts to obtain the field’s 
value after which any reflective access on that value will fail.

Paul.

diff -r 4f5f82c457af src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java Mon Jul 18 
13:13:52 2016 +0800
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java Mon Jul 18 
17:50:20 2016 +0200
@@ -26,8 +26,6 @@
 package jdk.internal.misc;

 import jdk.internal.HotSpotIntrinsicCandidate;
-import jdk.internal.reflect.CallerSensitive;
-import jdk.internal.reflect.Reflection;
 import jdk.internal.vm.annotation.ForceInline;

 import java.lang.reflect.Field;
@@ -57,7 +55,6 @@
 private static native void registerNatives();
 static {
 registerNatives();
-Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
 }

 private Unsafe() {}
@@ -87,16 +84,8 @@
  * }}
  *
  * (It may assist compilers to make the local variable {@code final}.)
- *
- * @throws  SecurityException if the class loader of the caller
- *  class is not in the system domain in which all permissions
- *  are granted.
  */
-@CallerSensitive
 public static Unsafe getUnsafe() {
-Class caller = Reflection.getCallerClass();
-if (!VM.isSystemDomainLoader(caller.getClassLoader()))
-throw new SecurityException("Unsafe");
 return theUnsafe;
 }

diff -r 4f5f82c457af src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
--- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.javaMon Jul 18 
13:13:52 2016 +0800
+++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.javaMon Jul 18 
17:50:20 2016 +0200
@@ -56,6 +56,7 @@

 static {
 Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");
+Reflection.registerFieldsToFilter(Unsafe.class, "theInternalUnsafe");
 }

 private Unsafe() {}

Re: [9] RFR: 8159214: jlink --include-locales problems

[Naoto Sato]

Ping.

On 7/14/16 5:42 AM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

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

The fix is located at:

http://cr.openjdk.java.net/~naoto/8159214/webrev.04/

Naoto

RFR (JAXP): 8021787: javax.xml.datatype.XMLGregorianCalendar.getMonth() return is documented wrong

[Svetlana Nikandrova]

Hello all,

please review this javadoc fix for 
javax.xml.datatype.XMLGregorianCalendar.getMonth()

Webrev:
http://cr.openjdk.java.net/~snikandrova/8021787/webrev.00/ 


JBS:
https://bugs.openjdk.java.net/browse/JDK-8021787

Thank you,
Svetlana

Re: RFR(XXXXXXXS): 8161034 Incorrect GPL header in jdk/src/java.base/windows/native/libjava/jni_util_md.c

[frederic parain]

Thanks for the review Alan!

Fred

On 18/07/2016 10:39, Alan Bateman wrote:

On 18/07/2016 14:48, frederic parain wrote:


Hello,

src/java.base/windows/native/libjava/jni_util_md.c file contains
incorrect GPL header which fails make/scripts/lic_check.sh script check.

Failure is caused by missing comma after modification years. Please,
help to review its addition:
http://cr.openjdk.java.net/~fparain/8161034/webrev.00/

lic_check.sh passes after this modification:
### Checking copyright notice in the file:
jdk/src/java.base/windows/native/libjava/jni_util_md.c
###
SUCCESS: The license header for
/export/fparain/WORK/JDK9/8161034/hs/jdk/src/java.base/windows/native/libjava/jni_util_md.c
has been verified.
###

Looks fine.

Re: RFR(XXXXXXXS): 8161034 Incorrect GPL header in jdk/src/java.base/windows/native/libjava/jni_util_md.c

[Alan Bateman]

On 18/07/2016 14:48, frederic parain wrote:


Hello,

src/java.base/windows/native/libjava/jni_util_md.c file contains 
incorrect GPL header which fails make/scripts/lic_check.sh script check.


Failure is caused by missing comma after modification years. Please, 
help to review its addition:

http://cr.openjdk.java.net/~fparain/8161034/webrev.00/

lic_check.sh passes after this modification:
### Checking copyright notice in the file: 
jdk/src/java.base/windows/native/libjava/jni_util_md.c

###
SUCCESS: The license header for 
/export/fparain/WORK/JDK9/8161034/hs/jdk/src/java.base/windows/native/libjava/jni_util_md.c 
has been verified.

###

Looks fine.

Re: JDK 9 RFR of JDK-8161567: Mark java/util/concurrent/forkjoin/FJExceptionTableLeak.java as intermittently failing

[Martin Buchholz]

Like I always say, I'm unhappy with the increased memory requirements.
Java programs that used to run in 4m now need more than 8m.  Hotspot should
be fixed!

If this is dependent on the particular GC, maybe there's a way to require
not having that gc.

I see:

./java/lang/management/MemoryMXBean/LowMemoryTest2.sh:28:# @requires
vm.gc=="null"
./java/lang/management/MemoryMXBean/PendingAllGC.sh:29:# @requires
vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementConcMarkSweepGC.sh:29:#
@requires vm.gc=="ConcMarkSweep" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryTestAllGC.sh:29:# @requires
vm.gc=="Parallel" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementParallelGC.sh:29:#
@requires vm.gc=="Parallel" | vm.gc=="null"
./java/lang/management/MemoryMXBean/MemoryManagementSerialGC.sh:29:#
@requires vm.gc=="Serial" | vm.gc=="null"

Maybe we can make this test pass in a similar way.

Bug reports should be clearer about which VM flags actually cause the
failure.


On Sun, Jul 17, 2016 at 11:46 PM, Amy Lu  wrote:

> java/util/concurrent/forkjoin/FJExceptionTableLeak.java
>
> Test was ever fixed in JDK-8144990, but still failing intermittently as
> reported in JDK-8151158.
>
> This patch is to add back @key intermittent to the test. (and keep it in
> tier2 till issue resolved.)
> Please review.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8161567
> webrev: http://cr.openjdk.java.net/~amlu/8161567/webrev.00/
>
> Thanks,
> Amy
>
> --- old/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
> 2016-07-18 14:37:25.0 +0800
> +++ new/test/java/util/concurrent/forkjoin/FJExceptionTableLeak.java
> 2016-07-18 14:37:25.0 +0800
> @@ -35,6 +35,7 @@
>   * @test
>   * @author Doug Lea
>   * @bug 8004138
> + * @key intermittent
>   * @summary Check if ForkJoinPool table leaks thrown exceptions.
>   * @run main/othervm -Xmx8m
> -Djava.util.concurrent.ForkJoinPool.common.parallelism=4
> FJExceptionTableLeak
>   */
>
>
>

RFR(XXXXXXXS): 8161034 Incorrect GPL header in jdk/src/java.base/windows/native/libjava/jni_util_md.c

[frederic parain]

Hello,

src/java.base/windows/native/libjava/jni_util_md.c file contains 
incorrect GPL header which fails make/scripts/lic_check.sh script check.


Failure is caused by missing comma after modification years. Please, 
help to review its addition:

http://cr.openjdk.java.net/~fparain/8161034/webrev.00/

lic_check.sh passes after this modification:
### Checking copyright notice in the file: 
jdk/src/java.base/windows/native/libjava/jni_util_md.c

###
SUCCESS: The license header for 
/export/fparain/WORK/JDK9/8161034/hs/jdk/src/java.base/windows/native/libjava/jni_util_md.c 
has been verified.

###

Thank you,

Fred

Re: RFR(S): 8161212: Test times out: java/lang/invoke/LoopCombinatorLongSignatureTest.java

[Michael Haupt]

Hi Claes,

thank you. Of course you're right about the -run option; I've replaced this 
with a -D interface and pushed that version.

Best,

Michael

> Am 18.07.2016 um 12:57 schrieb Claes Redestad :
> 
> Hi,
> 
> On 2016-07-13 15:12, Michael Haupt wrote:
>> Dear all,
>> 
>> please review this fix.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161212
>> Webrev: http://cr.openjdk.java.net/~mhaupt/8161212/webrev.00/
> 
> fix looks OK, but I'm genuinely curious about best practice when it comes to 
> pass arguments to jtreg tests. How do I even pass -run to the actual test?
> 
> Naively System properties seems easier and possibly less fragile (assuming 
> tests aren't run with a very strict security policy somewhere so that we'd 
> have to pair the test with the associated permissions):
> 
> jtreg ... -Djava.lang.invoke.LoopCombinatorLongSignatureTest.run 
> java/lang/invoke/LoopCombinatorLongSignatureTest.java
> 
> boolean run = 
> System.getProperty("java.lang.invoke.LoopCombinatorLongSignatureTest.run") != 
> null;
> 
> Thanks!
> 
> /Claes
> 
>> 
>> Running the test in LambdaForm interpretation mode takes a long time. As the 
>> test is actually about capturing excessively long loop clause lists, running 
>> the constructed loop should be optional. The fix disables running the loop 
>> by default and introduces a -run option that can be used if the loop should 
>> actually be run.
>> 
>> Thanks,
>> 
>> Michael
>> 
> 

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: RFR(S): 8161212: Test times out: java/lang/invoke/LoopCombinatorLongSignatureTest.java

[Claes Redestad]

Hi,

On 2016-07-13 15:12, Michael Haupt wrote:

Dear all,

please review this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8161212
Webrev: http://cr.openjdk.java.net/~mhaupt/8161212/webrev.00/


fix looks OK, but I'm genuinely curious about best practice when it 
comes to pass arguments to jtreg tests. How do I even pass -run to the 
actual test?


Naively System properties seems easier and possibly less fragile 
(assuming tests aren't run with a very strict security policy somewhere 
so that we'd have to pair the test with the associated permissions):


jtreg ... -Djava.lang.invoke.LoopCombinatorLongSignatureTest.run 
java/lang/invoke/LoopCombinatorLongSignatureTest.java


boolean run = 
System.getProperty("java.lang.invoke.LoopCombinatorLongSignatureTest.run") 
!= null;


Thanks!

/Claes



Running the test in LambdaForm interpretation mode takes a long time. As the 
test is actually about capturing excessively long loop clause lists, running 
the constructed loop should be optional. The fix disables running the loop by 
default and introduces a -run option that can be used if the loop should 
actually be run.

Thanks,

Michael

Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Amy Lu]

On 7/18/16 5:32 PM, Frank Yuan wrote:

Btw, I moved internaltest into unittest because it's unnecessary to separate 
them.


Maybe you'd like to regenerate the webrev with hg move for those files?

Thanks,
Amy

RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Frank Yuan]

Hi

Would you like to review http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/?
Bug: https://bugs.openjdk.java.net/browse/JDK-8067170


In this change, I enabled security manager for JAXP unit tests with improving 
the implementation approach and fixing some defects.

Now jaxp tests use TestNG annotation to enable security manager and apply 
policies combination, like the following example(this full
example is at: 
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/Bug8003147Test.java.html):
@Listeners({ jaxp.library.FilePolicy.class, 
jaxp.library.InternalAPIPolicy.class })
public class Bug8003147Test {

There are also 2 additional patterns for some special cases:
1.  JAXPTestUtilities.runWithTmpPermission(Runnable, Permission.) is used 
for some cases which require their own special
permissions, e.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/transform/CR6551600Test.java.sdiff.html.
2.  JAXPTestUtilities.tryRunWithPolicyManager(Runnable, Permission.) is 
used for cases where some test methods need to be run
with security manager and some others need to be run without security manager 
because they have different behaviors when having sm
or not. E.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/FactoryFindTest.java.sdiff.html.
 Such cases
must run in single thread, all other cases don't require it, are thread safe.

Btw, I moved internaltest into unittest because it's unnecessary to separate 
them.


Thanks,

Frank

Re: RFR: 8157524 Revert JarFile methods "entries" and "stream" to Java 8 behavior

[Paul Sandoz]

> On 15 Jul 2016, at 23:17, Steve Drach  wrote:
> 
> Hi,
> 
> Please review this change to JarFile that reverts JarFile::stream and 
> JarFile::entries to JDK 8 behavior.  The code is identical to that in JDK 8 
> except line 504 in JarEntryIterator that now uses a different constructor to 
> create a JarFileEntry.  I also added some implementation notes explaining how 
> to create a versionedStream and a versionedEntries method.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8157524 
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8157524/webrev/index.html 
> 
> 

You should use an @apiNote, as this is presenting some interesting information 
on how to use this method and not some particular characteristics of the 
implementation.

I would be inclined to drop the note for Enumeration and add a @see tag 
referencing the Stream returned method.

Paul.