Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[David Holmes]

Hi Leela,

On 29/07/2016 12:59 PM, Leela Mohan wrote:

I think, change in the file unsafe.cpp is incorrect. (
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )

Below function is accessing naked oops when thread has transitioned to
"native":

*+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
(java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
{**+ return NULL;**+   }**+   Klass* k =
java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
loader = k->class_loader();**+   return JNIHandles::make_local(env,
loader);**+ }*


klass types are no longer oops in the Java heap, but metaspace objects 
that reside in a per-classloader allocation region. They never get 
compacted or relocated so raw pointers to them are safe.


Cheers,
David



- Leela

On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
coleen.phillim...@oracle.com> wrote:



Thanks, Mandy!

Coleen


On 9/8/14, 6:59 PM, Mandy Chung wrote:


Thumbs up.

Mandy

On 9/5/2014 12:55 PM, Coleen Phillimore wrote:


Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.  The hotspot change can
be tested separately from the jdk change (but not the other way around).

Thanks,
Coleen

Re: RFR: 8162439: Runtime.Version.parse needs fast-path for major versions

[Claes Redestad]

Hi Paul,

On 07/28/2016 02:55 PM, Paul Sandoz wrote:

On 27 Jul 2016, at 19:36, Claes Redestad  wrote:

On 07/25/2016 08:01 PM, Iris Clark wrote:

Hi, Claes.


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

I think that this change looks good.  We provide a shortcut for the common case 
where only the major version number is of interest without introducing a new 
API.

Thanks! Any reviewer want to give this the go-ahead?

Looks ok.

I suppose you could do:

   boolean isSimpleNumber(String s) {
 for (int i = 0; i < s.length(); i++) {
   char c = s.get(i);
   if (c < ((i > 0) ? ‘0’ : ‘1’) || c > ‘9’)
   return false;
 }
 return true;
   }


   if (isSimpleNumber(s)) {
 ...
   } else {
 return VersionBuilder.parse(s);
   }

then let VersionBuilder.parse throw errors in incorrectly formatted version 
strings.


sounds reasonable. It'd still change the behavior for the empty string 
from IAE to NFE, but only having to do one pass over the input string is 
nice.


Another reasonable comment I got offline was that this patch splits the 
parsing into two places, which is hacky. Since what we really want to 
avoid is to eagerly load the Version string patterns (pulling in large 
parts of j.u.regex), we could inline the parsing code into 
Runtime.Version.parse, rename VersionBuilder to VersionPattern and 
access the constants therein from the parse method to allow lazy 
initialization. The overall result is arguably cleaner:


http://cr.openjdk.java.net/~redestad/8162439/webrev.02/


Thanks!

/Claes

Re: Files.read/readAllBytes can loop once with zero size buffer

[Martin Buchholz]

What do you suggest instead?

The extra allocation of a zero-size buffer is not that bad.

I think it's best to optimize for files with correct metadata, while
tolerating ones with buggy metadata.

On Thu, Jul 28, 2016 at 1:13 PM, Fabian Lange 
wrote:

> Hi,
> I just noticed that when one uses Files.readAllBytes() to read for
> example from the Linux proc file system, where file size is claimed to
> be 0, Files.read() will allocate a buffer of size 0 before then
> adjusting in a second loop (arraycopying the zero size buffer)
>
> In my opinion an initial size of 0 should not be used.
> I admit this might be a regression for files which are really empty
> and return an empty byte array, but I think it is actually more common
> to read from files which incorrectly report to be zero sized.
>
> Fabian
>

Re: JDK 9 RFR of JDK-8162746: VersionCheck.java failure after change for JDK-8160921

[Tim Bell]

Hi Joe:

When the binary p2launcher was renamed to jweblauncher (JDK-8160921), 
the test


tools/launcher/VersionCheck.java

did not get the corresponding update and started failing.

Please review the patch below to address this.


Looks good to me.

Thanks-

Tim


Thanks,

-Joe

--- a/test/tools/launcher/VersionCheck.javaThu Jul 28 12:09:07 
2016 -0700
+++ b/test/tools/launcher/VersionCheck.javaThu Jul 28 15:11:02 
2016 -0700

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2007, 2016, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,7 @@

 /**
  * @test
- * @bug 6545058 6611182 8016209 8139986
+ * @bug 6545058 6611182 8016209 8139986 8162746
  * @summary validate and test -version, -fullversion, and internal, 
as well as

  *  sanity checks if a tool can be launched.
  * @compile VersionCheck.java
@@ -56,7 +56,7 @@
 "jcontrol",
 "jmc",
 "jmc.ini",
-"jp2launcher",
+"jweblauncher",
 "jvisualvm",
 "packager",
 "ssvagent",
@@ -93,11 +93,11 @@
 "jps",
 "jrunscript",
 "jjs",
-"jp2launcher",
 "jsadebugd",
 "jstack",
 "jstat",
 "jstatd",
+"jweblauncher",
 "jvisualvm",
 "keytool",
 "kinit",

JDK 9 RFR of JDK-8162746: VersionCheck.java failure after change for JDK-8160921

[Joseph D. Darcy]

Hello,

When the binary p2launcher was renamed to jweblauncher (JDK-8160921), 
the test


tools/launcher/VersionCheck.java

did not get the corresponding update and started failing.

Please review the patch below to address this.

Thanks,

-Joe

--- a/test/tools/launcher/VersionCheck.javaThu Jul 28 12:09:07 2016 
-0700
+++ b/test/tools/launcher/VersionCheck.javaThu Jul 28 15:11:02 2016 
-0700

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2007, 2016, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,7 @@

 /**
  * @test
- * @bug 6545058 6611182 8016209 8139986
+ * @bug 6545058 6611182 8016209 8139986 8162746
  * @summary validate and test -version, -fullversion, and internal, as 
well as

  *  sanity checks if a tool can be launched.
  * @compile VersionCheck.java
@@ -56,7 +56,7 @@
 "jcontrol",
 "jmc",
 "jmc.ini",
-"jp2launcher",
+"jweblauncher",
 "jvisualvm",
 "packager",
 "ssvagent",
@@ -93,11 +93,11 @@
 "jps",
 "jrunscript",
 "jjs",
-"jp2launcher",
 "jsadebugd",
 "jstack",
 "jstat",
 "jstatd",
+"jweblauncher",
 "jvisualvm",
 "keytool",
 "kinit",

Re: RFR: 6543126: Level.known can leak memory

[Daniel Fuchs]

On 28/07/16 21:31, James Perkins wrote:

I just happened to take a glance at this and noticed the remove method
on the KnownLevels doesn't quite look right.
Using Optional.of(nameToLevels.get(mirroredLevel.name)).ifPresent((x) ->
x.remove(this)); will produce an NPE if the level is not present. If the
intention is that the levels may not be present in the list the
Optional.ofNullable() should be used.



Thanks a lot for your keen eyes James!

(sigh!) Why would Optional.of()  throw a NPE for null?
But it does...

This wasn't caught by the test because remove() usually
follows a purge() and every call to purge() is synchronized,
so I don't think it could happen, technically - except possibly
for the call purge(KnownLevel). But you're right and the code
is wrong, of course.

ofNullable() it is!

best regards,

-- daniel

Re: RFR: 6543126: Level.known can leak memory

[James Perkins]

I just happened to take a glance at this and noticed the remove method on
the KnownLevels doesn't quite look right.
Using Optional.of(nameToLevels.get(mirroredLevel.name)).ifPresent((x) ->
x.remove(this)); will produce an NPE if the level is not present. If the
intention is that the levels may not be present in the list the
Optional.ofNullable() should be used.

On Mon, Jul 25, 2016 at 11:10 AM, Daniel Fuchs 
wrote:

> Hi,
>
> Please find below a fix for:
>
> 6543126: Level.known can leak memory
> https://bugs.openjdk.java.net/browse/JDK-6543126
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.00
>
> This is a fix for a long standing issue: Level maintains a list
> of know levels which has strong references to any created levels.
> Because the references are strong references, they prevent the garbage
> collection of levels, level classes, and level class loaders.
>
> This fix changes the KnownLevel class to extend WeakReference
> in order to only retain a weak reference to custom level instances.
>
> best regards,
>
> -- daniel
>



-- 
James R. Perkins
JBoss by Red Hat

Files.read/readAllBytes can loop once with zero size buffer

[Fabian Lange]

Hi,
I just noticed that when one uses Files.readAllBytes() to read for
example from the Linux proc file system, where file size is claimed to
be 0, Files.read() will allocate a buffer of size 0 before then
adjusting in a second loop (arraycopying the zero size buffer)

In my opinion an initial size of 0 should not be used.
I admit this might be a regression for files which are really empty
and return an empty byte array, but I think it is actually more common
to read from files which incorrectly report to be zero sized.

Fabian

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

[Steve Drach]

I’ve updated the webrev to incorporate the changes suggested by Oleg Barbashov. 
 This changeset has a couple of minor changes to the tests and I’ve changed the 
Validator class to be an instance of Consumer.

http://cr.openjdk.java.net/~sdrach/8158295/webrev.03/index.html 



> On Jul 18, 2016, at 6:33 PM, Steve Drach  wrote:
> 
>>> 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.
> 
> 

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

[Steve Drach]

> On Jul 28, 2016, at 11:59 AM, Oleg G. Barbashov  
> wrote:
> 
> 27.07.2016 15:34, Oleg G. Barbashov пишет:
>> 
>> 07.07.2016 23:32, Steve Drach пишет:
>>> Hi,
>>> 
>>> 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.
>>> 
>>> Thanks
>>> Steve
>> 
>> 574 private boolean validate(String fname) {
>> 575 boolean valid = true;
>> 576 Validator validator = new Validator(this);
>> 577
>> 578 try (JarFile jf = new JarFile(fname)) {
>> 579 AtomicBoolean validHolder = new AtomicBoolean(valid);
>> 580 jf.stream()
>> 581 .filter(e -> !e.isDirectory())
>> 582 .filter(e -> !e.getName().equals(MANIFEST_NAME))
>> 583 .filter(e -> !e.getName().endsWith(MODULE_INFO))
>> 584 .sorted(entryComparator)
>> 585 .forEachOrdered(je -> {
>> 586 boolean b = validator.accept(je, jf);
>> 587 if (validHolder.get()) validHolder.set(b);
>> 588 });
>> 589 valid = validHolder.get();
>> 590 } catch (IOException e) {
>> 591 error(formatMsg2("error.validator.jarfile.exception", fname, 
>> e.getMessage()));
>> 592 valid = false;
>> 593 } catch (InvalidJarException e) {
>> 594 error(formatMsg("error.validator.bad.entry.name", 
>> e.getMessage()));
>> 595 valid = false;
>> 596 }
>> 597 return valid;
>> 598 }
>> 
>> (IMHO) Using of AtomicBoolean and forEachOrdered() for stateful validator 
>> here looks forced. It may be avoided by using regular iterator with "for" 
>> loop:
>> 
>>Stream sorted = jf.stream()
>>.filter(e -> !e.isDirectory())
>>.filter(e -> !e.getName().equals(MANIFEST_NAME))
>>.filter(e -> !e.getName().endsWith(MODULE_INFO))
>>.sorted(entryComparator);
>>for (Iterator iter = sorted.iterator(); 
>> iter.hasNext();) {
>>if (!validator.accept(iter.next(), jf)) {
>>valid = false;
>>}
>>}
>> 
>> or even better by storing "valid" state inside the Validator after turning 
>> it to regular Consumer:
>> 
>>try (JarFile jf = new JarFile(fname)) {
>>Validator validator = new Validator(this, jf);
>>jf.stream()
>>.filter(e -> !e.isDirectory())
>>.filter(e -> !e.getName().equals(MANIFEST_NAME))
>>.filter(e -> !e.getName().endsWith(MODULE_INFO))
>>.sorted(entryComparator)
>>.forEachOrdered(validator);
>>return validator.isValidated();
>>} catch (IOException e) {
>>error(formatMsg2("error.validator.jarfile.exception", fname, 
>> e.getMessage()));
>>return false;
>>} catch (NumberFormatException e) {
>>error(formatMsg("error.validator.bad.entry.name", 
>> e.getMessage()));
>>return false;
>>}
>> 
>> Moreover what is the reason to have an external loop over the jar's entries? 
>> Even if we will use several different filter sets in future it would be 
>> better to use it as optional parameter for Validator.
>> 
> I mean that the jar file entries iteration code ("580 
> jf.stream().filter(e ")...588 }); ) 
> processing should be a part of "Validator”.

Yes, I know what you meant.  I think it’s fine where it is, but I do like your 
suggestion to make Validator a Consumer and I’ll be releasing an updated webrev 
soon, with that change and the changes you recommended for the tests.  Thanks 
for the ideas!

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

[Oleg G. Barbashov]

27.07.2016 15:34, Oleg G. Barbashov пишет:


07.07.2016 23:32, Steve Drach пишет:

Hi,

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.


Thanks
Steve


574 private boolean validate(String fname) {
 575 boolean valid = true;
 576 Validator validator = new Validator(this);
 577
 578 try (JarFile jf = new JarFile(fname)) {
 579 AtomicBoolean validHolder = new AtomicBoolean(valid);
 580 jf.stream()
 581 .filter(e -> !e.isDirectory())
 582 .filter(e -> !e.getName().equals(MANIFEST_NAME))
 583 .filter(e -> !e.getName().endsWith(MODULE_INFO))
 584 .sorted(entryComparator)
 585 .forEachOrdered(je -> {
 586 boolean b = validator.accept(je, jf);
 587 if (validHolder.get()) validHolder.set(b);
 588 });
 589 valid = validHolder.get();
 590 } catch (IOException e) {
 591 error(formatMsg2("error.validator.jarfile.exception", fname, 
e.getMessage()));

 592 valid = false;
 593 } catch (InvalidJarException e) {
 594 error(formatMsg("error.validator.bad.entry.name", 
e.getMessage()));

 595 valid = false;
 596 }
 597 return valid;
 598 }

(IMHO) Using of AtomicBoolean and forEachOrdered() for stateful 
validator here looks forced. It may be avoided by using regular 
iterator with "for" loop:


Stream sorted = jf.stream()
.filter(e -> !e.isDirectory())
.filter(e -> !e.getName().equals(MANIFEST_NAME))
.filter(e -> !e.getName().endsWith(MODULE_INFO))
.sorted(entryComparator);
for (Iterator iter = sorted.iterator(); 
iter.hasNext();) {

if (!validator.accept(iter.next(), jf)) {
valid = false;
}
}

or even better by storing "valid" state inside the Validator after 
turning it to regular Consumer:


try (JarFile jf = new JarFile(fname)) {
Validator validator = new Validator(this, jf);
jf.stream()
.filter(e -> !e.isDirectory())
.filter(e -> !e.getName().equals(MANIFEST_NAME))
.filter(e -> !e.getName().endsWith(MODULE_INFO))
.sorted(entryComparator)
.forEachOrdered(validator);
return validator.isValidated();
} catch (IOException e) {
error(formatMsg2("error.validator.jarfile.exception", 
fname, e.getMessage()));

return false;
} catch (NumberFormatException e) {
error(formatMsg("error.validator.bad.entry.name", 
e.getMessage()));

return false;
}

Moreover what is the reason to have an external loop over the jar's 
entries? Even if we will use several different filter sets in future 
it would be better to use it as optional parameter for Validator.


I mean that the jar file entries iteration code ("580 
jf.stream().filter(e ")...588 }); ) 
processing should be a part of "Validator".

Re: RFR [9] 8134779 & 8134847: Minor usability issues with the jmod tool

[Alan Bateman]

On 28/07/2016 17:30, Chris Hegarty wrote:


This is a minor change for a couple of usability issues with the jmod tool.
It now issues a warning when it encounters duplicate entries, or an out
of place module-info.class.

http://cr.openjdk.java.net/~chegar/8134779_8134847/


This looks okay to me.

-Alan

RFR [9] 8134779 & 8134847: Minor usability issues with the jmod tool

[Chris Hegarty]

This is a minor change for a couple of usability issues with the jmod tool.
It now issues a warning when it encounters duplicate entries, or an out
of place module-info.class.

http://cr.openjdk.java.net/~chegar/8134779_8134847/

https://bugs.openjdk.java.net/browse/JDK-8134779
https://bugs.openjdk.java.net/browse/JDK-8134847

-Chris.

Re: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[Daniel Fuchs]

Hi Christoph,

On 28/07/16 16:05, Langer, Christoph wrote:

Looks good in general, even though the idiom
>if (existing instanceof Stack)
> caught my eye.

I didn't like it either but found no better way to get rid of the warnings. If 
you have a better idea here, let me know :)



The following code:

static class Foo {
T get() { return null; }
}

public static void main(String[] args) throws Exception {

Foo x = new Foo<>();
if (x instanceof Foo) {
System.out.println("ah");
}
}

doesn't generate any warning when compiled with javac -Xlint:all

cheers,

-- daniel

Re: Change of the toString implementation for annotations

[Rafael Winterhalter]

Another remark about "printability". Array types are printed in their
internal form, i.e. "java.lang.Void[].class" becomes
"Ljava.lang.Void;.class" in the string form.

While I very much agree over the improvements of the toString
implementation of the unresolved values, I still wonder if this special
treatment is really necessary and justify the compatibility break. Are the
curly braces meant to avoid confusion from parsing annotation values where
the braces could also be part of a type literal? Any parser would need to
process the annotation string sensitively anyways as string values could
contain any value, if this is the case.

Best regards, Rafael

2016-07-28 15:38 GMT+02:00 Rafael Winterhalter :

> Hi Joe,
> thank you for your answer. Can I ask for the rationale of using {} instead
> of [] only for classes? If the goal was to come closer to the source code
> representation, would this not imply to use curly braces for all array of
> an annotation?
> Thank you for your time! Best regards, Rafael
>
> 2016-07-19 4:13 GMT+02:00 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: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[Langer, Christoph]

Hi Daniel,

thanks for reviewing. Here my comments:

> Looks good in general, even though the idiom
>if (existing instanceof Stack)
> caught my eye.
I didn't like it either but found no better way to get rid of the warnings. If 
you have a better idea here, let me know :)

> Thanks for the new test! I wonder if it should be
> made more strict - with a golden record of the expected
> results.
> 
> In particular, to check that the xmlns="" in element
>  is removed only when it should.
> Before your fix, it was always removed, even when it
> should not have!

Well, I could String-compare the generated XML result with the blueprint - but 
I thought using the DOM API to query for the expected namespaces would be more 
resilient to future changes or platform specifics. But, as above, if you have 
suggestions on how it could be done better, I would love to refactor my test.

Best
Christoph

Re: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[Daniel Fuchs]

Hi Christoph,

Looks good in general, even though the idiom
  if (existing instanceof Stack)
caught my eye.

Thanks for the new test! I wonder if it should be
made more strict - with a golden record of the expected
results.

In particular, to check that the xmlns="" in element
 is removed only when it should.
Before your fix, it was always removed, even when it
should not have!

best regards,

-- daniel


On 28/07/16 14:10, Langer, Christoph wrote:

Hi,



please review my change for the XSLT namespace issue.



Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8162598.1/

Bug: https://bugs.openjdk.java.net/browse/JDK-8162598



The issue has already been discussed in this thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-July/042525.html



Apart from the real fix in LiteralElement.java, method translate(), I’ve
done some further cleanups in a few places. @Joe: The cleanups collide
with some places of your proposed change for
https://bugs.openjdk.java.net/browse/JDK-8158084 where we’d like to do
the same things. So we’ll have to synchronize on pushing.



I’ve also enhanced the test case “TransformerTest” and added a method
which does a regression test for the bug reported.



Thanks in advance for reviewing.



Best regards

Christoph

Re: RFR 8162458 Buffer view implementations use incorrect offset for Unsafe access

[Daniel D. Daugherty]

Paul,

I have marked this bug (JDK-8162458) as an 'integration_blocker'
in order to keep JDK-8151163 from leaving JDK9-hs in its current
state.

Dan


On 7/28/16 4:33 AM, Paul Sandoz wrote:

Hooking in nio dev.

I think this issue is important to review/push soon (i.e. this week) so as it 
does not leak out beyond the hs repo.

—

Incidentally this patch also fixes what might have been a long standing bug in 
the compact method of buffer views.

Here is ByteBufferAsIntBufferB’s compact method:

   public IntBuffer compact() {

   int pos = position();
   int lim = limit();
   assert (pos <= lim);
   int rem = (pos <= lim ? lim - pos : 0);

   ByteBuffer db = bb.duplicate();
   db.limit(ix(lim));
   db.position(ix(0));
   ByteBuffer sb = db.slice();
   sb.position(pos << 2);
   sb.compact();
   position(rem);
   limit(capacity());
   discardMark();
   return this;
   }


For view heap buffers the ix method used to return an offset relative to the 
underlying byte array, not relative to the buffer. this this can result values 
that are beyond the capacity of the buffer.

The following code:

   ByteBuffer bb0 = ByteBuffer.allocate(16);
   ByteBuffer bb4 = bb0.position(4).slice();
   ByteBuffer bb8 = bb4.position(4).slice();
   IntBuffer ib = bb8.asIntBuffer().compact();

results in:

   Exception in thread "main" java.lang.IllegalArgumentException: newLimit > 
capacity: (16 > 8)

Paul.



On 27 Jul 2016, at 13:47, Paul Sandoz  wrote:

Hi,

I made an embarrassing mistake in the fix for

  https://bugs.openjdk.java.net/browse/JDK-8151163
  All Buffer implementations should leverage Unsafe unaligned accessors

The offset calculation for Unsafe access was incorrect, it’s easy to get 
confused because for heap buffers the offset is relative to the array, and for 
direct buffers the address (which can update for slices/duplicates). 
Disturbingly all existing tests were passing both for core and hotspot when i 
pushed to hs.

As a penance i wrote a combinatorial test for buffer views to navigate the 
twisty maze of heap/direct, aligned/unaligned, little/big endian for accessing 
binary data and views from the source buffer.

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8162458-byte-buffer-view-offset-access-incorrect/webrev/

(This may be a duplicate of [1]).

Test has been verified to fail with the existing code. Focused JPRT runs pass, 
but i will kick off core/hotspot runs later on today.

I will push to hs since that is where JDK-8151163 and it has not been 
integrated into jdk9/dev.

Paul.

[1] https://bugs.openjdk.java.net/browse/JDK-8159257
unsafe.cpp: assert(byte_offset < p_size) failed: Unsafe access: offset 32767 > 
object's size 16

For the test runtime/Unsafe/RangeCheck.java I can reproduce a crash in jdk9/dev 
which does not have JDK-8151163, and i can reproduce on jdk9/hs with this fix 
for JDK-8162458.

Re: RFR [9] 8156824: com.sun.jndi.ldap.pool.PoolCleaner should clear its context class loader

[Daniel Fuchs]

On 28/07/16 15:11, Chris Hegarty wrote:

On 28 Jul 2016, at 14:52, Daniel Fuchs  wrote:
>
> On 28/07/16 14:22, Chris Hegarty wrote:

>> Another issue where an internal platform thread is unnecessarily
>> retaining a reference to its creating thread’s context class loader.
>> In this case, it appears to be safe to use an InnocuousThread,
>> which will have a null context class loader.
>>
>> http://cr.openjdk.java.net/~chegar/8156824/01/
>> https://bugs.openjdk.java.net/browse/JDK-8156824

>
> Hi Chris,
>
> I was concerned that Pool.expire() might require permissions
> (it writes to the socket channel and closes the socket),
> but if I'm not mistaken you only need perms to create/bind
> a socket, right?

Correct.


Then I guess this looks OK.

best regards,

-- daniel

Re: RFR [9] 8156824: com.sun.jndi.ldap.pool.PoolCleaner should clear its context class loader

[Chris Hegarty]

> On 28 Jul 2016, at 14:52, Daniel Fuchs  wrote:
> 
> On 28/07/16 14:22, Chris Hegarty wrote:
>> Another issue where an internal platform thread is unnecessarily
>> retaining a reference to its creating thread’s context class loader.
>> In this case, it appears to be safe to use an InnocuousThread,
>> which will have a null context class loader.
>> 
>> http://cr.openjdk.java.net/~chegar/8156824/01/
>> https://bugs.openjdk.java.net/browse/JDK-8156824
> 
> Hi Chris,
> 
> I was concerned that Pool.expire() might require permissions
> (it writes to the socket channel and closes the socket),
> but if I'm not mistaken you only need perms to create/bind
> a socket, right?

Correct.

-Chris.

Re: RFR [9] 8156824: com.sun.jndi.ldap.pool.PoolCleaner should clear its context class loader

[Daniel Fuchs]

On 28/07/16 14:22, Chris Hegarty wrote:

Another issue where an internal platform thread is unnecessarily
retaining a reference to its creating thread’s context class loader.
In this case, it appears to be safe to use an InnocuousThread,
which will have a null context class loader.

http://cr.openjdk.java.net/~chegar/8156824/01/
https://bugs.openjdk.java.net/browse/JDK-8156824


Hi Chris,

I was concerned that Pool.expire() might require permissions
(it writes to the socket channel and closes the socket),
but if I'm not mistaken you only need perms to create/bind
a socket, right?

best regards,

-- daniel



It’s not clear to me why the construction was not always in a
privileged block, maybe “modifyThread” was assumed. We now
definitely need a privileged block as the context class loader is
being set.

-Chris.

Re: Change of the toString implementation for annotations

[Rafael Winterhalter]

Hi Joe,
thank you for your answer. Can I ask for the rationale of using {} instead
of [] only for classes? If the goal was to come closer to the source code
representation, would this not imply to use curly braces for all array of
an annotation?
Thank you for your time! Best regards, Rafael

2016-07-19 4:13 GMT+02:00 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
>

RFR [9] 8156824: com.sun.jndi.ldap.pool.PoolCleaner should clear its context class loader

[Chris Hegarty]

Another issue where an internal platform thread is unnecessarily
retaining a reference to its creating thread’s context class loader.
In this case, it appears to be safe to use an InnocuousThread,
which will have a null context class loader.

http://cr.openjdk.java.net/~chegar/8156824/01/
https://bugs.openjdk.java.net/browse/JDK-8156824

It’s not clear to me why the construction was not always in a
privileged block, maybe “modifyThread” was assumed. We now
definitely need a privileged block as the context class loader is
being set.

-Chris.

RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[Langer, Christoph]

Hi,

please review my change for the XSLT namespace issue.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8162598.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8162598

The issue has already been discussed in this thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-July/042525.html

Apart from the real fix in LiteralElement.java, method translate(), I've done 
some further cleanups in a few places. @Joe: The cleanups collide with some 
places of your proposed change for 
https://bugs.openjdk.java.net/browse/JDK-8158084 where we'd like to do the same 
things. So we'll have to synchronize on pushing.

I've also enhanced the test case "TransformerTest" and added a method which 
does a regression test for the bug reported.

Thanks in advance for reviewing.

Best regards
Christoph

Re: RFR: 8162439: Runtime.Version.parse needs fast-path for major versions

[Paul Sandoz]

> On 27 Jul 2016, at 19:36, Claes Redestad  wrote:
> 
> On 07/25/2016 08:01 PM, Iris Clark wrote:
>> Hi, Claes.
>> 
>>> Webrev: http://cr.openjdk.java.net/~redestad/8162439/webrev.01/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8162439
>> I think that this change looks good.  We provide a shortcut for the common 
>> case where only the major version number is of interest without introducing 
>> a new API.
> 
> Thanks! Any reviewer want to give this the go-ahead?

Looks ok.

I suppose you could do:

  boolean isSimpleNumber(String s) {
for (int i = 0; i < s.length(); i++) {
  char c = s.get(i);
  if (c < ((i > 0) ? ‘0’ : ‘1’) || c > ‘9’)
  return false;
}
return true;
  }


  if (isSimpleNumber(s)) {
...
  } else {
return VersionBuilder.parse(s);
  }

then let VersionBuilder.parse throw errors in incorrectly formatted version 
strings.

Paul.

> 
>> 
>> Note that this has a minor side-effect that invocations of the form 
>> Runtime.Version.parse("notANumber") will now throw NFE instead of IAE.  I 
>> don't think that this is a problem since NFE extends IAE and the order that 
>> the exceptions will be checked is intentionally unspecified.
> 
> Right, I considered the minor behavior difference and thought it would be 
> considered OK and in accordance with the specification, and unproblematic 
> since this API is as of yet unreleased.
> 
> As an aside: I do think specifying both NFE and IAE to be thrown is 
> suspicious when it's not perfectly clear when which one is to be expected, 
> since it invites to wrong code with strict dependencies on which is currently 
> thrown when, even though it's unspecified, which would mean changing the 
> implementation (like in this patch) would have some possibility of messing 
> with compatibility. I wonder if we should simply specify IAE and drop NFE to 
> allow greater flexibility for future implementations?
> 
> /Claes
> 
>> 
>> Regards,
>> iris
>> (not a Reviewer)
>> 
>> -Original Message-
>> From: Claes Redestad
>> Sent: Saturday, July 23, 2016 9:24 AM
>> To: core-libs-dev@openjdk.java.net core-libs-dev
>> Subject: RFR: 8162439: Runtime.Version.parse needs fast-path for major 
>> versions
>> 
>> Hi,
>> 
>> please review this patch to address a startup regression due to use of
>> Runtime.Version.parse("8") etc in JarFile, as introduced by JDK-8150680.
>> This solution introduce a fast-path in case of what appears to be a single 
>> number is sent to Runtime.Version.parse to avoid initializing
>> Runtime.VersionBuilder:
>> 
>> Webrev: http://cr.openjdk.java.net/~redestad/8162439/webrev.01/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8162439
>> 
>> Passes all existing tests.
>> 
>> Thanks!
>> 
>> /Claes
> 

Re: RFR [9] 8157570: sun.rmi.transport.GC retains a strong reference to the context class loader ( was :8160513 )

[Daniel Fuchs]

Hi Chris,

Looks good to me!

best regards,

-- daniel

On 28/07/16 12:40, Chris Hegarty wrote:

On 28 Jul 2016, at 12:28, Alan Bateman  wrote:


On 28/07/2016 11:22, Chris Hegarty wrote:


[ switching to 8157570 as it better describes the issue, rather than the affect 
]



Looks good to me Chris.
Another possibility might be to use InnocuousThread?

Good idea Daniel. I updated the webrev to use InnocuousThread, and
added an assert, since this is no test.

http://cr.openjdk.java.net/~chegar/8157570/



This looks much better and looks good to me. Separately, should the thread name be 
"RMI GC Daemon" to make it a bit clearer in thread dumps.


Thanks for the review Alan. Webrev updated in-place
   http://cr.openjdk.java.net/~chegar/8157570/

-Chris.

Re: RFR 8162458 Buffer view implementations use incorrect offset for Unsafe access

[Paul Sandoz]

> On 28 Jul 2016, at 12:40, Alan Bateman  wrote:
> 
> On 27/07/2016 12:47, Paul Sandoz wrote:
> 
>> Hi,
>> 
>> I made an embarrassing mistake in the fix for
>> 
>>   https://bugs.openjdk.java.net/browse/JDK-8151163
>>   All Buffer implementations should leverage Unsafe unaligned accessors
>> 
>> The offset calculation for Unsafe access was incorrect, it’s easy to get 
>> confused because for heap buffers the offset is relative to the array, and 
>> for direct buffers the address (which can update for slices/duplicates). 
>> Disturbingly all existing tests were passing both for core and hotspot when 
>> i pushed to hs.
>> 
>> As a penance i wrote a combinatorial test for buffer views to navigate the 
>> twisty maze of heap/direct, aligned/unaligned, little/big endian for 
>> accessing binary data and views from the source buffer.
>> 
>> Please review:
>> 
>>   
>> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8162458-byte-buffer-view-offset-access-incorrect/webrev/
>> 
>> (This may be a duplicate of [1]).
>> 
>> Test has been verified to fail with the existing code. Focused JPRT runs 
>> pass, but i will kick off core/hotspot runs later on today.
>> 
> It is disturbing that it wasn't caught by the original tests. The new test 
> does appear to overlap with existing tests but I think that is okay.

Yes, i wanted to write a focused test that explicitly enumerates the test cases 
(and is easy to add more), otherwise it can be really hard to track down which 
combination of properties are failing.


> One small request is to trim down the really long lines as they are annoying 
> when doing side-by-side diffs.
> 

I split up lines over 110 characters or more.


> In any case, the offsets in the updated code look right for me but tests is 
> the only way to catch issues.
> 

Thanks,
Paul.

Re: RFR: 8160605: java/util/SplittableRandom/SplittableRandomTest.java failed with timeout

[Paul Sandoz]

> On 27 Jul 2016, at 19:42, Martin Buchholz  wrote:
> 
> Hi Paul,
> 
> This deletes the testng ports of jsr166 tck random tests.
> 
> If you can remember doing any work worth keeping, other than explicitly 
> testing -Djava.util.secureRandomSeed=true (which we are adding to our tck 
> tests), let me know.
> 
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-RandomTests/ 
> 

None that i can think of.

+1

Paul.

Re: RFR [9] 8157570: sun.rmi.transport.GC retains a strong reference to the context class loader ( was :8160513 )

[Alan Bateman]

On 28/07/2016 12:40, Chris Hegarty wrote:


Thanks for the review Alan. Webrev updated in-place
http://cr.openjdk.java.net/~chegar/8157570/



Looks good.

Re: RFR [9] 8157570: sun.rmi.transport.GC retains a strong reference to the context class loader ( was :8160513 )

[Chris Hegarty]

On 28 Jul 2016, at 12:28, Alan Bateman  wrote:
> 
> On 28/07/2016 11:22, Chris Hegarty wrote:
> 
>> [ switching to 8157570 as it better describes the issue, rather than the 
>> affect ]
>> 
>> 
>>> Looks good to me Chris.
>>> Another possibility might be to use InnocuousThread?
>> Good idea Daniel. I updated the webrev to use InnocuousThread, and
>> added an assert, since this is no test.
>> 
>> http://cr.openjdk.java.net/~chegar/8157570/
>> 
>> 
> This looks much better and looks good to me. Separately, should the thread 
> name be "RMI GC Daemon" to make it a bit clearer in thread dumps.

Thanks for the review Alan. Webrev updated in-place
   http://cr.openjdk.java.net/~chegar/8157570/

-Chris.

Re: RFR [9] 8157570: sun.rmi.transport.GC retains a strong reference to the context class loader ( was :8160513 )

[Alan Bateman]

On 28/07/2016 11:22, Chris Hegarty wrote:


[ switching to 8157570 as it better describes the issue, rather than the affect 
]



Looks good to me Chris.
Another possibility might be to use InnocuousThread?

Good idea Daniel. I updated the webrev to use InnocuousThread, and
added an assert, since this is no test.

http://cr.openjdk.java.net/~chegar/8157570/


This looks much better and looks good to me. Separately, should the 
thread name be "RMI GC Daemon" to make it a bit clearer in thread dumps.


-Alan

Re: RFR 8162458 Buffer view implementations use incorrect offset for Unsafe access

[Alan Bateman]

On 27/07/2016 12:47, Paul Sandoz wrote:


Hi,

I made an embarrassing mistake in the fix for

   https://bugs.openjdk.java.net/browse/JDK-8151163
   All Buffer implementations should leverage Unsafe unaligned accessors

The offset calculation for Unsafe access was incorrect, it’s easy to get 
confused because for heap buffers the offset is relative to the array, and for 
direct buffers the address (which can update for slices/duplicates). 
Disturbingly all existing tests were passing both for core and hotspot when i 
pushed to hs.

As a penance i wrote a combinatorial test for buffer views to navigate the 
twisty maze of heap/direct, aligned/unaligned, little/big endian for accessing 
binary data and views from the source buffer.

Please review:

   
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8162458-byte-buffer-view-offset-access-incorrect/webrev/

(This may be a duplicate of [1]).

Test has been verified to fail with the existing code. Focused JPRT runs pass, 
but i will kick off core/hotspot runs later on today.

It is disturbing that it wasn't caught by the original tests. The new 
test does appear to overlap with existing tests but I think that is 
okay. One small request is to trim down the really long lines as they 
are annoying when doing side-by-side diffs.


In any case, the offsets in the updated code look right for me but tests 
is the only way to catch issues.


-Alan

Re: RFR 8162458 Buffer view implementations use incorrect offset for Unsafe access

[Paul Sandoz]

Hooking in nio dev.

I think this issue is important to review/push soon (i.e. this week) so as it 
does not leak out beyond the hs repo.

—

Incidentally this patch also fixes what might have been a long standing bug in 
the compact method of buffer views.

Here is ByteBufferAsIntBufferB’s compact method:

  public IntBuffer compact() {

  int pos = position();
  int lim = limit();
  assert (pos <= lim);
  int rem = (pos <= lim ? lim - pos : 0);

  ByteBuffer db = bb.duplicate();
  db.limit(ix(lim));
  db.position(ix(0));
  ByteBuffer sb = db.slice();
  sb.position(pos << 2);
  sb.compact();
  position(rem);
  limit(capacity());
  discardMark();
  return this;
  }


For view heap buffers the ix method used to return an offset relative to the 
underlying byte array, not relative to the buffer. this this can result values 
that are beyond the capacity of the buffer.

The following code:

  ByteBuffer bb0 = ByteBuffer.allocate(16);
  ByteBuffer bb4 = bb0.position(4).slice();
  ByteBuffer bb8 = bb4.position(4).slice();
  IntBuffer ib = bb8.asIntBuffer().compact();

results in:

  Exception in thread "main" java.lang.IllegalArgumentException: newLimit > 
capacity: (16 > 8)

Paul.


> On 27 Jul 2016, at 13:47, Paul Sandoz  wrote:
> 
> Hi,
> 
> I made an embarrassing mistake in the fix for
> 
>  https://bugs.openjdk.java.net/browse/JDK-8151163
>  All Buffer implementations should leverage Unsafe unaligned accessors
> 
> The offset calculation for Unsafe access was incorrect, it’s easy to get 
> confused because for heap buffers the offset is relative to the array, and 
> for direct buffers the address (which can update for slices/duplicates). 
> Disturbingly all existing tests were passing both for core and hotspot when i 
> pushed to hs.
> 
> As a penance i wrote a combinatorial test for buffer views to navigate the 
> twisty maze of heap/direct, aligned/unaligned, little/big endian for 
> accessing binary data and views from the source buffer.
> 
> Please review:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8162458-byte-buffer-view-offset-access-incorrect/webrev/
> 
> (This may be a duplicate of [1]).
> 
> Test has been verified to fail with the existing code. Focused JPRT runs 
> pass, but i will kick off core/hotspot runs later on today.
> 
> I will push to hs since that is where JDK-8151163 and it has not been 
> integrated into jdk9/dev.
> 
> Paul.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8159257
> unsafe.cpp: assert(byte_offset < p_size) failed: Unsafe access: offset 32767 
> > object's size 16
> 
> For the test runtime/Unsafe/RangeCheck.java I can reproduce a crash in 
> jdk9/dev which does not have JDK-8151163, and i can reproduce on jdk9/hs with 
> this fix for JDK-8162458.

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

[Frank Yuan]

Hi Daniel

Thank you very much for your comments! Please check my reply inline below:

> 
> Hi Frank,
> 
> Please see my comments inline.
> 
> On 27/07/16 10:27, Frank Yuan wrote:
> > Hi Daniel
> >
> > Would you like to have a look at the following changes before I finish all 
> > rework?
> > It shows runWithAllPerm, and the handling for user.dir property in 
> > FilePolicy.java. The code like runWithTmpPermission is still
> > kept, please ignore them, I will remove them finally.
> >
> > diff -r 1bfe60e61bad test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java
> > --- /dev/null   Thu Jan 01 00:00:00 1970 +
> > +++ b/test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java Wed Jul 27 
> > 02:23:58 2016 -0700
> > @@ -0,0 +1,44 @@
...
> > +addPermission(new RuntimePermission("setIO"));
> > +addPermission(new RuntimePermission("setContextClassLoader"));
> > +addPermission(new RuntimePermission("accessDeclaredMembers"));*/
> 
> Good to see these permissions are now commented: make sure to remove
> the commented code in the final version.
> 
Corrected, thanks!

> > +/*
> > + * set allowAll in current thread context.
> > + */
> > +void setAllowAll(boolean allow) {
> > +policy.setAllowAll(allow);
> > +}
> 
> I'd suggest to return the previous value of allowAll here.
> This will be more convenient to restore the previous value
> in the finally block.
> Since setters usually don't return values you might name it e.g.
> 
> /**
>   * Switches allowAll to the given value and return its
>   * previous value in current thread context.
>   * 
>   * Example of use:
>   * {@code
>   *final boolean before = switchAllowAll(true);
>   *try {
>   * // do some privileged operation here
>   *} finally {
>   * // restore allowAll to its previous value
>   * switchAllowAll(before);
>   *}
>   * }
>   */
> boolean switchAllowAll(boolean allowAll) {
>  // alternatively you could add a switchAllowAll method
>  // to TestPolicy
>  final boolean before = policy.allowAll();
>  policy.setAllowAll(allowAll);
>  return before;
> }
> 
> [..]

switchAllowAll is more flexible, but I would take only one use case, that is
setAllowAll(true));
try {
do something
} finally {
setAllowAll(false);
}

> > +/**
> > + * Run the RunnableWithException with creating a JAXPPolicyManager and
> > + * assigning temporary permissions. It's not thread-safe to use this
> > + * function.
> > + *
> > + * @param r
> > + *RunnableWithException to execute
> > + * @param ps
> > + *assigning permissions to add.
> > + */
> > +public static void tryRunWithPolicyManager(RunnableWithException r, 
> > Permission... ps) throws Exception {
> > +JAXPPolicyManager policyManager = 
> > JAXPPolicyManager.getJAXPPolicyManager(true);
> > +if (policyManager != null)
> > +Stream.of(ps).forEach(p -> policyManager.addTmpPermission(p));
> > +try {
> > +r.run();
> > +} finally {
> > +JAXPPolicyManager.teardownPolicyManager();
> > +}
> > +}
> 
> This method is suspicious because if there existed a JAXPPolicyManager
> before entering it it will be removed after the method finishes.
> I'd suggest removing this method. You can accomplish the same things
> by calling:
> 
> JAXPPolicyManager policyManager =
>  JAXPPolicyManager.getJAXPPolicyManager(true);
> runWithTmpPermission(r, ps);
> 
> since runWithTmpPermission seems to do things right.
> 
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.

> > +
> > +/**
> > + * Run the runnable with assigning temporary permissions. This won't 
> > impact
> > + * global policy.
> > + *
> > + * @param r
> > + *Runnable to run
> > + * @param ps
> > + *assigning permissions to add.
> > + */
> > +public static void runWithTmpPermission(Runnable r, Permission... ps) {
> > +JAXPPolicyManager policyManager = 
> > JAXPPolicyManager.getJAXPPolicyManager(false);
> > +List tmpPermissionIndexes = new ArrayList();
> > +if (policyManager != null)
> > +Stream.of(ps).forEach(p -> 
> > tmpPermissionIndexes.add(policyManager.addTmpPermission(p)));
> > +try {
> > +r.run();
> > +} finally {
> 
> if policyManager is null this will throw NPE.
> Maybe you shoud make addTmpPermission and removeTmpPermission static
> in 

RFR [9] 8157570: sun.rmi.transport.GC retains a strong reference to the context class loader ( was :8160513 )

[Chris Hegarty]

[ switching to 8157570 as it better describes the issue, rather than the affect 
]


> Looks good to me Chris.
> Another possibility might be to use InnocuousThread?

Good idea Daniel. I updated the webrev to use InnocuousThread, and
added an assert, since this is no test.

http://cr.openjdk.java.net/~chegar/8157570/

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8157570

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

[Frank Yuan]

> > +/*
> > + * Install a SecurityManager along with a default Policy to allow 
> > testNG to
> > + * run when there is a security manager.
> > + */
> > +private JAXPPolicyManager() {
> > +// Backing up policy and security manager for restore
> > +policyBackup = Policy.getPolicy();
> > +smBackup = System.getSecurityManager();
> > +
> > +// Set customized policy
> > +setDefaultPermissions();
> > +Policy.setPolicy(policy);
> > +System.setSecurityManager(new SecurityManager());
> > +}
> 
> Will the test suite be configured to run with and without
> SecurityManager? It seems to me, with a TestNG test listener, a
> SecurityManager is always installed. I don't seem to see it checks
> whether the test shall run with a SecurityManager.
> 
> -Joe
> 

It's same as the original code, I think it's to be ready for running in agent 
mode.

Frank