RFR: 8156153: java/lang/System/LoggerFinder/jdk/DefaultLoggerBridgeTest/DefaultLoggerBridgeTest.java fails with java.lang.RuntimeException

[Daniel Fuchs]

Hi,

This is a fix for a subtle test bug caused by a logger being GCed.

bug id:
https://bugs.openjdk.java.net/browse/JDK-8156153
webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8156153/webrev/

This test has been seen failing in hs integration with the following
error:


...
*** With Security Manager, without permissions
...
--System.err:(24/1937)--
java.lang.RuntimeException: identical backend loggers
at DefaultLoggerBridgeTest.test(DefaultLoggerBridgeTest.java:404)
...


The main runs the same test sequentially in 3 different configurations:

- without security manager,
- with security manager and no permissions granted,
- with security manager and permissions granted.

The test attempts to first create a lazy logger for a system class,
then a logger for an application class. It expects that the second
concrete logger will be created first - because it should not be
wrapped in a lazy logger.

However, since the loggers are not referenced outside of the test
method, then it can happen that the application logger gets garbage
collected between one scenario and the next, which will invert the
creation sequence (or more exactly pseudo-creation sequence in
this case) in the next scenario.

To fix this I propose to run each scenario in its own VM - by using
three different @run main lines. This way each scenario will start
with a clean slate.

best regards,

-- daniel

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

On 02/05/16 20:06, Mandy Chung wrote:



On May 2, 2016, at 10:59 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi Mandy,

I applied the suggested changes.

http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.08/index.html



Looks very good and much cleaner.  Nits:

 166 // The LoggingMXBeanSupport class uses reflection to determine

s/LoggingMXBeanSupport/LoggingMXBeanAccess/

Also renaming the variable name “support” to “loggingAccess” (or something like 
that) might help.

You can fix it up before you push.  No need to generate a new webrev.


Thanks! Done.

-- done


Mandy

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

Hi Mandy,

I applied the suggested changes.

http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.08/index.html

best regards,

-- daniel

On 02/05/16 19:00, Mandy Chung wrote:

hanks a lot for the feedback!
>
> New webrev here:
> http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.07/index.html

Nit: in the class spec for LoggingMXBean.java - wrap type names with {@code…} 
such as MBeanServer, PlatformLoggingMXBean.

ManagementFactoryHelper.java
line 333: extra line to be removed?

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

Hi Mandy,

Answers inline, and new webrev at the end.

On 29/04/16 21:55, Mandy Chung wrote:

Hi Daniel,


On Apr 29, 2016, at 8:08 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi,

Please find below a patch [2] that eliminates a static
dependency of java.lang.management on java.util.logging.LoggingMXBean.

When JDK-6876135 was fixed, it introduced the PlatformLoggingMXBean
interface, and recommended using PlatformLoggingMXBean over
LoggingMXBean.


Adding to this, JDK-6876135 prepared the JDK for modularization and 
PlatformLoggingMXBean was introduced that can be replaced with existing usage 
of LoggingMXBean.

With this change, java.management dependency on java.logging will become 
implementation details for logging purpose and can be eliminated completely in 
the future.

About the deprecation, to be specific, LoggingMXBean will no longer be a 
platform MXBean and an instance of LoggingMXBean will not register in the 
platform MBeanServer.

This is a revised wording for the deprecation note for LoggingMXBean:

@deprecated {@code LoggingMXBean} is no longer a {@link 
java.lang.management.PlatformManagedObject platform MXBean} and replaced with 
{@link java.lang.management.PlatformLoggingMXBean}.
It will not register in the platform MBeanServer. Use
{@code ManagementFactory.getPlatformMXBean(PlatformLoggingMXBean.class)} 
instead.


Done.


One question about: ManagementFactory::getPlatformMXBean(MBeanServerConnection, 
PlatformLoggingMXBean.class)
- what would happen if this method is called from an image with java.logging 
module present and connect to
a VM with no java.logging module?
Should the ManagementFactory::getPlatformMXBean spec or PlatformLoggingMXBean 
spec be clarified?


I don't think there's anything to clarify: it will throw
an IllegalArgumentException as specified in the spec.
(it will get an InstanceNotFoundException from the
remote VM, which will be wrapped and thrown as the
cause of an IllegalArgumentException)



ManagementFactoryHelper.java

 191 if (result != null) {
 192 LoggingMXBeanSupport.class.getModule().addReads(m);
 193 }

Reflection access assumes readability now:
  
http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectionWithoutReadability

>
> So this addReads is not needed.

Oh - OK thanks for the pointer!



 252 final Map<String, Method> methods = AccessController.doPrivileged(
 253 new PrivilegedAction<>() {
 254 @Override
 255 public Map<String, Method> run() {
 256 return initMethodMap(impl);
 257 }
 258 });

I believe this doPrivileged is not necessary and so do the other getMethod 
calls.
Probably you are thinking ahead what if java.management may one day be defined
by a child loader of the defining loader of java.logging.
Then you can move doPrivileged to initMethodMap.


The doPrivileged around initMethod is necessary because
the implementation class from which the methods are looked
up is package protected, so unfortunately Method.setAccessible
needs to be called. Another possibility would be to lookup
the method on java.util.logging.LoggingMXBean instead of
looking them up on the concrete implementation class.
In that case setAccessible should no longer be needed.

Do you think I should modify the code to do that?

However your remark made me review the doPrivs and I believe
the one in getMXBeanImplementation() is not needed, since
it invokes a public static method on public exported class.
I removed that.



 217 // skip
- better to throw InternalError since it expects all methods are used?


No. getObjectName() will fall in this bucket - it's not
defined on LoggingMXBean.


 273 throw new SecurityException(ex);
- should not reach here.  SecurityException may cause confusion since this will 
be thrown even without security manager.  could simply throw InternalException


OK - Wrapped in UnsupportedOperationException instead.



 296 private PlatformLoggingImpl(LoggingMXBeanSupport support) {
- perhaps renaming LoggingMXBeanSupport to LoggingProxy to better represent it.


It's not really a proxy - it only implemements "invoke". Maybe it should
be called LoggingMXBeanAccess?



Backward Compatibility considerations:
--

1. Local clients which obtain an instance of the logging
MXBean by calling ManagementFactory.getPlatformMXBean(
   "java.util.logging:type=Logging",
PlatformLoggingMXBean.class)
will no longer be able to cast the result on
java.util.logging.LoggingMXBean.
[There should be few, given that PlatformLoggingMXBean
 already has all the methods defined in LoggingMXBean]



I expect this would be really rare too.


2. ManagementFactory.getPlatformMBeanServer().isInstanceOf(
ObjectName, "java.util.logging.LoggingMXBean"

Re: RFR (JAXP) 8152912: SAX XMLReaderFactory needs to be ServiceLoader compliant

[Daniel Fuchs]

Hi Joe,

This looks much better to me.
I just wonder about the @Deprecated("5") vs @Deprecated("1.5")
We started dropping the 1. for Java 5 - so I guess
@Deprecated("5") is OK. Hopefully Joe Darcy will be able
to confirm :-)

best regards,

-- daniel

On 04/05/16 02:22, huizhe wang wrote:


On 5/3/2016 3:36 AM, Daniel Fuchs wrote:

Hi Joe,

This look good but the implementation might be overly complex,
which makes it difficult to read.


It was basically the existing code with some cleanup. What's in
jarLookup was a copy of the original code. As you can see I was eager to
add ServiceLoader support and forget about it (deprecated) :-)


First:
141 ClassLoader cl = ss.getContextClassLoader();
is misnamed, because as far as I can see this method returns
the context class loader if not null, otherwise the system
class loader.


Make sense. That method was changed. I now renamed it to getClassLoader
with javadoc.


So cl is either the context class loader or the system class
loader and is guaranteed to be non null.
I would suggest adding a comment to make it clear.

This means in turn that the new jarLookup(ClassLoader)
method can be greatly simplified - you can do:
final ClassLoader cl = Objects.requiresNonNull(loader);
and then simplify the if (cl != null) else ... logic,
because you now know that cl cannot be null.


The cl-null checks in both jarLookup and findServiceProvider are removed.

The new webrev also includes some cleanup of warnings, deprecation with
javadocs (since Java SE 5) but without the annotation. I limited the
patch to the sax package only.

New webrev:
http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev01/

Best,
Joe



Best regards,

-- daniel



On 02/05/16 20:30, huizhe wang wrote:

Hi,

Please review a change that adds a step using the ServiceLoader to
XMLReaderFactory's provider-lookup process. Meanwhile, XMLReaderFactory
is deprecated in favor of javax.xml.parsers.SAXParserFactory.

JBS: https://bugs.openjdk.java.net/browse/JDK-8152912
Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev/

The change has been verified by SQE test that Frank will submit soon for
review (that is, when he starts to work at his local time).

Thanks,
Joe

Re: RFR [9] 8153158: Remove sun.misc.ManagedLocalsThread from java.logging

[Daniel Fuchs]

On 18/04/16 08:01, Chris Hegarty wrote:

8056152 added a new constructor to java.lang.Thread to constructing Threads that
do not  inherit inheritable-thread-local initial values. Given there is now a 
supported
API for creating such threads, other areas of the JDK should be updated to use 
it.

This change updates the code in java.logging to use the new Thread constructor.


Hi Chris,

Looks good to me.

best regards

-- daniel



--- a/src/java.logging/share/classes/java/util/logging/LogManager.java
+++ b/src/java.logging/share/classes/java/util/logging/LogManager.java
@@ -42,7 +42,6 @@
 import java.util.stream.Stream;
 import jdk.internal.misc.JavaAWTAccess;
 import jdk.internal.misc.SharedSecrets;
-import sun.misc.ManagedLocalsThread;
 import sun.util.logging.internal.LoggingProviderImpl;

 /**
@@ -254,9 +253,10 @@

 // This private class is used as a shutdown hook.
 // It does a "reset" to close all open handlers.
-private class Cleaner extends ManagedLocalsThread {
+private class Cleaner extends Thread {

 private Cleaner() {
+super(null, null, "Logging-Cleaner", 0, false);
 /* Set context class loader to null in order to avoid
  * keeping a strong reference to an application classloader.
  */
diff --git a/src/java.logging/share/classes/module-info.java 
b/src/java.logging/share/classes/module-info.java
--- a/src/java.logging/share/classes/module-info.java
+++ b/src/java.logging/share/classes/module-info.java
@@ -24,8 +24,6 @@
  */

 module java.logging {
-// 8153158
-requires jdk.unsupported;
 exports java.util.logging;
 provides jdk.internal.logger.DefaultLoggerFinder with
 sun.util.logging.internal.LoggingProviderImpl;

-Chris.

Re: RFR [9] 8147553: Remove sun.misc.ManagedLocalsThread from java.management

[Daniel Fuchs]

Hi Chris,

The changes look good to me.

best regards,

-- daniel

On 18/04/16 12:37, Chris Hegarty wrote:

8056152 added a new constructor to java.lang.Thread to constructing
Threads that do not  inherit inheritable-thread-local initial values.
Given there is now a supported API for creating such threads, other
areas of the JDK should be updated to use it.

This change updates the code in java.management to use the new Thread
constructor.

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

-Chris.

Re: JDK 9 pre-review of JDK-6850612: Deprecate Class.newInstance since it violates the checked exception language contract

[Daniel Fuchs]

Hi Joe,

Changes in java.util.logging and java.management look good.

I glanced at the rest and spotted one issue here:

http://cr.openjdk.java.net/~darcy/6850612.0/src/java.desktop/share/classes/javax/swing/text/html/HTMLEditorKit.java.frames.html

 615 Object o = 
Class.forName("javax.swing.text.html.parser.ParserDelegator");

 616 defaultParser = (Parser) o;

The call to newInstance() is missing at line 615

best regards,

-- daniel

On 21/04/16 18:25, joe darcy wrote:

Hello,

After a generally positive reception, please review the webrev to
implement the deprecation of Class.newInstance and the suppression of
the resulting warnings:

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

There are also some changes in the langtools repo; I'll send a separate
review request for those changes to compiler-dev.

I'll also forward this review request to other areas with affected code.

Thanks,

-Joe

On 4/17/2016 10:31 AM, joe darcy wrote:

Hello,

With talk of deprecation in the air [1], I thought it would be a fine
time to examine one of the bugs on my list

JDK-6850612: Deprecate Class.newInstance since it violates the
checked exception language contract

As the title of the bug implies, The Class.newInstance method
knowingly violates the checking exception contract. This has long been
documented in its specification:


Note that this method propagates any exception thrown by the nullary
constructor, including a checked exception. Use of this method
effectively bypasses the compile-time exception checking that would
otherwise be performed by the compiler. The Constructor.newInstance
method avoids this problem by wrapping any exception thrown by the
constructor in a (checked) InvocationTargetException.


Roughly, the fix would be to turn the text of this note into the
@deprecated text and to add a @Deprecated(since="9") annotation to the
method. There are a few dozen uses of the method in the JDK that would
have to be @SuppressWarning-ed or otherwise updated.

Thoughts on the appropriateness of deprecating this method at this time?

Comments on the bug have suggested that besides deprecating the
method, a new method on Class could be introduced,
newInstanceWithProperExceptionBehavior, that had the same signature
but wrapped exceptions thrown by the constructor call in the same way
Constructor.newInstance does.

Thanks,

-Joe

[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040192.html

Re: JAXP: XSLTC transformer swallows empty namespace declaration which is necessary to undeclare default namespace

[Daniel Fuchs]

Hi Christoph,

I was at first a bit suspicious of your proposed patch, but
I applied it and played with it and could not fault it.
Now I tend to believe this is the correct thing to do (though
that's not my area of expertise).

I tried it with a bit more elaborate content in the "transform"
template:

-- XSL 

http://www.w3.org/1999/XSL/Transform; 
version="1.0">


   
   name="transform"/>

   


xmlns="">
xmlns="">







---

and that gave me the content I expected (ignoring the double
xmlns declaration which seems to be another bug):


-- Formatted result with your patch (newlines added manually) --


  
  
  
  
  
  

---

This seems a more correct result than what the JDK currently does:

-- Current JDK 9/dev ---


  
  
  
  
  
  

---

where the empty default namespace declaration always disappears,
even at line 4 in the element !

Hope this helps,

-- daniel

On 22/07/16 11:38, Langer, Christoph wrote:

Hi,

I have a customer reporting the following phenomena which I believe is an issue.

Consider the following xsl:

-XSL-

http://www.w3.org/1999/XSL/Transform; version="1.0">

   
   
   





-End of XSL-


This is the XML snippet where the XSL gets applied (just a dummy):


The result with the current XSLTC is:

But this would not undeclare the default namespace "ns1" from the element named "root" 
for the element named "test" which was the intention of the xsl. So I believe it should be:


Looking at the coding I came up with the following: 
http://cr.openjdk.java.net/~clanger/webrevs/xsltc-namespaceissue/ With that, 
XSLTC would also emit the namespace attribute for an empty namespace. It works 
for my example but I'm not sure if it is the right thing to do or if it breaks 
things at other places and violates specs elsewhere.

Comparing with the Apache Xalan, I can see that the Apache XSLTC matches the 
JDK XSLTC behavior to suppress the namespace declaration but the interpretative 
transformer (org.apache.xalan.processor.TransformerFactoryImpl) would emit the 
namespace attribute.

Please give me some comments before I open a Bug...

Thanks a lot in advance
Christoph

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

[Daniel Fuchs]

On 26/07/16 00:53, huizhe wang wrote:

To avoid having to grant the permission, the test may choose to read the
property before setting the SecurityManager.  You might not be able to
use TestNG Listeners in such case, or maybe you can by initializing the
properties before the test starts.


Or you can use my trick with an AtomicBoolean for such cases:

set allowAll to true
try {
   read system property
} finally {
   set allowAll to false (or to the value it had before)
}

Adding a ThreadLocal allowAll to BasePolicy
for that purpose is very easy :-)

That should ensure that you only need to give those permissions
to the test that a regular user of the JAXP API would need to
use the JAXP API.

If the test read/writes an XML document from file, then the
FilePermission to read/write that document is something that
a regular user of JAXP would need. So those permission should
be granted to the test through Policy.

If the test reads/writes a system property or creates a directory
or create a class loader to set up an initial configuration for
the test to run, then this is not something a regular user of the
JAXP API would need - so it would then be legitimate to perform this
setup inside a block that sets allowAll to true to locally escape
permissions checks during this setup, thus avoiding to grant those
permissions to all in the Policy (alternatively you could use your
tmpPermissions trick to do that as well - but it is a bit more
complex and adds more clutter than a simple on/off switch).

cheers,

-- daniel

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

[Daniel Fuchs]

On 26/07/16 04:24, Frank Yuan wrote:

Thank you very much for your suggestions! Now I fully understand the rule(at 
least I think so :P)
I will use a runWithAllPerm block surrounding the user setup code as Daniel's 
way. Btw, Daniel, ThreadLocal should not need Atomic
any more, correct?



Hi Frank,

runWithAllPerm is another way to do it.
It uses a ThreadLocal, right?

I agree it's adequate. Just be careful of what might
happen if you run runWithAllPerm inside runWithPermissions,
or runWithPermissions(runnable, a,b,c) with a runnable that
later calls runWithPermission(runnable2, a, d, e) further down
the road.

At the moment I'm not sure whether your code will work correctly
in the presence of such nested invocation (maybe it does),
but because it seems to be index based it's not immediately
obvious (I'm not asking you to change it - just to verify and
confirm that it's something you have taken into account, and
that you're confident that it works).


Best regards,

-- daniel

Re: Review request: JDK-8160398 (jdeps) Replace a list of JDK 8 internal API for detecting if it's removed in JDK 9 or later

[Daniel Fuchs]

On 13/07/16 11:48, Mandy Chung wrote:

On Jul 13, 2016, at 4:55 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
>
> Hi Mandy,
>
> I have not finished reviewing yet - but as early feedback I believe
> that the following probably has a mistake:
>
> jdk.jdeps/share/classes/com/sun/tools/jdeps/Analyzer.java
>
> 392
> 393 if (!jdk8Internals.contains(pn)) {
> 394 return false;
> 395 }
> 396
> 397 return jdk8Internals.contains(pn);
>
> I suspect that one of the 'pn' should be 'cn’ ?

Good catch.  The if-statement should be removed.


Right. Then with that change the fix looks good to me.

best regards,

-- daniel

Re: --dry-run description enhancement

[Daniel Fuchs]

Hi Paul,

On 7/12/16 8:00 AM, Paul Benedict wrote:

Mandy, perhaps there is a JVM technicality here I'm unfamiliar with. My
understanding of loading a class has always been that it's coupled with
running its static initializers. So my inference was that the class does
not get loaded into memory because the latter no longer occurs with
--dry-run.


--dry-run will load the main class without initializing it (= without
  running the static initializers) - otherwise it wouldn't quite
  be a dry run. see:

http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/604fc0a43f5b#l1.22

(the 'false' parameter given to Class.forName())

best regards,

-- daniel



On Jul 11, 2016 9:41 PM, "Mandy Chung"  wrote:




On Jul 12, 2016, at 10:35 AM, Paul Benedict 

wrote:


Mandy, can you please give a look to the first email in this thread and

see if you believe the enhanced message I suggested is helpful? I think it
would be.





Your Suggestion:
"create VM but do not load mainclass or execute main method”

The main class is loaded and your suggested text does not apply.  What
enhanced message do you refer to?

Mandy

[JAXP] RFR: 8153082: Update XSTL compiler to generate classes that invoke addReads

[Daniel Fuchs]

Hi,

Please find below a fix for
8153082: Update XSTL compiler to generate classes that invoke addReads
https://bugs.openjdk.java.net/browse/JDK-8153082

This fix removes a dependency from java.xml to an internal
java base API.

http://cr.openjdk.java.net/~dfuchs/webrev_8153082/webrev.00/

It changes FunctionCall.java to generate an additional
addReads statement before invoking a method on an external
class.

Namely, where the generated translet class used to have
a call to:

   com.foo.Bar.func(...);
   (resp: new com.foo.Bar())

it will now also contain an additional call to Module.addReads
to add the external's class module to the list of modules that
the generated translet's module can read. So the generated
code now looks like:

   .class.getModule().addReads(
   Class.forName("com.foo.Bar").getModule());
   com.foo.Bar.func(...);
   (resp: new com.foo.Bar())

best regards,

-- daniel

Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes that invoke addReads

[Daniel Fuchs]

Hi Christoph,

On 22/07/16 20:23, Langer, Christoph wrote:

Hi Daniel,

looks good to me.

Maybe you'll want to take the chance to update the apache headers in the xalan 
files?



Thanks for your review!

I'm only an occasional wanderer in JAXP land - which files do you see
have an outdated header?

I'll try to ping Joe Wang to get the correct headers.

best regards,

-- daniel

Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes that invoke addReads

[Daniel Fuchs]

Hi Alan,

On 23/07/16 07:57, Alan Bateman wrote:

On 22/07/2016 17:16, Daniel Fuchs wrote:


Hi,

Please find below a fix for
8153082: Update XSTL compiler to generate classes that invoke addReads
https://bugs.openjdk.java.net/browse/JDK-8153082

This fix removes a dependency from java.xml to an internal
java base API.

This looks good. The only thing that looks a bit strange is changing the
class file version during generation. It there any reason not to bump
the default? Separately, we should cache the Configuration as each
translet has the same but that is a separate issue of course.


I've done the experiment and bumped the default class file version
to 49. But then some other tests (e.g. SAXTFactoryTest) started failing
with strange ClassFileFormat errors (see at the end of the mail).

My guess is that the XSLT compiler (or is it BCEL?) is not ready for
a global upgrade of class file version to 49.
It seems that something is sometimes genereting bytecode that's
incompatible with class file version 49
(I verified that those tests would pass with 48, but they fail with 49).

I have unfortunately no idea what exactly is causing the issue - so
I've revised my patch to generate 1.1 compatible byte code instead.
(mixing bytecode that requires v < 49 with bytecode that requires v > 48
 does not sound like a good idea ;-( )

So here is a new webrev that does not require 49 and makes all
the tests happy:

http://cr.openjdk.java.net/~dfuchs/webrev_8153082/webrev.01/

best regards,

-- daniel

-

 ClassFormatError in SAXTFactoryTest when class file
 version is > 48

java.lang.ClassFormatError: Illegal class name "die/verwandlung/" in 
class file 

at java.lang.ClassLoader.defineClass1(java.base@9-internal/Native 
Method)
	at 
java.lang.ClassLoader.defineClass(java.base@9-internal/ClassLoader.java:942)
	at 
java.lang.ClassLoader.defineClass(java.base@9-internal/ClassLoader.java:806)
	at 
com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl$TransletClassLoader.defineClass(java.xml@9-internal/TemplatesImpl.java:197)
	at 
com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl.defineTransletClasses(java.xml@9-internal/TemplatesImpl.java:492)
	at 
com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl.getTransletInstance(java.xml@9-internal/TemplatesImpl.java:529)
	at 
com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl.newTransformer(java.xml@9-internal/TemplatesImpl.java:564)
	at 
com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl.newTransformer(java.xml@9-internal/TransformerFactoryImpl.java:798)
	at 
com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl.newTransformerHandler(java.xml@9-internal/TransformerFactoryImpl.java:1105)
	at 
javax.xml.transform.ptests.SAXTFactoryTest.testcase06(SAXTFactoryTest.java:237)
	at 
jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-internal/Native 
Method)
	at 
jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-internal/NativeMethodAccessorImpl.java:62)
	at 
jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-internal/DelegatingMethodAccessorImpl.java:43)

at java.lang.reflect.Method.invoke(java.base@9-internal/Method.java:533)
	at 
org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:84)

at org.testng.internal.Invoker.invokeMethod(Invoker.java:714)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1231)
	at 
org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:127)

at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:111)
at org.testng.TestRunner.privateRun(TestRunner.java:767)
at org.testng.TestRunner.run(TestRunner.java:617)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:334)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:329)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:291)
at org.testng.SuiteRunner.run(SuiteRunner.java:240)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1224)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1149)
at org.testng.TestNG.run(TestNG.java:1057)
	at 
com.sun.javatest.regtest.TestNGAction$TestNGRunner.main(TestNGAction.java:220)
	at 
com.sun.javatest.regtest.TestNGAction$TestNGRunner.main(TestNGAction.java:184)
	at 
jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-internal/Native 
Method)
	at 
jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-internal/NativeMethodAccessorImpl.java:62)
	at 
jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-internal/DelegatingMethodA

Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes that invoke addReads

[Daniel Fuchs]

On 25/07/16 18:38, Langer, Christoph wrote:

Hi,

looking good. I believe you could also remove the comment sections after the 
headers like:
/*
  * $Id: Constants.java,v 1.7 2006/06/19 19:49:04 spericas Exp $
  */

But no need for new webrev for that, of course :-)


Thanks Christoph! I was unsure whether to keep it or remove it.
I will remove it before pushing.

best regards,

-- daniel



Best regards
Christoph


-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Montag, 25. Juli 2016 19:22
To: Langer, Christoph <christoph.lan...@sap.com>; Joe Wang
<huizhe.w...@oracle.com>; Alan Bateman <alan.bate...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>
Subject: Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes
that invoke addReads

Hi,

Here is the later version of the fix:

- Header files fixed
- Bytecode 1.1 compatible

http://cr.openjdk.java.net/~dfuchs/webrev_8153082/webrev.02/

cheers,

-- daniel

On 25/07/16 16:16, Langer, Christoph wrote:

Hi Daniel,

yes, I just recognized the header is different everywhere. It is a good idea to

consult Joe on this - he was also giving me hints on how to do it correctly 
when I
was touching JAXP.


Best regards
Christoph


-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Montag, 25. Juli 2016 16:43
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: Joe Wang <huizhe.w...@oracle.com>; core-libs-dev 
Subject: Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes
that invoke addReads

Hi Christoph,

On 22/07/16 20:23, Langer, Christoph wrote:

Hi Daniel,

looks good to me.

Maybe you'll want to take the chance to update the apache headers in the

xalan files?




Thanks for your review!

I'm only an occasional wanderer in JAXP land - which files do you see
have an outdated header?

I'll try to ping Joe Wang to get the correct headers.

best regards,

-- daniel

Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes that invoke addReads

[Daniel Fuchs]

Hi,

Here is the later version of the fix:

- Header files fixed
- Bytecode 1.1 compatible

http://cr.openjdk.java.net/~dfuchs/webrev_8153082/webrev.02/

cheers,

-- daniel

On 25/07/16 16:16, Langer, Christoph wrote:

Hi Daniel,

yes, I just recognized the header is different everywhere. It is a good idea to 
consult Joe on this - he was also giving me hints on how to do it correctly 
when I was touching JAXP.

Best regards
Christoph


-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Montag, 25. Juli 2016 16:43
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: Joe Wang <huizhe.w...@oracle.com>; core-libs-dev 
Subject: Re: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes
that invoke addReads

Hi Christoph,

On 22/07/16 20:23, Langer, Christoph wrote:

Hi Daniel,

looks good to me.

Maybe you'll want to take the chance to update the apache headers in the

xalan files?




Thanks for your review!

I'm only an occasional wanderer in JAXP land - which files do you see
have an outdated header?

I'll try to ping Joe Wang to get the correct headers.

best regards,

-- daniel

Re: RFR [9] 8160513: ClassNotFoundException sun.misc.GC when running Tomcat 9 with JDK 9

[Daniel Fuchs]

On 26/07/16 16:24, Chris Hegarty wrote:

The GC.Daemon thread has no need of any user defined class loader,
so should set its context class loader to null before starting, so as to
not inadvertently retain a reference to the creating thread’s context
class loader.

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


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

best regards,

-- daniel



-Chris.

P.S. I added a detailed comment the JIRA issue, for those wondering
why Tomcat is running into this.

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 (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 [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: 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 <daniel.fu...@oracle.com> 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 (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: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Daniel Fuchs]

Hi Frank,

I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:

 107 addPermission(new SecurityPermission("getPolicy"));
 108 addPermission(new SecurityPermission("setPolicy"));
 109 addPermission(new RuntimePermission("getClassLoader"));
 110 addPermission(new RuntimePermission("createClassLoader"));
 111 addPermission(new RuntimePermission("setSecurityManager"));
 112 addPermission(new RuntimePermission("createSecurityManager"));
 113 addPermission(new RuntimePermission("modifyThread"));
 114 addPermission(new PropertyPermission("*", "read, write"));
 115 addPermission(new ReflectPermission("suppressAccessChecks"));
 116 addPermission(new RuntimePermission("setIO"));
 117 addPermission(new RuntimePermission("setContextClassLoader"));
 118 addPermission(new RuntimePermission("accessDeclaredMembers"));

These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.

I had a similar issue when writing logging test, were I wanted
to temporarily disable permission checking in the middle of a test
to perform an infrastructure configuration.

So what I did is use an ThreadLocal to temporarily
disable permission checking - which allows me in my tests to do things
like:

boolean before = allowAll.get().get();
allowAll.get().set(true);
try {
   do something that requires a permission
} finally {
   allowAll.get().set(before);
}

My implementation of Policy::implies also checks for

if (allowAll.get().get()) return true;

This allows me to control more tightly the set of permissions
I want my test to run under, while still being able to
perform any action I want to set up things without having
to give the same permission to all.

Hope this helps,

-- daniel



On 22/07/16 07:59, Frank Yuan wrote:

According to Amy's suggestion, re-generate a webrev 
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix some issues,
please check.

Thanks
Frank


-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?

Thanks,
Amy

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

[Daniel Fuchs]

Hi,

Here is the new webrev with Chris & James feedback
taken in.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

best regards,

-- daniel

On 28/07/16 22:55, Daniel Fuchs wrote:

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: [JAXP] RFR: 8153082: Update XSTL compiler to generate classes that invoke addReads

[Daniel Fuchs]

On 26/07/16 10:44, Alan Bateman wrote:

On 25/07/2016 18:21, Daniel Fuchs wrote:


Hi,

Here is the later version of the fix:

- Header files fixed
- Bytecode 1.1 compatible

http://cr.openjdk.java.net/~dfuchs/webrev_8153082/webrev.02/

This looks okay to me although I think I'd like to understand more as to
why this needs to emit v45 classes (something for another issue).


Thanks Alan.

I logged https://bugs.openjdk.java.net/browse/JDK-8162527 to
track it.

best regards,

-- daniel



-Alan

Re: JAXP: XSLTC transformer swallows empty namespace declaration which is necessary to undeclare default namespace

[Daniel Fuchs]

Hi Christoph,

On 26/07/16 14:53, Langer, Christoph wrote:

Hi Daniel,

you mean with my webrev built in or with the current jdk9 dev?


With the current jdk9 dev - sorry if that was unclear.

cheers,

-- daniel

public static void main(String[] args) throws XMLStreamException,
TransformerConfigurationException, TransformerException {
TransformerFactory trans = TransformerFactory.newInstance();
XMLInputFactory input = XMLInputFactory.newFactory();
XMLOutputFactory output = XMLOutputFactory.newFactory();
XMLEventReader reader = 
input.createXMLEventReader(XSLTTest.class.getResourceAsStream("test.xsl"));

StAXSource source = new StAXSource(reader);
Templates temp = trans.newTemplates(source);
StAXResult result = new 
StAXResult(output.createXMLEventWriter(System.out));

temp.newTransformer().transform(
new 
StAXSource(input.createXMLEventReader(XSLTTest.class.getResourceAsStream("test.xml"))),

result);
}




Best
Christoph

Re: JAXP: XSLTC transformer swallows empty namespace declaration which is necessary to undeclare default namespace

[Daniel Fuchs]

Hi Christoph, Joe,

Actually what I see with the latest dev version of
JDK 9 (eng. build built a few minutes ago) is this:

xmlns="ns1">


Notice that xmlns="ns1" appears twice in the root element.

So maybe there's more than one bug here :-(

cheers,

-- daniel

On 26/07/16 12:56, Langer, Christoph wrote:

Hi Joe,

thanks for looking at this.

Here is my comments:

I'm not sure why empty namespace was explicitly excluded. But for the
2nd part, the developer was clear with a note on the intention. You may
want to try removing the condition statement that excluded the empty
namespace, but keep the 2nd part as is, and then run the tests to see if
there's any issue or a reason why it was excluded.


You are right, the comment for the second part is quite explicit. However, the method 
declaresDefaultNS() of XslElement always returns false and the comment to it says: 
"This method is now deprecated. The new implemation of this class never declares the 
default NS". As I didn't find any other class in the hierarchy overwriting this 
method, I decided to remove it and to cleanup code. Are you ok with that or is it illegal 
here?



On another thought, if the following workaround works for you, we may as
well leave the current implementation as is:
try replacing  in the above, with "


This might work, however, the customer wants to change his current XML toolkit 
to the JDK one and expects his existing XML/XSL code to work as is. So that's 
no option.

I'll go open a bug for that now and prepare a real review.

Best regards
Christoph

Re: JAXP: XSLTC transformer swallows empty namespace declaration which is necessary to undeclare default namespace

[Daniel Fuchs]

On 26/07/16 14:53, Langer, Christoph wrote:

you mean with my webrev built in or with the current jdk9 dev?


... and with your patch applied I see this:

xmlns="ns1">


cheers,

-- daniel

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

[Daniel Fuchs]

.library.JAXPTestUtilities.setSystemProperty;
+
 import java.io.IOException;
 import java.io.StringReader;
 import java.io.StringWriter;
@@ -37,6 +39,7 @@
 import javax.xml.transform.stream.StreamResult;

 import org.testng.Assert;
+import org.testng.annotations.Listeners;
 import org.testng.annotations.Test;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
@@ -46,6 +49,7 @@
  * @bug 6490921
  * @summary Test property org.xml.sax.driver is always applied in transformer 
API.
  */
+@Listeners({jaxp.library.BasePolicy.class})
 public class Bug6490921 {

 public static class ReaderStub extends XMLFilterImpl {
@@ -71,7 +75,7 @@
 public void test01() {
 String xml = "";
 ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", "");
+setSystemProperty("org.xml.sax.driver", "");

 // Don't set 'org.xml.sax.driver' here, just use default
 try {
@@ -91,7 +95,7 @@
 public void test02() {
 String xml = "";
 ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
 try {
 TransformerFactory transFactory = TransformerFactory.newInstance();
 Transformer transformer = transFactory.newTransformer();
@@ -111,7 +115,7 @@
 + "   Hello World!\n" + 
"\n";

 ReaderStub.used = false;
-System.setProperty("org.xml.sax.driver", ReaderStub.class.getName());
+setSystemProperty("org.xml.sax.driver", ReaderStub.class.getName());
 try {
 TransformerFactory transFactory = TransformerFactory.newInstance();
 if (transFactory.getFeature(SAXTransformerFactory.FEATURE) == 
false) {



I think the test looks fine.

cheers,

-- daniel


Thanks
Frank

-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Tuesday, July 26, 2016 3:46 PM
To: Frank Yuan; 'huizhe wang'
Cc: 'Amy Lu'; 'core-libs-dev'
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

On 26/07/16 04:24, Frank Yuan wrote:

Thank you very much for your suggestions! Now I fully understand the rule(at 
least I think so :P)
I will use a runWithAllPerm block surrounding the user setup code as Daniel's 
way. Btw, Daniel, ThreadLocal should not need Atomic
any more, correct?



Hi Frank,

runWithAllPerm is another way to do it.
It uses a ThreadLocal, right?

I agree it's adequate. Just be careful of what might
happen if you run runWithAllPerm inside runWithPermissions,
or runWithPermissions(runnable, a,b,c) with a runnable that
later calls runWithPermission(runnable2, a, d, e) further down
the road.

At the moment I'm not sure whether your code will work correctly
in the presence of such nested invocation (maybe it does),
but because it seems to be index based it's not immediately
obvious (I'm not asking you to change it - just to verify and
confirm that it's something you have taken into account, and
that you're confident that it works).


Best regards,

-- daniel

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

[Daniel Fuchs]

Hi Chris,

On 27/07/16 11:17, Chris Hegarty wrote:

Hi Daniel,

On 25/07/16 19:10, 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


Since mirroredLevel is a strong reference to the same given Level,
in the case where the level's class is the JDK's Level.class, then
you do not need either of the reachabilityFence's.


Yes good catch.


With this change there is a race between checking the weak reference
and accessing the mirroredLevel, this is evident in findLevel(String).
I think this is benign, the code is racy, but not susceptible to any
issues resulting from this.


Yes - nothing would prevent the custom level Object (the
reference's target) to be garbaged collected just after the
mirrored level is returned, but we don't care.
So there would be no point in trying to ensure the reference is
not stale just before returning. That's acceptable.



Otherwise, the changes look good to me.


Thanks!



-Chris

Re: RFR 9: JEP 290: Filter Incoming Serialization Data

[Daniel Fuchs]

Hi Roger,

ObjectInputStream.java:

 179  * If a {@link #setObjectInputFilter(ObjectInputFilter) filter is set}

 184  * A {@link 
ObjectInputFilter.Config#setSerialFilter(ObjectInputFilter) process-wide 
filter}


these two lines should be using {@linkplain, not {@link.

 308 private ObjectInputFilter serialFilter;

This field is supposed to be set only once. We can't use final
because we may not know its value right at construction time, so
the setter tries to do its best to ensure that the field is not
changed after serialization has begun.
To improve that and make it more 'final-like' I would make this
field volatile and the setter synchronized.

best regards,

-- daniel


On 26/07/16 18:57, Roger Riggs wrote:

Hi,

Updated the specdiff and javadoc with SerializablePermission and misc
editorial cleanups.

SpecDiff:
http://cr.openjdk.java.net/~rriggs/filter-diffs/overview-summary.html

Javadoc (subset)
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputStream.html

http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputFilter.html

http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/SerializablePermission.html


Also, noticed that a filter could not distinguish between a reference to
an array class and
the callback to check the size of a zero length array (size == 0).
Modified the
range of the size to be positive when creating an array and otherwise
negative.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-serial-filter-jdk9-8155760/

Roger


On 7/26/2016 12:34 PM, Roger Riggs wrote:

Hi Chris,

yes, its in the webrev, but I neglected to include it in the javadoc
and specdiff updates.

Thanks, Roger


On 7/26/2016 12:20 PM, Chris Hegarty wrote:

Another final thought that just occurred to me…

java.io.SerializablePermission will need its class-level javadoc
updated to
include the new permission target name.

-Chris.


On 25 Jul 2016, at 19:55, Roger Riggs  wrote:

Hi Chris,

Thanks for the review and comments,

Updates in place:

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-serial-filter-jdk9-8155760/

SpecDiff:
http://cr.openjdk.java.net/~rriggs/filter-diffs/overview-summary.html

Javadoc (subset)
http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputStream.html


http://cr.openjdk.java.net/~rriggs/filter-javadoc/java/io/ObjectInputFilter.html

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

[Daniel Fuchs]

Hi Frank,

I am intrigued by these change which do not seem
to have anything to do with the rest. I mean, I'm not opposed
as long as they are intended and that Joe validates them:


http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/ptests/XMLReaderNSTableTest.java.frames.html

118 spf.setNamespaceAware(false);

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.frames.html

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.html

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/publish.xml.frames.html
This looks like a desirable change - but what does it have to do
with enabling security manager?

Also:


http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/unittest/transform/XSLTFunctionsTest.java.frames.html

  70 
tf.setFeature("http://www.oracle.com/xml/jaxp/properties/enableExtensionFunctions;, 
true);


Is this needed only when there is a security manager?



http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java.html


 119 /*
 120 addPermission(new RuntimePermission("getClassLoader"));
 121 addPermission(new RuntimePermission("createClassLoader"));
 122 addPermission(new RuntimePermission("createSecurityManager"));
 123 addPermission(new RuntimePermission("modifyThread"));
 124 addPermission(new PropertyPermission("*", "read,write"));
 125
 126 addPermission(new RuntimePermission("setIO"));
 127 addPermission(new RuntimePermission("setContextClassLoader"));
 128 addPermission(new 
RuntimePermission("accessDeclaredMembers"));*/



Please remove before pushing.




test/javax/xml/jaxp/internaltest/javax/xml/transform/cli/tigertest-in.xml
test/javax/xml/jaxp/internaltest/javax/xml/transform/cli/tigertest.xsl

It seems these two files have been removed, but shouldn't they have
been moved to test/javax/xml/jaxp/internaltest/javax/xml/transform
instead?
I see they are used by 
test/javax/xml/jaxp/internaltest/javax/xml/transform/CLITest.java


Did you forget to hg add them?




Otherwise the new JAXPPolicyManager & its Policy implementation
look good. This is much simpler and better than the first
iteration :-)

This was a *very* long patch - so congratulations for seeing
this through!

cheers,

-- daniel


On 02/08/16 11:20, Frank Yuan wrote:

Hi Joe and Daniel

I have finished the rework as your comments, please check 
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/

JAXP tests use Policy classes, as well as 3 other patterns provided by 
JAXPTestUtilities:
1. runWithAllPerm methods, are only used for user setup code, never run jaxp 
code with them.
2. tryRunWithPolicyManager method, is used to run some test methods with 
security manager and run the others without security
manager in single test class
3. runWithTmpPermission methods, which may run jaxp code with some limited 
permissions, those permissions belong to user permissions
and jaxp code won't doPrivileged for them. E.g. PropertyPermission("MyInputFactory", 
"read") or
FilePermission("/tmp/this/does/not/exist/but/that/is/ok", "read")


Btw, some tests or test methods are disabled or commented sm support 
temporarily for some known jaxp security bugs.

Thanks
Frank

Re: RFR 9: [TESTBUG] 8160151: java/lang/ProcessBuilder/Zombies.java failed with error "1 zombies!"

[Daniel Fuchs]

Hi Roger,

Running in othervm looks good to me.
The only thing I wonder about is these lines which are
removed:

  79 // Log remaining processes
  80 ProcessBuilder pb = new ProcessBuilder("/bin/ps", "-ef");
  81 pb.inheritIO();
  82 Process p2 = pb.start();
  83 p2.waitFor();

Isn't that removing diagnostic information?

best regards,

-- daniel

On 02/08/16 14:45, Roger Riggs wrote:

Please review a test fix for an intermittently failing ProcessBuilder test.
The test should be run in a separate vm so it only sees Zombies from
processes it creates.
When run in-process with jtreg, it may see Zombies from other processes
created by jtreg.
Additional diagnostic information is added as well in case of recurrence.

The test is moved back to tier1.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-zombies-8160151/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8160151

Thanks, Roger

Re: RFR 9: [TESTBUG] 8160151: java/lang/ProcessBuilder/Zombies.java failed with error "1 zombies!"

[Daniel Fuchs]

On 02/08/16 15:03, Roger Riggs wrote:

Hi Daniel,

Those were added in a previous diagnostic iteration.
They do a separate ps which might be too late to provide any using
information.
The first perl script retains  and prints the suspect processes and
should be enough debugging info.
(There is no harm in retaining this extra info but I didn't think it was
particularly useful.)


Thanks Roger. No issue removing this code then.

+1

-- daniel



Thanks, Roger


On 8/2/2016 9:59 AM, Daniel Fuchs wrote:

Hi Roger,

Running in othervm looks good to me.
The only thing I wonder about is these lines which are
removed:

  79 // Log remaining processes
  80 ProcessBuilder pb = new ProcessBuilder("/bin/ps",
"-ef");
  81 pb.inheritIO();
  82 Process p2 = pb.start();
  83 p2.waitFor();

Isn't that removing diagnostic information?

best regards,

-- daniel

On 02/08/16 14:45, Roger Riggs wrote:

Please review a test fix for an intermittently failing ProcessBuilder
test.
The test should be run in a separate vm so it only sees Zombies from
processes it creates.
When run in-process with jtreg, it may see Zombies from other processes
created by jtreg.
Additional diagnostic information is added as well in case of
recurrence.

The test is moved back to tier1.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-zombies-8160151/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8160151

Thanks, Roger

Re: [jdk9] RFR (XXS): 8163180: Typos around @code javadoc tag usage

[Daniel Fuchs]

Hi Ivan,

Looks good!

best regards,

-- daniel

On 04/08/16 13:52, Ivan Gerasimov wrote:

Hello!

In a few places the @code javadoc tag misses the curly bracket.
Would you please review a trivial fix?
http://cr.openjdk.java.net/~igerasim/8163180/00/webrev/

With kind regards,
Ivan

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

[Daniel Fuchs]

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
>
> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change KnownLevel::findByName, 
findByValue and findByLocalizedLevelName to return Optional instead such 
that the parse method implementaiton could be simplified.


We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.
I tried returning Optional but it does not make the
code any simpler.

best regards,

-- daniel

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

[Daniel Fuchs]

Hi Peter,

On 10/08/16 22:06, Peter Levart wrote:

static synchronized void add(Level l) {
purge();
KnownLevel o = new KnownLevel(l);
nameToLevels.computeIfAbsent(l.name, n -> new ArrayList<>()).add(o);
intToLevels.computeIfAbsent(l.value, v -> new ArrayList<>()).add(o);
}


I agree that's a much cleaner piece of code :-)

However I purposefully stayed away from lambdas in
KnownLevel.add because that will be called in the
static initializer of the Level class (when creating
standard levels) - and I didn't want to trigger the
initialization of the lambda infrastructure too early.
This may or may not be an issue any more though.

best regards,

-- daniel

Re: RFR (JAXP) JDK-8163468: javax/xml/jaxp/unittest/validation/Bug6773084Test.java fails intermittently

[Daniel Fuchs]

Hi Frank,

Good analysis of the failure root cause!

The proposed fix looks good to me.

As a side note, are there other multi-threaded tests in JAXP?

If so maybe you'll need a special method in JAXPSecurityManager
to transfer the permissions of the current to another thread
(I mean - find a way to make the other thread run its
 runnable in a similar runWithTmpPermission(...) call
 than the main thread, I believe InheritableThreadLocal would not
 be appropriate nor sufficient for that).

JAXP multi-threaded tests might need to be revisited with that
in mind.

best regards,

-- daniel


On 10/08/16 09:06, Frank Yuan wrote:

Hi,



Would you like to review http://cr.openjdk.java.net/~fyuan/8163468/webrev.00/

It is to fix https://bugs.openjdk.java.net/browse/JDK-8163468



Please check the bug comment for the root cause, this patch moved the code 
which requires file permission to the main test method,
that can guarantee to have the permission. And try to wait the children threads 
to complete in the main test method, that make the
test method more graceful.





Thanks

Frank

Re: Review Request: JDK-8157464: StackWalker.getCallerClass() should not filter out non-invoker frames

[Daniel Fuchs]

On 13/07/16 23:35, Mandy Chung wrote:

I considered that and it’s possible and that would simplify the VM logic would 
be a good thing (do the filtering in the library as much as possible).

This would need to fill an array of MemberName rather than Class object.  
MemberName would need to provide a way to determine if it’s a hidden method 
(that I have a patch).

This would need performance measurement and possible further tuning on the 
batch size to minimize the number of up calls that I think better to separate 
as a follow up issue.


Hi Mandy,

I'm not sure we are talking of the same thing.
This was what I wanted to suggest:

   int frames_decoded = 0;
   for (; !stream.at_end(); stream.next()) {
 Method* method = stream.method();
 int bci = stream.bci();

 if (method == NULL) continue;
+
+if (StackWalk::get_caller_class(mode)) {
+  // JVM_GetCallerClass may return a hidden method
+  // StackWalker::getCallerClass should return the same caller as 
JVM_GetCallerClass

+  // even the jdk implementation does not use it for performance reason
+  if (method->is_ignored_by_security_stack_walk()) continue;
+} else {
+  // filter hidden frames
 if (!ShowHiddenFrames && StackWalk::skip_hidden_frames(mode)) {
   if (method->is_hidden()) {
 if (TraceStackWalk) {
   tty->print("  hidden method: "); method->print_short_name();
   tty->print("\n");
 }
 continue;
   }
 }
+}

 int index = end_index++;
 if (TraceStackWalk) {
   tty->print("  %d: frame method: ", index); 
method->print_short_name();

   tty->print_cr(" bci=%d", bci);

@@ -139,10 +150,14 @@
   Handle stackFrame(frames_array->obj_at(index));
   fill_stackframe(stackFrame, method, bci);
 } else {
   assert (use_frames_array(mode) == false, "Bad mode for get 
caller class");
   frames_array->obj_at_put(index, 
method->method_holder()->java_mirror());

+  // nothing special to do here since we already checked
+  // StackWalk::get_caller_class(mode) above
 }
 if (++frames_decoded >= max_nframes)  break;
   }
   return frames_decoded;
 }





Mandy


On Jul 13, 2016, at 10:55 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi Mandy,

I wonder whether intermediate frames should be skipped always, whether
the method is @CS or not. Indeed StackWalker::getCallerClass() is
intented to be called from methods that are not @CS.

If so the code in stackwalk.cpp could probably be simplified to simply
look at method->is_ignored_by_security_stack_walk() when the
JVM_STACKWALK_GET_CALLER_CLASS is set, and the @CS flag
could be ignored.

(I'm comparing with what I see in jvm.cpp:JVM_GetCallerClass)

what do you think?

best regards,

-- daniel

On 13/07/16 12:01, Mandy Chung wrote:

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

StackWalker::getCallerClass() is specified to return the invoker of the 
MethodHandle and java.lang.reflect.Method for the method calling 
StackWalker::getCallerClass().

StackWalker::getCallerClass() is not used by any @CallerSensitive method.  
Instead one intended usage of StackWalker::getCallerClass() is to be called by 
library code acting as an agent that calls @CallerSensitive method on behalf of 
the true caller and typically it will call an appropriate method with the 
appropriate parameter (e.g. ResourceBundle.getBundle(String, ClassLoader).

MethodHandle for @CS method behaves as if the caller is the lookup class.  The 
actual caller class may not be the lookup class which is left as implementation 
details.  This patch adjusts the stack walker to return the same caller as 
jdk.internal.reflect.Reflection::getCallerClass.

Mandy

Re: Review Request: JDK-8157464: StackWalker.getCallerClass() should not filter out non-invoker frames

[Daniel Fuchs]

Hi Mandy,

I wonder whether intermediate frames should be skipped always, whether
the method is @CS or not. Indeed StackWalker::getCallerClass() is
intented to be called from methods that are not @CS.

If so the code in stackwalk.cpp could probably be simplified to simply
look at method->is_ignored_by_security_stack_walk() when the
JVM_STACKWALK_GET_CALLER_CLASS is set, and the @CS flag
could be ignored.

(I'm comparing with what I see in jvm.cpp:JVM_GetCallerClass)

what do you think?

best regards,

-- daniel

On 13/07/16 12:01, Mandy Chung wrote:

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

StackWalker::getCallerClass() is specified to return the invoker of the 
MethodHandle and java.lang.reflect.Method for the method calling 
StackWalker::getCallerClass().

StackWalker::getCallerClass() is not used by any @CallerSensitive method.  
Instead one intended usage of StackWalker::getCallerClass() is to be called by 
library code acting as an agent that calls @CallerSensitive method on behalf of 
the true caller and typically it will call an appropriate method with the 
appropriate parameter (e.g. ResourceBundle.getBundle(String, ClassLoader).

MethodHandle for @CS method behaves as if the caller is the lookup class.  The 
actual caller class may not be the lookup class which is left as implementation 
details.  This patch adjusts the stack walker to return the same caller as 
jdk.internal.reflect.Reflection::getCallerClass.

Mandy

Re: RFR(XS): 8160457: VersionProps.versionNumbers() is broken

[Daniel Fuchs]

Hi,

WRT to testing - one thing that could be done (possibly in a
followup patch) would be:

class VersionProps {

   ...

   static List parseVersionNumbers(String versionNumber) {
   // parsing code goes there
   }

   static List versionNumbers() {
   return parseVersionNumbers(VERSION_NUMBER);
   }

   ...
}

that would allow writing/adding a unit test for the algorithm
implemented in parseVersionNumbers (either using white-box
with -Xpatch or using reflection and the proper
incantations to invoke parseVersionNumbers from outside the
package).

cheers,

-- daniel


On 28/06/16 15:57, Volker Simonis wrote:

Hi,

can somebody please review this trivial fix:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160457/
https://bugs.openjdk.java.net/browse/JDK-8160457

The problem is that VersionProps.versionNumbers() incorrectly parses a
Java version string because it doesn't skip the separating dots. The
current version only works for one digit versions like "9" but will
fail for any longer version string like for example "9.0.0.1" with:

at 
java.lang.NumberFormatException.forInputString(java.base@9.0.0.1-internal/NumberFormatException.java:65)
at 
java.lang.Integer.parseInt(java.base@9.0.0.1-internal/Integer.java:791)
at 
java.lang.VersionProps.versionNumbers(java.base@9.0.0.1-internal/VersionProps.java:76)
at 
java.lang.Runtime.version(java.base@9.0.0.1-internal/Runtime.java:940)

This also breaks the build which uses a newly built jdk for
bootstrapping if we set '--with-version-patch=1' for example.

An can you PLEASE, PLEASE finally do your internal/early access builds
with '--with-version-patch=1'. It seems really careless to me that you
introduce a new, up to four digit versioning schema but only test the
shortcut version with one digit. I wouldn't be surprised if a version
like "9.0.0.1" breaks more Java applications than the sun.misc.Unsafe
removal :)

Thank you and best regards,
Volker

RFR: 8056285: java/util/logging/CheckLockLocationTest.java java.lang.RuntimeException: Test failed: should have been able to create FileHandler for %t/writable-dir/log.log in writable directory.

[Daniel Fuchs]

Hi,

JDK-8056285 has been sighted again (sigh).
I strongly suspect a configuration issue but I have
no proof. Here is an attempt at gathering some more
data to nail down the root cause of the issue.

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

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8056285/webrev.00

best regards,

-- daniel

Re: [9] RFR 8160611: Clean up ProblemList.txt for closed/resolved issues

[Daniel Fuchs]

Hi John,

JDK-8061177 [1] has been resolved as a duplicate of
JDK-8065756 [2] which is still open.
The correct action for this one might be to leave the
test in the problem list but change the bug ID.

The rest looks good to me - even though
two of these test have been fixed by either adding
diagnosis information or fixed as 'Cannot Reproduce'.

Since most of these tests are in the serviceability
area however, it might be good to get someone from
serviceability-dev (added in cc:) to confirm whether
they want to get these out of the problem list.

best regards,

[1] https://bugs.openjdk.java.net/browse/JDK-8061177
[2] https://bugs.openjdk.java.net/browse/JDK-8065756

-- daniel

On 09/08/16 09:34, John Jiang wrote:

Hi,
The below issues have been closed,
JDK-8130339, JDK-8068645, JDK-8061177, JDK-8058616, JDK-8046285,
JDK-8031555
but the associated items still be contained by ProblemList.txt. This
small patch removes them.

Issue: https://bugs.openjdk.java.net/browse/JDK-8160611
Webrev: http://cr.openjdk.java.net/~jjiang/8160611/webrev.00

Best regards,
John Jiang

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

[Daniel Fuchs]

On 22/07/16 10:15, Frank Yuan wrote:

Hi Daniel

Thank you very much for your review and the comments!


-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

Hi Frank,

I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:

  107 addPermission(new SecurityPermission("getPolicy"));
  108 addPermission(new SecurityPermission("setPolicy"));
  109 addPermission(new RuntimePermission("getClassLoader"));
  110 addPermission(new RuntimePermission("createClassLoader"));
  111 addPermission(new RuntimePermission("setSecurityManager"));
  112 addPermission(new RuntimePermission("createSecurityManager"));
  113 addPermission(new RuntimePermission("modifyThread"));
  114 addPermission(new PropertyPermission("*", "read, write"));
  115 addPermission(new ReflectPermission("suppressAccessChecks"));
  116 addPermission(new RuntimePermission("setIO"));
  117 addPermission(new RuntimePermission("setContextClassLoader"));
  118 addPermission(new RuntimePermission("accessDeclaredMembers"));

These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.

Yes, I agree completely. I would control the permission assignment more tightly:
1. only allow the following necessary permissions in default:
addPermission(new SecurityPermission("getPolicy"));
addPermission(new SecurityPermission("setPolicy"));
addPermission(new RuntimePermission("setSecurityManager"));
2. other permissions in current default set are commonly used for the tests, so 
I would add more Policy classes like existing
FilePolicy, etc.
  //ClassLoaderPolicy, an individual test may only require some of them, but I 
would put them in only one class if you agree
addPermission(new RuntimePermission("getClassLoader"));
addPermission(new RuntimePermission("createClassLoader"));
addPermission(new RuntimePermission("setContextClassLoader"));

  // PropertyPolicy, there are various properties needed by the tests, I would 
not classify them further...
addPermission(new PropertyPermission("*", "read, write"));

  //Reflection permissions, jtreg may require them in deed, so I may add them 
in default set
addPermission(new ReflectPermission("suppressAccessChecks"));
addPermission(new RuntimePermission("accessDeclaredMembers"));

  //other permissions are used in less tests, I may use tryRunWithTmpPermission 
instead of Policy class
addPermission(new RuntimePermission("setIO"));
addPermission(new RuntimePermission("createSecurityManager"));
addPermission(new RuntimePermission("modifyThread"));

How about you think?


It would definitely improve things - but then all the classes
in the test that runs with this new policy class will inherit
from these permissions as well. This may or may not be an issue.
(I confess I haven't looked at all the webrev ;-()



I had a similar issue when writing logging test, were I wanted
to temporarily disable permission checking in the middle of a test
to perform an infrastructure configuration.

So what I did is use an ThreadLocal to temporarily
disable permission checking - which allows me in my tests to do things
like:

boolean before = allowAll.get().get();
allowAll.get().set(true);
try {
do something that requires a permission
} finally {
allowAll.get().set(before);
}


JAXPTestUtilities.tryRunWithTmpPermission is similar with this, see the example:
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/test/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6555001.java.sdiff.htm
l


Yes that part looks fine.

cheers,

-- daniel


My implementation of Policy::implies also checks for

if (allowAll.get().get()) return true;

This allows me to control more tightly the set of permissions
I want my test to run under, while still being able to
perform any action I want to set up things without having
to give the same permission to all.

Hope this helps,

-- daniel



On 22/07/16 07:59, Frank Yuan wrote:

According to Amy's suggestion, re-generate a webrev 
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix some

issues,

please check.

Thanks
Frank


-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?

Thanks,
Amy

Re: RFR: jsr166 jdk9 integration wave 7

[Daniel Fuchs]

On 30/06/16 01:04, Doug Lea wrote:

On Wed, Jun 29, 2016 at 8:38 AM, Daniel Fuchs <daniel.fu...@oracle.com
<mailto:daniel.fu...@oracle.com>> wrote:



I mean something like: "The maximum number of spare threads used by the
common pool is 256: to arrange the same value as is used by default
for the
common pool, use {@code 256} plus the parallelism level for {@code
maximumPoolSize}."



Thanks! I added a variant of this sentence.


On 30/06/16 12:20, Martin Buchholz wrote:

Webrev regenerated with updates.
Lots of rework for the atomic VarHandle specs.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/



Thanks Doug, Martin,

I find the new text is indeed more clear :-)

best regards,

-- daniel

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

Hi Mandy,

On 21/06/16 17:01, Mandy Chung wrote:



On Jun 21, 2016, at 8:39 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

I don't understand this scenario.
ConfigurationData should remain as simple as possible.
Logger.getLogger() / LogManager.demandSystemLogger() will never return
a logger before it has been configured.
When we're merging the configuration data the system logger has
not been configured yet. Level etc are already volatile in Logger and
we should not introduce any new synchronization there.


This is the scenario I was thinking about - would this ever happen?

T1 is creating a system logger named “foo” and calling this.importConfg(other) 
at line 462 after setLevel(otherLevel) is done.

this is the system logger “foo” and other is the user-created logger “foo”.

T2 is calling other.setLevel(newLevel) on user-created logger “foo”.


Indeed, good catch! I should have seen that :-(

Here is a patch that should take care of the issue:

http://cr.openjdk.java.net/~dfuchs/webrev_8159245/webrev.05

Thanks for your offline suggestions on how to deal with that
scenario.

best regards,

-- daniel



Mandy

Re: RFR: jsr166 jdk9 integration wave 7

[Daniel Fuchs]

Hi Martin,

I was looking at the new constructor's API documentation
in ForkJoinPool - and  somehow got confused by the sentence:

2235  * @param maximumPoolSize [...
2241  * ...]  To
2240  * arrange the same value as is used by default for the common
2241  * pool, use {@code 256} plus the parallelism level.

I mean - this looks bizarre, until you understand that the
common pool has a maxSpares of 256 - which means that its
actual max core pool size is 256 + parallelism level
(am I getting that part right?).

I wonder if it would be worth expanding on the rationale
for that value?

I mean something like: "The maximum number of spare threads
used by the common pool is 256: to arrange the same value as is
used by default for the common pool, use {@code 256} plus the
parallelism level for {@code maximumPoolSize}."

best regards,

-- daniel



On 27/06/16 20:38, Martin Buchholz wrote:

jsr166 has been pervasively modified to use VarHandles, but there are some
other pending changes (that cannot be cleanly separated from VarHandle
conversion).  We expect this to be the last feature integration for jdk9.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

We're asking Paul to do most of the review work here, as owner of
VarHandles JEP and as Oracle employee.
We need approval for API changes in

https://bugs.openjdk.java.net/browse/JDK-8157523
Various improvements to ForkJoin/SubmissionPublisher code

https://bugs.openjdk.java.net/browse/JDK-8080603
Replace Unsafe with VarHandle in java.util.concurrent classes

There is currently a VarHandle bootstrap problem with
ThreadLocal/AtomicInteger that causes
java/util/Locale/Bug4152725.java
to fail.  Again I'm hoping that Paul will figure out what to do.  In the
past rearranging the order of operations in  has worked for similar
problems.  It's not surprising we have problems, since j.u.c. needs
VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably
AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very
low-level concurrency infrastructure that does not use VarHandles, only for
bootstrap?

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

On 01/07/16 16:33, Mandy Chung wrote:

I think the additional check is unecessary but it’s harmless if you prefre to 
keep that.


Oh - I see - the permission check in ConfigurationData::merge
can be removed: ConfigurationData is a private class and
config is a private field. I will do that.

cheers,

-- daniel


Ship it.

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

On 01/07/16 16:19, Mandy Chung wrote:

I'd prefer to keep the doPrivileged in LogManager so that
> Logger.mergeWithSystemLogger can call checkpermission().
>
> From a conceptual point of view it's only when calling
> this method from LogManager that we want to be privileged,
> even though the method is package private and only called
> from LogManager…

Why is this extra checkPermission necessary?


Because we're importing an application logger configuration
inside a system logger. Although the method can't be called
from outside the package I'd prefer to keep the permission
check in there. Also it's better to do it up front than
having it fail midway when we later call addHandler or
setLevel.

best regards,

-- daniel

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

[Daniel Fuchs]

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:



On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:


http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change KnownLevel::findByName, 
findByValue and findByLocalizedLevelName to return Optional instead such 
that the parse method implementaiton could be simplified.


We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.


So either findByName(String name, boolean mirror) or two methods: 
findLevelByName and findMirroredLevelByName??

Or seriously consider to remove KnownLevel class by introducing a new Level 
subclass with final Level.getName, Level.getLocalizedName, 
Level.getResourceBundleName methods??

Mandy

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

[Daniel Fuchs]

Hi Peter,

On 16/08/16 13:05, Peter Levart wrote:

Hi Daniel,

Passing Stream returning extractors to Stream::flatMap is one way
of doing it. The other would be to make findByXXX methods return
Optional and then use Optional::map method on the result to
extract the needed property, so instead of:

Optional level = findByName(name, KnownLevel::referent);

You could simply make findByName return Optional and then:

Optional level = findByName(name).map(Reference::get);

Just plain null-when-absent-returning getters used with Optional::map
would do.


Well - not exactly because when looking for levelObject the KnownLevel
could become stale after it's returned. What we want is loop until
we find a levelObject which is not null (that's why the previous
version had some while loops in Level.parse).

But maybe I'm misunderstanding what you are saying?

best regards,

-- daniel




Regards, Peter

On 08/16/2016 12:42 PM, Daniel Fuchs wrote:

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:



On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fu...@oracle.com>
wrote:

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs
<daniel.fu...@oracle.com> wrote:


http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change
KnownLevel::findByName, findByValue and findByLocalizedLevelName to
return Optional instead such that the parse method
implementaiton could be simplified.


We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.


So either findByName(String name, boolean mirror) or two methods:
findLevelByName and findMirroredLevelByName??

Or seriously consider to remove KnownLevel class by introducing a new
Level subclass with final Level.getName, Level.getLocalizedName,
Level.getResourceBundleName methods??

Mandy

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

[Daniel Fuchs]

Hi Peter,

Rereading the comment and looking at the code I see
that the comment is actually right. There are two methods
with similar names:

Level.getLocalizedName() which can be overridden by subclasses
and Level.getLocalizedLevelName() which cannot.

By default Level.getLocalizedName() calls Level.getLocalizedLevelName(),
and  findByLocalizedLevelName() calls Level.getLocalizedLevelName() too,
not the overridable Level.getLocalizedName().

best regards,

-- daniel

On 16/08/16 13:59, Peter Levart wrote:

Hi Daniel,

Another place that is not clear to me is the following (in old code):

 585 // Returns a KnownLevel with the given localized name matching
 586 // by calling the Level.getLocalizedLevelName() method (i.e. found
 587 // from the resourceBundle associated with the Level object).
 588 // This method does not call Level.getLocalizedName() that may
 589 // be overridden in a subclass implementation
590 static synchronized KnownLevel findByLocalizedLevelName(String name) {
591 for (List levels : nameToLevels.values()) {
592 for (KnownLevel l : levels) {
593 String lname = l.levelObject.getLocalizedLevelName();
594 if (name.equals(lname)) {
595 return l;
596 }
597 }
598 }
599 return null;
 600 }

In spite of claiming that "this method doesn't call
Level.getLocalizedName() that may be overridden in subclass", it does
just that. it calls the getLocalizedLevelName on the
KnownLevel.levelObject, which is a user-constructed object. You changed
the code into:

 627 static synchronized Optional
findByLocalizedLevelName(String name,
 628 Function selector) {
 629 purge();
 630 return nameToLevels.values()
 631 .stream()
 632 .flatMap(List::stream)
 633 .flatMap(selector)
 634 .filter(lo ->
name.equals(lo.getLocalizedLevelName()))
 635 .findFirst();
 636 }

Which calls getLocalizedName() on the Level object selected by the given
selector. In line 385 you pass in the selector to select the
mirroredLevel, but in line 487, you pass in the selector to select the
referent.

So which one is the right one? If you want to obey the comment it should
always be the mirroredLevel which is checked against localized name, right?

Regards, Peter

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

[Daniel Fuchs]

Hi Peter,


Now that you have various findByXXX methods return Optional, you
could make methods that use them more Optional-API-powered. For example,
findLevel could be written as a single expression:


Right. But do you mind if I leave that for another day?
I'm not too keen on multi-line code statements embedded
in lambdas - but I feel like we did enough restructuring
for this fix now ;-)

From  your other mail:

Oh, yes. I missed Chris' comment. Perhaps just add a comment about that for 
posterity?


I added the following comment to stress out that
a reachability fence is not needed (at the two places
where it occurred):

 479 // add new Level.
 480 Level levelObject = new Level(name, x);
 481 // There's no need to use a reachability fence here because
 482 // KnownLevel keeps a strong reference on the level when
 483 // level.getClass() == Level.class.
 484 return KnownLevel.findByValue(x, KnownLevel::referent).get();

For the record here are the changes - including Mandy's suggestions.
If there's no objection I will push them after testing has passed,
since Mandy said she didn't require a new webrev.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.03

best regards,

-- daniel

On 16/08/16 14:54, Peter Levart wrote:

Hi Daniel,

Now that you have various findByXXX methods return Optional, you
could make methods that use them more Optional-API-powered. For example,
findLevel could be written as a single expression:

static Level findLevel(String name) {
return KnownLevel
.findByName(Objects.requireNonNull(name), KnownLevel::mirrored)
.or(() -> {
// Now, check if the given name is an integer.  If so,
// first look for a Level with the given value and then
// if necessary create one.
try {
int x = Integer.parseInt(name);
return KnownLevel
.findByValue(x, KnownLevel::mirrored)
.or(() -> {
// add new Level
new Level(name, x);
return KnownLevel.findByValue(x,
KnownLevel::mirrored);
});
} catch (NumberFormatException ex) {
// Not an integer.
// Drop through.
return Optional.empty();
}
})
.or(() -> KnownLevel.findByLocalizedLevelName(name,
KnownLevel::mirrored))
.orElse(null);
}


Same with parse:

public static synchronized Level parse(String name) throws
IllegalArgumentException {
return KnownLevel
// Look for a known Level with the given non-localized name.
.findByName(Objects.requireNonNull(name), KnownLevel::referent)
.or(() -> {
// Now, check if the given name is an integer.  If so,
// first look for a Level with the given value and then
// if necessary create one.
try {
int x = Integer.parseInt(name);
return KnownLevel
.findByValue(x, KnownLevel::referent)
.or(() -> {
// add new Level.
new Level(name, x);
return KnownLevel.findByValue(x,
KnownLevel::referent);
});
} catch (NumberFormatException ex) {
// Not an integer.
// Drop through.
return Optional.empty();
}
})
// Finally, look for a known level with the given localized
name,
// in the current default locale.
// This is relatively expensive, but not excessively so.
.or(() -> KnownLevel.findByLocalizedLevelName(name,
KnownLevel::referent))
// OK, we've tried everything and failed
.orElseThrow(() -> new IllegalArgumentException("Bad level
\"" + name + "\""));
}


Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:



On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fu...@oracle.com>
wrote:

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs
<daniel.fu...@oracle.com> wrote:


http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change
KnownLevel::findByName, findByValue and findByLocalizedLevelName to
return O

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

[Daniel Fuchs]

Hi Peter,

On 17/08/16 12:20, Peter Levart wrote:

Hi Daniel,

Thinking of this patch from the compatibility standpoint, aren't you
afraid that someone might be using the following idiom:

public class MyLevel extends java.util.logging.Level {
static {
new MyLevel("LEVEL1", 1);
new MyLevel("LEVEL2", 2);
...
}

public static void init() {}

private MyLevel(String name, int value) {
super(name, value);
}
...
}

...and then use those levels in code like:

MyLevel.init();
...
Level level = Level.parse("LEVEL1") ...


That's a possibility, and maybe that would deserve to be noted as
a side effect of the fix in the release notes.

I do not believe this is something we would want to actively support
though: the usual idiom for custom level would rather be to define
them as public final static constant, and use a direct reference
in the code.

Something like:

public class MyLevel extends Level {

public static final MyLevel LEVEL1 = new MyLevel("LEVEL1", 1);
...
}

   Level level = MyLevel.LEVEL1;

or even more simpler:

   public static class Levels {

  public static Level LEVEL1 = new Level("LEVEL1", 1) {};
  ...
   }


So maybe this is a risk we should take. Mandy, what do you think?

The other alternative is not to fix the bug as it's been here
for several releases ;-)

-- daniel




With the proposed patch, this will not work any more as custom Level
subclass instances will possibly be GCed before being "parsed" and
retained.

To prevent that from happening and still allow custom Level subclasses
and their class loaders to be garbage-collectable, I recommend that you
"pin" each constructed subclass instance to its ClassLoader with a
strong reference. You could use a ClassLoaderValue for this purpose,
like in the following addition to your patch (see KnownLevel.add):

http://cr.openjdk.java.net/~plevart/jdk9-dev/Level.known/webrev.01/

Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:



On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fu...@oracle.com>
wrote:

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs
<daniel.fu...@oracle.com> wrote:


http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change
KnownLevel::findByName, findByValue and findByLocalizedLevelName to
return Optional instead such that the parse method
implementaiton could be simplified.


We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.


So either findByName(String name, boolean mirror) or two methods:
findLevelByName and findMirroredLevelByName??

Or seriously consider to remove KnownLevel class by introducing a new
Level subclass with final Level.getName, Level.getLocalizedName,
Level.getResourceBundleName methods??

Mandy

RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

Hi,

Please find below a simple fix for:

8173898: StackWalker.walk throws InternalError if called
 from a constructor invoked through reflection.
https://bugs.openjdk.java.net/browse/JDK-8173898

http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.00/

best regards,

-- daniel

Re: RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

Hi,

Please find below a new webrev. Part of the fix was mising
in the previous one. I also took the opportunity to replace
the test's assertEquals with those from org.testng.Assert.

http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.01

best regards,

-- daniel

On 03/02/17 19:51, Daniel Fuchs wrote:

Hi,

Please find below a simple fix for:

8173898: StackWalker.walk throws InternalError if called
 from a constructor invoked through reflection.
https://bugs.openjdk.java.net/browse/JDK-8173898

http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.00/

best regards,

-- daniel

Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Daniel Fuchs]

Hi Rob,

This is not a code I know well - but here are a couple
of comments:

LdapCtxFactory.java:

168 NamingException ne = new NamingException();
232 NamingException ne = new NamingException();

Creating an exception is somewhat costly as it records the
backtrace in the Throwable constructor. I don't know if
it's an issue there but I thought I'd mention it. It might
be better to create the exception only if you're actually
going to throw it.

 186 } catch (Exception e) {
 187 ne.setRootCause(e);
 188 }

This will catch the NamingException thrown at line 173
and set it at the cause of another NamingException that
has no message. Maybe it would be better to have two
catch clauses:

 186 } catch (NamingException e) {
 187 throw e;
 186 } catch (Exception e) {
 ... transform it to NamingException ...

In getDnsUrls:

 198 if (env.containsKey(LdapCtx.DNS_PROVIDER)
 199 && env.get(LdapCtx.DNS_PROVIDER) != null
 200 && !env.get(LdapCtx.DNS_PROVIDER).equals(""))

I'd suggest to simplify this and make a single lookup:

 String providerClass = env.get(LdapCtx.DNS_PROVIDER);
 if (providerClass != null && !providerClass.isEmpty()) {
 ...
 Class cls = Class.forName(providerClass, true, cl);

best regards,

-- daniel


On 01/02/17 20:05, Rob McKenna wrote:

New webrev with updated test here:

http://cr.openjdk.java.net/~robm/8160768/webrev.03/

-Rob

On 26/01/17 04:03, Rob McKenna wrote:

Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
again) That code was actually supposed to be in the getDnsUrls method.
Updated in place:

http://cr.openjdk.java.net/~robm/8160768/webrev.02/

I'll add a couple of tests for the SecurityManager along with some input
checking tests too.

    -Rob

On 25/01/17 05:50, Daniel Fuchs wrote:

Hi Rob,

I believe you should move the security check to before
the class is actually loaded, before the call to
 171 List urls = getDnsUrls(url, env);

best regards,

-- daniel

On 25/01/17 17:44, Rob McKenna wrote:

I neglected to include a security check so I've cribbed the one from
OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:

http://cr.openjdk.java.net/~robm/8160768/webrev.02/

Note, this wraps the SecurityException in a NamingException. Presumably
its better to throw something than simply leave the default value in
place, but feedback appreciated!

   -Rob

On 25/01/17 04:31, Rob McKenna wrote:

Hi folks,

I'm looking for feedback on this suggested fix for the following bug:

https://bugs.openjdk.java.net/browse/JDK-816076
http://cr.openjdk.java.net/~robm/8160768/webrev.01/

This is something that has come up a few times. Basically in certain
environments (e.g. MS Active Directory) there is a dependence on
DNS records to provide a pointer to the actual ldap server to be used
for a given LdapCtx.PROVIDER_URL where the url itself simply points to the
domain name.

This fix add a new Ldap context property which allows a user to specify a
class (implementing BiFunction) which can perform any necessary extra steps
to derive the ldap servers hostname/port from the PROVIDER_URL.

   -Rob

Re: RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

Hi Paul,

On 03/02/17 21:54, Paul Sandoz wrote:



On 3 Feb 2017, at 11:51, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi,

Please find below a simple fix for:

8173898: StackWalker.walk throws InternalError if called
from a constructor invoked through reflection.
https://bugs.openjdk.java.net/browse/JDK-8173898

http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.00/



Looks ok.

Do you need invoke Dummy.create via reflection?

  71 List found = ((Dummy)Dummy.class.getMethod("create")
  72  .invoke(null)).found;


No. But since the bug is about reflection frames not
being filtered out correctly I added it for good measure.

best regards,

-- daniel



Paul.



best regards,

-- daniel

RFR: 8173607: JMX RMI connector should be in its own module

[Daniel Fuchs]

Hi,

Please find below a patch for:

8173607: JMX RMI connector should be in its own module
https://bugs.openjdk.java.net/browse/JDK-8173607

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.05

This patch makes it possible to remove the requires transitive
java.rmi from the java.management module.

For convenience here is the HTML module-level API documentation
produced by javadoc for the new java.management.rmi module:

http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.05/java.management.rmi-summary.html

The patch has some temporary hacks (mainly ConnectorBootstrap.java)
that we will be able to revert when
JDK-8173608 Separate JDK management agent from java.management module
is pushed.

The changes were mainly mechanical except for:

ConnectorBootstrap.java: see above
JMXConnectorFactory.java/JMXConnectorServerFactory.java:
   ServiceLoader is now used to load the default RMI Connector provider
   as a service. There should however be no observable behavior change
   to existing application.
ProxyRef.java, Unmarshal.java: are moved to a new
   com.sun.jmx.remote.internal.rmi package in the new module.
ClientNotifForwarder.java: is modified to no longer depend on
   java.rmi.UnmarshalException - NotSerializableException is used
   instead
RMIConnector.java: fetchNotif is updated to throw
   NotSerializableException instead of UnmarshalException
   The PRef serialization bytes are updated to accomodate the
   new ProxyRef package name.

Some further cleanup of java.base and java.rmi module-info.java
should become possible once JDK-8173608 is in (as well as moving
RMIExporter to java.management.rmi - which is not possible yet).

Further cleanup of @modules in tests might become possible as
well.

Note:
JCK tests for api/javax_management and api/java_lang/management
are passing with this change.

best regards,

-- daniel

Re: RFR: 8173607: JMX RMI connector should be in its own module

[Daniel Fuchs]

Hi Mandy,

On 01/02/17 05:11, Mandy Chung wrote:



On Jan 31, 2017, at 10:26 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi,

Please find below a patch for:

8173607: JMX RMI connector should be in its own module
https://bugs.openjdk.java.net/browse/JDK-8173607

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.05



This mostly looks good.  I’d like to see an updated webrev after you sync with 
JDK-8173608.  I believe you can revert test/sun/management/jdp and 
test/sun/management/jmxremote tests change and ConnectorBootstrap.java as you 
noted.  Also you can run jdeps —-check on java.rmi, java.management, and 
jdk.management.agent to update its requires and qualified exports.  
java.management should no longer require java.rmi and the qualified exports 
from java.rmi is not needed.


Here is the updated webrev, rebased after pulling JDK-8173608, and with
your feedback below integrated.

I am pleased to report that java.management no longer requires
java.rmi or java.naming :-)

Compared to previous version, then RMIExporter has been moved
to java.management.rmi, various module-info.java have been
cleaned up, some @modules in tests have been updated (mostly
due to the RMIExporter move).

I have also improved some javadoc comments in JMXConnectorFactory.java
No changes in build files compared to webrev.05

http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.06
http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.06/java.management.rmi-summary.html

best regards,

-- daniel



java.management.rmi module-info.java
  32  * @see javax.management.remote.rmi

This @see is not needed.  This package is listed in the exported packages table.

JMXConnectorFactory.java
   I like the ProviderFinder approach to look up the custom providers and 
platform providers; and shared code used by both JMXConnectorFactory and 
JMXConnectorServerFactory which is good.

   Nit: line 481-491 - this is javadoc and probably /* .. */ comment block is 
more appropriate here.



Some further cleanup of java.base and java.rmi module-info.java
should become possible once JDK-8173608 is in (as well as moving
RMIExporter to java.management.rmi - which is not possible yet).



Yes.


Further cleanup of @modules in tests might become possible as
well.



Hope there will be a bulk @modules cleanup some time.

Thanks
Mandy

Re: RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

Hi Mandy,

On 06/02/17 00:16, Mandy Chung wrote:

Hi Daniel,

Thanks for the patch and uncover that Constructor::newInstance is not covered 
by SHOW_REFLECT_FRAMES.

The change looks okay.  The javadoc of SHOW_REFLECT_FRAMES can be clarified to 
include Constructor::newInstance.  As for the test, can you separate this in a 
new test to test showing and hiding reflecton frames and also getCallerClass?


Please find below a new webrev that incorporates your feedback.
I have added a new test, and also tweaked the API documentation
in StackWalker.java as you suggested:
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.03/

For convenience here is the corresponding specdiff for the
API documentation tweaks in StackWalker.java:

- StackWalker.Option.SHOW_REFLECT_FRAMES:
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.03/StackWalker.Option-report.html

- StackWalker::getCallerClass:
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.03/StackWalker-report.html

best regards,

-- daniel




Mandy


On Feb 3, 2017, at 11:51 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi,

Please find below a simple fix for:

8173898: StackWalker.walk throws InternalError if called
from a constructor invoked through reflection.
https://bugs.openjdk.java.net/browse/JDK-8173898

http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.00/

best regards,

-- daniel

Re: RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

On 08/02/17 15:58, Mandy Chung wrote:

I disagree: the text starts by saying:
"By default reflection frames are hidden. The reflection
frames include […]"

209  * Shows all reflection frames.

This is the first sentence.  line 216-217 repeats line 209.


but the sentence at lines 216-217 is the only place where we say what
the option actually does: it shows all reflection frames.
I think it's needed.
Possibly the sentence at lines 216-217 could be moved to become
the first sentence in the doc comment, but I'd rather not change
it now that the CCC has approved the doc changes as in webrev.03.

This does not change the spec and this is minor rewording.  No need to update 
CCC.

What about:

Shows all reflection frames.

By default, reflection frames are hidden.  A {@code StackWalker} with this 
{@code SHOW_REFLECT_FRAMES} option will show the reflection frames that include ….


Oh - you're right! I missed it.

I have made this change in my workspace:
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.05/

/**
 * Shows all reflection frames.
 *
 * By default, reflection frames are hidden.  A {@code 
StackWalker}

 * configured with this {@code SHOW_REFLECT_FRAMES} option
 * will show all reflection frames that
 * include {@link java.lang.reflect.Method#invoke} and
 * {@link java.lang.reflect.Constructor#newInstance(Object...)}
 * and their reflection implementation classes.

best regards,

-- daniel

Re: RFR: 8173607: JMX RMI connector should be in its own module

[Daniel Fuchs]

Hi Mandy,

On 02/02/17 02:41, Mandy Chung wrote:

http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.06


Does java.management still depend on java.base/jdk.internal.module?


Well spotted! It doesn't. I have updated module-info.java for java.base.

http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.07


http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.06/java.management.rmi-summary.html



Maybe the first sentence of @provides could be simplified to:

A provider of JMXConnectorProvider for the RMI protocol.
A provider of JMXConnectorServerProvider for the RMI protocol.


Done. But I think you meant A provider of JMXConnector[Server] for the 
RMI protocol.


http://cr.openjdk.java.net/~dfuchs/webrev_8173607/webrev.07/java.management.rmi-summary.html

best regards,

-- daniel



Otherwise looks good.
Mandy

Re: JDK 9: 8174128: [testbug] Remove implementation dependency from java.time TCK tests

[Daniel Fuchs]

Looks good!

-- daniel

On 07/02/17 20:06, Roger Riggs wrote:

Please review minor changes to the java.time.tck tests to avoid the use of
implementation details and the related opening of the modules.
The testng directive to open needed modules is now specific to the
java.time.test... tests.

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-opens-8174128/

Thanks, Roger

Re: RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

[Daniel Fuchs]

Hi Mandy,

Thanks for all the suggestions!

On 07/02/17 20:03, Mandy Chung wrote:

Please find below a new webrev that incorporates your feedback.
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.03/



I think this sentence is not needed.
 216  * A {@code StackWalker} with this {@code SHOW_REFLECT_FRAMES} 
option
 217  * will show all reflection frames.


I disagree: the text starts by saying:
"By default reflection frames are hidden. The reflection
 frames include [...]"
but the sentence at lines 216-217 is the only place where we say what
the option actually does: it shows all reflection frames.
I think it's needed.
Possibly the sentence at lines 216-217 could be moved to become
the first sentence in the doc comment, but I'd rather not change
it now that the CCC has approved the doc changes as in webrev.03.



Basic.java test - the @bug already includes the bug number.  line 67-69 can be 
removed.


Done.


Minor comments: StackFrame.getClassName() can replace 
StackFrame.getDeclaringClass().getName().  Or if you want to compare package 
name, StackFrame.getDeclaringClass().getPackageName is an alternative.


Oh - good remark. StackFrame.getClassName() it is.


Nit: what about renaming SimpleObj to ConstructorNewInstance?
SimpleObj::found should probably name as `framesInUnnamedOrReflectionPackage`.


SimpleObj => ConstructorNewInstance
found => testFramesOrReflectionFrames
 (frames in the unnamed package are test frames)

I added some comments as well in the accept() method.


Nit: ReflectionFrames.java test - the test is a bit hard maybe SimpleObj can be 
renamed to TestHelper and `found` be `result`.   Just trying to make the test 
easier to read and understand.  You may have better idea.


SimpleObj => StackInspector (it calls walk & getCallerClass
 to inspect the stack from its constructor)
found => collectedFrames

Also added some comments to better explain the purposes of
StackInspector and StackInspector.Caller.

Here is the new webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.04/

best regards,

-- daniel


Otherwise looks good.

Mandy

Re: RFR(s): 8174194: Several java/lang tests failing due to undeclared module dependencies.

[Daniel Fuchs]

Hi Sergey,

LFMultiThreadCachingTest.java

Can you clarify the change in this file?

jdk.management requires java.management - so if the test failed with
--limit-modules when it required jdk.management then it's not going to
pass if it is changed to require only java.management.

Or is it that jdk.management was actually not required and the
test was passing all right because it only required java.management?

The other files look OK, I guess.

best regards,

-- daniel

On 08/02/17 13:51, Sergei Kovalev wrote:

Hi Team,

Please review small fix for test suite regarding module dependency.

BugID: https://bugs.openjdk.java.net/browse/JDK-8174194
WebRev: http://cr.openjdk.java.net/~skovalev/8174194/webrev.00/

Re: RFR(s): 8174194: Several java/lang tests failing due to undeclared module dependencies.

[Daniel Fuchs]

On 08/02/17 14:13, Sergei Kovalev wrote:

Actually jdk.management is not required for this particular test.
java.management module is enough to pass the test.


Well, looks good to me then!

best regards,

-- daniel

Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Daniel Fuchs]

Hi Rob,

On 02/02/17 22:14, Rob McKenna wrote:

Thanks Daniel,

New webrev at http://cr.openjdk.java.net/~robm/8160768/webrev.04/

In addition to your comments below I've added the package access check
we discussed.

Note: when a provider returns multiple results we create an LdapCtx
based on the first of those. I think it might be useful to extend
LdapClient to accept a list of addresses which can be iterated over in the
event of a connection failure but I'll tackle that separately.


AFAICS the new webrev looks good to me.
I'm not too aware of the specifics of the JNDI LDAP bridge
though. I see that Vyom has taken a look at your patch and
suggested a change too and that is good :-)

best regards,

-- daniel



-Rob

On 02/02/17 11:01, Daniel Fuchs wrote:

Hi Rob,

This is not a code I know well - but here are a couple
of comments:

LdapCtxFactory.java:

168 NamingException ne = new NamingException();
232 NamingException ne = new NamingException();

Creating an exception is somewhat costly as it records the
backtrace in the Throwable constructor. I don't know if
it's an issue there but I thought I'd mention it. It might
be better to create the exception only if you're actually
going to throw it.

 186 } catch (Exception e) {
 187 ne.setRootCause(e);
 188 }

This will catch the NamingException thrown at line 173
and set it at the cause of another NamingException that
has no message. Maybe it would be better to have two
catch clauses:

 186 } catch (NamingException e) {
 187 throw e;
 186 } catch (Exception e) {
 ... transform it to NamingException ...

In getDnsUrls:

 198 if (env.containsKey(LdapCtx.DNS_PROVIDER)
 199 && env.get(LdapCtx.DNS_PROVIDER) != null
 200 && !env.get(LdapCtx.DNS_PROVIDER).equals(""))

I'd suggest to simplify this and make a single lookup:

 String providerClass = env.get(LdapCtx.DNS_PROVIDER);
 if (providerClass != null && !providerClass.isEmpty()) {
 ...
 Class cls = Class.forName(providerClass, true, cl);

best regards,

-- daniel


On 01/02/17 20:05, Rob McKenna wrote:

New webrev with updated test here:

http://cr.openjdk.java.net/~robm/8160768/webrev.03/

   -Rob

On 26/01/17 04:03, Rob McKenna wrote:

Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
again) That code was actually supposed to be in the getDnsUrls method.
Updated in place:

http://cr.openjdk.java.net/~robm/8160768/webrev.02/

I'll add a couple of tests for the SecurityManager along with some input
checking tests too.

   -Rob

On 25/01/17 05:50, Daniel Fuchs wrote:

Hi Rob,

I believe you should move the security check to before
the class is actually loaded, before the call to
171 List urls = getDnsUrls(url, env);

best regards,

-- daniel

On 25/01/17 17:44, Rob McKenna wrote:

I neglected to include a security check so I've cribbed the one from
OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:

http://cr.openjdk.java.net/~robm/8160768/webrev.02/

Note, this wraps the SecurityException in a NamingException. Presumably
its better to throw something than simply leave the default value in
place, but feedback appreciated!

  -Rob

On 25/01/17 04:31, Rob McKenna wrote:

Hi folks,

I'm looking for feedback on this suggested fix for the following bug:

https://bugs.openjdk.java.net/browse/JDK-816076
http://cr.openjdk.java.net/~robm/8160768/webrev.01/

This is something that has come up a few times. Basically in certain
environments (e.g. MS Active Directory) there is a dependence on
DNS records to provide a pointer to the actual ldap server to be used
for a given LdapCtx.PROVIDER_URL where the url itself simply points to the
domain name.

This fix add a new Ldap context property which allows a user to specify a
class (implementing BiFunction) which can perform any necessary extra steps
to derive the ldap servers hostname/port from the PROVIDER_URL.

  -Rob

Re: Ping: [JAXP] RFR: 8173602: TESTBUG: javax/xml/jaxp/unittest/transform/TransformerTest.java needs refactoring

[Daniel Fuchs]

Hi Christoph,

I had a look at your patch, it seems OK.
I didn't know that testng supported @Test
annotations on methods of inner classes, but
that looks useful.

The patch is a bit difficult to review because the diff
seems a bit lost sometime - but as far as I could see
what was there before seems to be there after too :-)

I noticed you changed the calls for pretty-printing
the results - using public APIs now. Hopefully that
doesn't matter to test (I mean that's hopefully not
what the test was testing).

I ran your new test locally on my machine, and sent it
through our test system as well, and everything passed,
so I guess that's a +1 for me.

Will you push that to the new 10 forest or did you
plan to push it in 9?

best regards,

-- daniel

On 03/02/17 10:11, Langer, Christoph wrote:

Hi,

Ping - anyone out there who could have a look at my JAXP test update?

Thanks
Christoph

From: Langer, Christoph
Sent: Montag, 30. Januar 2017 15:28
To: core-libs-dev@openjdk.java.net
Subject: [JAXP] RFR: 8173602: TESTBUG: 
javax/xml/jaxp/unittest/transform/TransformerTest.java needs refactoring

Hi,

please review a test fix for 
javax/xml/jaxp/unittest/transform/TransformerTest.java:
Bug: https://bugs.openjdk.java.net/browse/JDK-8173602
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8173602.0/

The main motivation for this refactoring is to remove the dependency to the JDK 
internal classes com.sun.org.apache.xml.internal.serialize.OutputFormat and 
com.sun.org.apache.xml.internal.serialize.XMLSerializer.

Generally, TransformerTest.java contains several subtests which do similar 
things (test XALAN transformations) but the implementations differ. I created a 
common template with helper methods which all tests take advantage of now. I 
also introduced data sources to some tests to make them more readable and 
adaptable.

Thanks
Christoph

Re: Ping: [JAXP] RFR: 8173602: TESTBUG: javax/xml/jaxp/unittest/transform/TransformerTest.java needs refactoring

[Daniel Fuchs]

Hi Christoph,

No objections.

best regards,

-- daniel

On 03/02/17 13:01, Langer, Christoph wrote:

Hi Daniel,

thanks for taking care of this.

As for the pretty printing: That's not something which is explicitly tested 
here. I think it was me who introduced this for new tests I had added. I found 
it quite useful when analyzing test results. So now with the public APIs I 
guess this code is more independent and portable to other Java versions.

I was only looking at JDK9 with that so far. My understanding was that pushing 
test fixes is still ok. Let me know if you have objections.

Best regards
Christoph


-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Freitag, 3. Februar 2017 13:54
To: Langer, Christoph <christoph.lan...@sap.com>; core-libs-
d...@openjdk.java.net
Subject: Re: Ping: [JAXP] RFR: 8173602: TESTBUG:
javax/xml/jaxp/unittest/transform/TransformerTest.java needs refactoring

Hi Christoph,

I had a look at your patch, it seems OK.
I didn't know that testng supported @Test
annotations on methods of inner classes, but
that looks useful.

The patch is a bit difficult to review because the diff
seems a bit lost sometime - but as far as I could see
what was there before seems to be there after too :-)

I noticed you changed the calls for pretty-printing
the results - using public APIs now. Hopefully that
doesn't matter to test (I mean that's hopefully not
what the test was testing).

I ran your new test locally on my machine, and sent it
through our test system as well, and everything passed,
so I guess that's a +1 for me.

Will you push that to the new 10 forest or did you
plan to push it in 9?

best regards,

-- daniel

On 03/02/17 10:11, Langer, Christoph wrote:

Hi,

Ping - anyone out there who could have a look at my JAXP test update?

Thanks
Christoph

From: Langer, Christoph
Sent: Montag, 30. Januar 2017 15:28
To: core-libs-dev@openjdk.java.net
Subject: [JAXP] RFR: 8173602: TESTBUG:

javax/xml/jaxp/unittest/transform/TransformerTest.java needs refactoring


Hi,

please review a test fix for

javax/xml/jaxp/unittest/transform/TransformerTest.java:

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

The main motivation for this refactoring is to remove the dependency to the

JDK internal classes com.sun.org.apache.xml.internal.serialize.OutputFormat
and com.sun.org.apache.xml.internal.serialize.XMLSerializer.


Generally, TransformerTest.java contains several subtests which do similar

things (test XALAN transformations) but the implementations differ. I created a
common template with helper methods which all tests take advantage of now. I
also introduced data sources to some tests to make them more readable and
adaptable.


Thanks
Christoph

Heads up: Fwd: hg: jdk9/dev/jdk: 8173607: JMX RMI connector should be in its own module

[Daniel Fuchs]

Hi,

A direct consequence of this change is that you may have
to either blow up your ./build directory, or run
'make clean-java' after pulling this fix.

Otherwise you may see strange build failures like below:

Error occurred during initialization of VM
java.lang.RuntimeException: Package com.sun.jmx.remote.protocol.rmi in 
both module java.management.rmi and module java.management
	at 
jdk.internal.module.ModuleBootstrap.fail(java.base/ModuleBootstrap.java:699)
	at 
jdk.internal.module.ModuleBootstrap.boot(java.base/ModuleBootstrap.java:329)

at java.lang.System.initPhase2(java.base/System.java:1928)


best regards,

-- daniel
--- Begin Message ---
Changeset: db6e995edd0a
Author:dfuchs
Date:  2017-02-02 16:50 +
URL:   http://hg.openjdk.java.net/jdk9/dev/jdk/rev/db6e995edd0a

8173607: JMX RMI connector should be in its own module
Summary: The JMX RMI connector is moved to a new java.management.rmi module.
Reviewed-by: mchung, erikj

- make/rmic/Rmic-java.management.gmk
+ make/rmic/Rmic-java.management.rmi.gmk
! src/java.base/share/classes/module-info.java
+ 
src/java.management.rmi/share/classes/com/sun/jmx/remote/internal/rmi/ProxyRef.java
+ 
src/java.management.rmi/share/classes/com/sun/jmx/remote/internal/rmi/RMIExporter.java
+ 
src/java.management.rmi/share/classes/com/sun/jmx/remote/internal/rmi/Unmarshal.java
+ 
src/java.management.rmi/share/classes/com/sun/jmx/remote/protocol/rmi/ClientProvider.java
+ 
src/java.management.rmi/share/classes/com/sun/jmx/remote/protocol/rmi/ServerProvider.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/NoCallStackClassLoader.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnector.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectorServer.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIIIOPServerImpl.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIJRMPServerImpl.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIServer.java
+ 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIServerImpl.java
+ src/java.management.rmi/share/classes/javax/management/remote/rmi/package.html
+ src/java.management.rmi/share/classes/module-info.java
! 
src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java
- src/java.management/share/classes/com/sun/jmx/remote/internal/ProxyRef.java
- src/java.management/share/classes/com/sun/jmx/remote/internal/RMIExporter.java
- src/java.management/share/classes/com/sun/jmx/remote/internal/Unmarshal.java
- 
src/java.management/share/classes/com/sun/jmx/remote/protocol/rmi/ClientProvider.java
- 
src/java.management/share/classes/com/sun/jmx/remote/protocol/rmi/ServerProvider.java
! 
src/java.management/share/classes/javax/management/remote/JMXConnectorFactory.java
! 
src/java.management/share/classes/javax/management/remote/JMXConnectorServerFactory.java
- 
src/java.management/share/classes/javax/management/remote/rmi/NoCallStackClassLoader.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIConnection.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIConnector.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIConnectorServer.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIIIOPServerImpl.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIJRMPServerImpl.java
- src/java.management/share/classes/javax/management/remote/rmi/RMIServer.java
- 
src/java.management/share/classes/javax/management/remote/rmi/RMIServerImpl.java
- src/java.management/share/classes/javax/management/remote/rmi/package.html
! src/java.management/share/classes/module-info.java
! src/java.rmi/share/classes/module-info.java
! src/java.se/share/classes/module-info.java
! src/jdk.jconsole/share/classes/module-info.java
! src/jdk.management.agent/share/classes/module-info.java
! 
src/jdk.management.agent/share/classes/sun/management/jmxremote/ConnectorBootstrap.java
! test/javax/management/MBeanInfo/NotificationInfoTest.java
! test/javax/management/MBeanServer/ExceptionTest.java
! test/javax/management/MBeanServer/OldMBeanServerTest.java
! test/javax/management/modelmbean/UnserializableTargetObjectTest.java
! test/javax/management/mxbean/GenericArrayTypeTest.java
! test/javax/management/mxbean/MXBeanExceptionHandlingTest.java
! test/javax/management/mxbean/MXBeanInteropTest1.java
! test/javax/management/mxbean/MXBeanInteropTest2.java
! test/javax/management/mxbean/MXBeanNotifTest.java
! test/javax/management/mxbean/MXBeanTest.java
! 

Re: RFR 8173159, Problem list java/rmi/activation/ActivationGroup/downloadActivationGroup/DownloadActivationGroup.java on Windows

[Daniel Fuchs]

+1

-- daniel

On 21/01/17 05:43, Felix Yang wrote:

Hi there,
   please review the  request to problem-list the test on Windows platform. It 
has been observed to be failing frequently.

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


Thanks,
Felix


diff -r 1045f9722697 test/ProblemList.txt
--- a/test/ProblemList.txt  Sat Jan 21 03:53:21 2017 +
+++ b/test/ProblemList.txt  Fri Jan 20 21:15:34 2017 -0800
@@ -228,6 +228,8 @@

 java/rmi/activation/Activatable/extLoadedImpl/ext.sh8062724 
generic-all

+java/rmi/activation/ActivationGroup/downloadActivationGroup/DownloadActivationGroup.java
 8169569 windows-all
+
 sun/rmi/rmic/newrmic/equivalence/run.sh 8145980 
generic-all

 

Re: RFR (JAXP) 8169827: javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed

[Daniel Fuchs]

Hi Christoph,

Thanks for fixing this test!

I imported your patch, modified ProblemList.txt to not skip the test,
and sent it through our test system, and I'm happy to report that
the test was run on all available platforms with no failure.

So I think you should simply remove the line from ProblemList.txt
(no need for a new webrev).
If it fails again on more exotic platform we'll simply add it
back to ProblemList.txt for those platforms where it fails
(I guess it could happen if there's not enough disk space).

Otherwise I have looked over the changes you proposed and they
do seem OK to me.

+1

best regards,

-- daniel

On 23/01/17 10:03, Langer, Christoph wrote:

Hi,

while working on jaxp changes and running jtreg tests I found that test 
javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh does not work. I then saw 
that this was already reported with bug 8169827. But, as I had already spent 
some time to fix this test I'd like to contribute my fix:

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

I converted the test to Java and removed the shell script PropertiesTest.sh. 
This resolves the compilation issue.

However, the test needs to copy an isolated jdk as it modifies files in the 
JDK. So I'm still using the copy script to first copy the jdk and afterwards 
remove the copy. These are separate 'shell' test steps. And in the actual test 
I'm running a child process with the isolated JDK.

I also don't know if the test should be kept in the problem list and/or also be 
tagged as 'intermittent' as the whole jdk copying procedure by means of a shell 
script seems error prone. In case we keep the entry in the problem list, I can 
also open a separate bug for my change.

@Frank: I don't know if you have some larger change in mind which improves the 
isolated jdk type testing greatly, otherwise I think this fix could at least 
make things better than they are at the moment.

Thanks & Best regards
Christoph

Re: (JAXP) RFR: 8173111: Excessive recursion in EventFilterSupport when filtering over large number of XML events can cause StackOverflow

[Daniel Fuchs]

Hi,

Thanks for all the feedback.

I have updated the webrev
http://cr.openjdk.java.net/~dfuchs/webrev_8173111/webrev.01

The test now uses assertEquals as suggested by Lance and Aleksej
(thanks for the suggestion).
I also fixed the missing white space in the code that Christoph
spotted at line 67.
I kept the main method (useful when you tweak the test in
NetBeans).

I'm going to launch it again through our test system and if
successful I'll consider it reviewed and push it, unless I
hear otherwise by then :-)

best regards,

-- daniel



On 23/01/17 17:48, Daniel Fuchs wrote:

Hi,

Please find below a fix for:

8173111: Excessive recursion in EventFilterSupport when filtering
 over large number of XML events can cause StackOverflow
https://bugs.openjdk.java.net/browse/JDK-8173111

The fix is almost trivial: it replaces a recursion by a loop in
two places.

http://cr.openjdk.java.net/~dfuchs/webrev_8173111/webrev.00/

The test will fail without the fix (at least on my machine)
and pass with it (on my machine and in our test system).

best regards,

-- daniel

[JAXP] RFR: 8173260: CatalogManager.catalogResolver should not fail when non-existing URI is passed to it

[Daniel Fuchs]

Hi,

Please find below a fix for

8173260: CatalogManager.catalogResolver should not fail
 when non-existing URI is passed to it
https://bugs.openjdk.java.net/browse/JDK-8173260

The specification for CatalogManager.catalogResolver and
CatalogManager.catalog says that invalid catalog entries
will be ignored.

However, this is not what happens when a catalog is created
with an URI that points to a file (or jar) which does not exist.
In that case, instead of ignoring the catalog entry,
an IllegalArgumentException is thrown, even if the URL itself is
valid.

This fix proposes to simply remove the special case that
the implementation makes for file or jar URLs.

test/javax/xml/jaxp/unittest/catalog/CatalogFileInputTest.java
is updated to reflect this change.

http://cr.openjdk.java.net/~dfuchs/webrev_8173260/webrev.01/

best regards,

-- daniel

Re: RFR 8170116: Remove qualified exports from java.base to java.corba

[Daniel Fuchs]

On 25/01/17 12:32, Alan Bateman wrote:



On 25/01/2017 11:05, Pavel Rappo wrote:

Hello,

Could you please review the following change for [1]?

http://cr.openjdk.java.net/~prappo/8170116/webrev.00/



Hi Alan,


I agree with Daniel on the name.

Also the comment in the Bridge constructor says "Bridge.get() is always
called in doPrivileged() blocks" which is at odds with the permission
check in Bridge.get. That is, if Bridge.get is always called with
privileges then the permission check is not needed.


The comment was intended to explain that doPrivileged is not needed
in new Bridge() because the code that calls Bridge.get() (which in
turn creates new Bridge) uses doPrivileged to call Bridge.get()).
Otherwise creating the StackWalker in new Bridge might have caused
a new SecurityException to be propagated to user code at the time
bridge creation was triggered.

But maybe the comment should be removed if it's misleading.

cheers,

-- daniel



-Alan.

Re: RFR 8170116: Remove qualified exports from java.base to java.corba

[Daniel Fuchs]

Hi Pavel,

Looks good to me.
I might have kept the 'defaultPresentationManager' name though.
Even though it's global it feels like a better name than 'globalPM'.

best regards,

-- daniel

On 25/01/17 11:05, Pavel Rappo wrote:

Hello,

Could you please review the following change for [1]?

   http://cr.openjdk.java.net/~prappo/8170116/webrev.00/

Thanks,
-Pavel


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

Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Daniel Fuchs]

Hi Rob,

I believe you should move the security check to before
the class is actually loaded, before the call to
 171 List urls = getDnsUrls(url, env);

best regards,

-- daniel

On 25/01/17 17:44, Rob McKenna wrote:

I neglected to include a security check so I've cribbed the one from
OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:

http://cr.openjdk.java.net/~robm/8160768/webrev.02/

Note, this wraps the SecurityException in a NamingException. Presumably
its better to throw something than simply leave the default value in
place, but feedback appreciated!

-Rob

On 25/01/17 04:31, Rob McKenna wrote:

Hi folks,

I'm looking for feedback on this suggested fix for the following bug:

https://bugs.openjdk.java.net/browse/JDK-816076
http://cr.openjdk.java.net/~robm/8160768/webrev.01/

This is something that has come up a few times. Basically in certain
environments (e.g. MS Active Directory) there is a dependence on
DNS records to provide a pointer to the actual ldap server to be used
for a given LdapCtx.PROVIDER_URL where the url itself simply points to the
domain name.

This fix add a new Ldap context property which allows a user to specify a
class (implementing BiFunction) which can perform any necessary extra steps
to derive the ldap servers hostname/port from the PROVIDER_URL.

-Rob

Re: Soften interface for javax.management.ObjectName.getInstance and friends

[Daniel Fuchs]

Hi Dave,

I'm not sure this is quite a good idea because
order doesn't count when comparing ObjectNames.

So the folder analogy would just be a lure:
/a/b/c would be equal to /a/c/b

That said - I can see value in trying to get rid
of the legacy Hashtable - so adding a new method
that takes a Map wouldn't necessarily
be a bad thing :-)

A work around for your use case would be to use:

 static ObjectName getInstance(String name)

instead of

 static ObjectName getInstance(String domain
   Hashtable table)

An object name has both a string representation
and a canonical representation.
IIRC we did try to preserve the original string
representation, even if it's not canonical.
It's also preserved in the serial form.

See:
https://docs.oracle.com/javase/8/docs/api/javax/management/ObjectName.html#getKeyPropertyListString--

Hope this helps,

-- daniel


On 24/02/17 00:17, Dave Brosius wrote:

Greetings. the method

public static ObjectName getInstance(String domain,
Hashtable table)
throws MalformedObjectNameException {
return new ObjectName(domain, table);
}

in javax.management.ObjectName allows for a provided Hashtable to
specify properties for the objectname.

The semantics of an ObjectName don't consider the order of these
properties, however certain tools like jconsole (when used as a name for
a jmx property) does consider the order.

If you wish to create a folder structure to report metrics in jmx, you
need to use this properties map to specify the folder names. JConsole,
then, uses order of iteration to determine the order of the folder
hierarchy.

Suppose you want a folder hierarchy similar to a package name, you may
specify properties like

table.put("a0", "com");
table.put("a1", "acme");
table.put("name", "MyMetric");

in hopes of producing a metric in JConsole in the folder structure,
com/acme/MyMetric.

The problem is of course, that the argument is a Hashtable, not a Map,
and so the items are not ordered at all, yet JConsole uses iteration
order to build the path, so you may get

acme/ao/MyMetric or MyMetric/acme/ao or .

This means if you really want to have ordered packages, you have to
derive from Hashtable, and override the entrySet() method, including
that set's iterator() to return the values in the order you wish to have
them shown.

That is really janky.

I'm proposing that the interface for getInstance be softened to

public static ObjectName getInstance(String domain,
 Map table)

as well as

public ObjectName(String domain, Map table)

thoughts?

Re: RFR (JAXP) 8169450: StAX parse error if there is a newline in xml declaration

[Daniel Fuchs]

On 14/02/17 17:21, huizhe wang wrote:

Thanks!

Here's an updated webrev:
http://cr.openjdk.java.net/~joehw/jdk9/8169450/webrev/


+1

-- daniel



-Joe

On 2/14/2017 4:07 AM, Lance Andersen wrote:

Looks good overall Joe.  I would agree that I would clean up the minor
comment alignment issues.

Best
Lance

On Feb 13, 2017, at 9:27 PM, huizhe wang > wrote:

A quick fix for the error parsing xml declaration. This is one of the
three outstanding issues in the xml area that must be addressed for
the coming development deadline.

JBS: https://bugs.openjdk.java.net/browse/JDK-8169450
webrev: http://cr.openjdk.java.net/~joehw/jdk9/8169450/webrev/


Checking whether a space follows "




Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR (JAXP) 8169450: StAX parse error if there is a newline in xml declaration

[Daniel Fuchs]

Looks good Joe.

I wonder about this though (which may be an issue for
another time):

102 [25] Eq ::= S? '=' S?

Do we support space (new line?) before and after the '=' sign?

best regards,

-- daniel

On 14/02/17 02:27, huizhe wang wrote:

A quick fix for the error parsing xml declaration. This is one of the
three outstanding issues in the xml area that must be addressed for the
coming development deadline.

JBS: https://bugs.openjdk.java.net/browse/JDK-8169450
webrev: http://cr.openjdk.java.net/~joehw/jdk9/8169450/webrev/

Checking whether a space follows "

Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by JDK-8087303

[Daniel Fuchs]

Hi Frank,

Should you skip '\r' if it's not followed by '\n'?


best regards,

-- daniel

On 14/02/17 10:33, Frank Yuan wrote:

Hi Joe

As you suggested, I made pretty-print a little better based on the fix. That is 
when adding indentation, just check the beginning
character(s), in case of '\n' or '\r' then, ignore it/them.

Please check the new webrev: 
http://cr.openjdk.java.net/~fyuan/8174025/webrev.01/


Thanks
Frank

-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Subject: should have been 8174025 -> Re: RFR [JAXP] JDK-8170192 Regression in 
XML Transform caused by JDK-8087303

Note that the bug id was incorrect, it should have been 8174025. 8170192
was a test bug fix.

-Joe

On 2/13/2017 1:35 AM, Frank Yuan wrote:

Hi Joe and Daniel

Thank you very much for your review!

Frank


-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR [JAXP] JDK-8170192 Regression in XML Transform caused by 
JDK-8087303

+1 from me too.

Thanks,
Joe

On 2/10/2017 5:25 AM, Daniel Fuchs wrote:

Hi Frank,

Thanks for fixing this!

I imported your patch and played with it a bit.
Also ran the jaxp test.

Both issues reported have indeed disappeared.

So that's a +1 from me.

best regards,

-- daniel

On 10/02/17 11:03, Frank Yuan wrote:

Hi All



Would you like to review
http://cr.openjdk.java.net/~fyuan/8174025/webrev.00/?

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



JDK-8087303 introduced 2 issues:

1.   Flaw when xlst uses disable-output-escaping attribute

2.   Eat the whitespace between html inline elements



This patch fixed the issues.

To fix the second issue, we decide to keep the compatibility with JDK 8
on the whitespace handling, that is only LSSerializer cleans the extra
whitespaces in if pretty-print is on, but XSLT doesn't.

I modified the behavior of getIndent() method in class ToStream, to make
LSSerializer be sensitive of current state from ToStream. This should be
safe because ToStream is an internal class and getIndent() method is
never used before.



Thanks

Frank

Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by JDK-8087303

[Daniel Fuchs]

Hi Frank,

On 14/02/17 13:43, Frank Yuan wrote:



-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Subject: Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by 
JDK-8087303

Hi Frank,

Should you skip '\r' if it's not followed by '\n'?


Well - I'll let Joe answer that. ;-)
It was just a question, I was wondering whether that could
potentially cause problems down the road - since new lines
are usually only either '\n' or '\r'+'\n'.

Your patch looks fine otherwise, maybe the code that skips
the '\n' could be factorized somewhere to avoid duplication,
but that's not really important.

Both issues reported in the bug are still fix - so I think we
should try to get this patch in as soon as we can.

best regards,

-- daniel




Does it matter? Since XML processor should normalize the newline.

Thanks
Frank


best regards,

-- daniel

On 14/02/17 10:33, Frank Yuan wrote:

Hi Joe

As you suggested, I made pretty-print a little better based on the fix. That is 
when adding indentation, just check the

beginning

character(s), in case of '\n' or '\r' then, ignore it/them.

Please check the new webrev: 
http://cr.openjdk.java.net/~fyuan/8174025/webrev.01/


Thanks
Frank

-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Subject: should have been 8174025 -> Re: RFR [JAXP] JDK-8170192 Regression in 
XML Transform caused by JDK-8087303

Note that the bug id was incorrect, it should have been 8174025. 8170192
was a test bug fix.

-Joe

On 2/13/2017 1:35 AM, Frank Yuan wrote:

Hi Joe and Daniel

Thank you very much for your review!

Frank


-Original Message-
From: huizhe wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR [JAXP] JDK-8170192 Regression in XML Transform caused by 
JDK-8087303

+1 from me too.

Thanks,
Joe

On 2/10/2017 5:25 AM, Daniel Fuchs wrote:

Hi Frank,

Thanks for fixing this!

I imported your patch and played with it a bit.
Also ran the jaxp test.

Both issues reported have indeed disappeared.

So that's a +1 from me.

best regards,

-- daniel

On 10/02/17 11:03, Frank Yuan wrote:

Hi All



Would you like to review
http://cr.openjdk.java.net/~fyuan/8174025/webrev.00/?

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



JDK-8087303 introduced 2 issues:

1.   Flaw when xlst uses disable-output-escaping attribute

2.   Eat the whitespace between html inline elements



This patch fixed the issues.

To fix the second issue, we decide to keep the compatibility with JDK 8
on the whitespace handling, that is only LSSerializer cleans the extra
whitespaces in if pretty-print is on, but XSLT doesn't.

I modified the behavior of getIndent() method in class ToStream, to make
LSSerializer be sensitive of current state from ToStream. This should be
safe because ToStream is an internal class and getIndent() method is
never used before.



Thanks

Frank

Re: RFR [JAXP] JDK-8170192 Regression in XML Transform caused by JDK-8087303

[Daniel Fuchs]

Hi Frank,

Thanks for fixing this!

I imported your patch and played with it a bit.
Also ran the jaxp test.

Both issues reported have indeed disappeared.

So that's a +1 from me.

best regards,

-- daniel

On 10/02/17 11:03, Frank Yuan wrote:

Hi All



Would you like to review
http://cr.openjdk.java.net/~fyuan/8174025/webrev.00/?

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



JDK-8087303 introduced 2 issues:

1.   Flaw when xlst uses disable-output-escaping attribute

2.   Eat the whitespace between html inline elements



This patch fixed the issues.

To fix the second issue, we decide to keep the compatibility with JDK 8
on the whitespace handling, that is only LSSerializer cleans the extra
whitespaces in if pretty-print is on, but XSLT doesn’t.

I modified the behavior of getIndent() method in class ToStream, to make
LSSerializer be sensitive of current state from ToStream. This should be
safe because ToStream is an internal class and getIndent() method is
never used before.



Thanks

Frank

Re: RFR: 8174735 Update JAX-WS RI integration to latest version

[Daniel Fuchs]

Hi Aleksej,

On 15/02/17 23:49, Aleks Efimov wrote:

Hi,

The new webrev with addressed comments was uploaded here:
http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01


This is probably a question for the upstream project, but I'm
puzzled by this change:

http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/jdk.xml.bind/share/classes/com/sun/codemodel/internal/JModuleDirective.java.frames.html

... and even more since Integer.hashCode() calls Integer.hashCode(int) 
which just returns the value of the integer...

What's the purpose of calling valueOf which 1. contradict the javadoc
comment and 2. might trigger the creation of a new Integer instance?


Nit: There are also some files which present some strange
 formatting/misalignment where new code/comment was added.
 I have noted them below:

There's strange formatting for the anonymous subclass
of AbstractList in this file:

http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/packaging/mime/internet/InternetHeaders.java.frames.html

 324 headerValueView = new AbstractList() {
 325 @Override
 326 public String get(int index) {
 327 return headers.get(index).line;
 328 }
 329
 330 @Override
 331 public int size() {
 332 return headers.size();
 333 }
 334 };

and
http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/packaging/mime/internet/MimePartDataSource.java.frames.html

has also some alignment issues where @Overrides were added.

The following file has some comment alignment issues which impair
readability:

http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/packaging/mime/util/ASCIIUtility.java.frames.html

best regards,

-- daniel



Best,
Aleksej


On 15/02/17 15:42, Roman Grigoriadi wrote:

Hi Mandy,

On 02/14/2017 11:53 PM, Mandy Chung wrote:

On Feb 14, 2017, at 4:00 AM, Roman Grigoriadi
 wrote:

Hi,

Please review standalone JAXB/JAXWS changes, synced to jdk/jaxws repo.

JBS: https://bugs.openjdk.java.net/browse/JDK-8174735
Webrev:
http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/00/


jaxws/src/jdk.xml.ws/share/classes/com/sun/tools/internal/ws/wscompile/WsimportTool.java


-/** JAXWS module name. JAXWS dependency is mandatory in
generated Java module. */
-private static final String JAXWS_MODULE = "java.xml.ws";
+/** JAXB module name. JAXB dependency is mandatory in generated
Java module. */
+private static final String JAXWS_PACKAGE = "java.xml.ws”;

this looks to be merge failure on our side, will fix it again.


JAXWS_MODULE is the right name as we discussed in the last JAX-WS
integration to jdk9. This change should be reverted and the upstream
project  should be fixed.

+if ("-release".equals(opt) && 9 >=
getVersion(javacOptions.get(i + 1))) {

thanks, will be fixed to --release


javac supports `—-release` (double dashes, GNU long form style) but
not the single dash option.  Is this new option in wsimport and wsgen
tools?  It should probably be consistent with javac.

You can run jdeps —-check java.base,java.xml option to double check
if any remaining qualified exports to these modules.

Otherwise looks okay.

Mandy

Re: rmid on Unix fails with Exception - maybe aftermath of JDK-8173607 ??

[Daniel Fuchs]

Hi Christoph,

It looks like one of the dreaded class initialization cycle
issues.

If you look at the stack trace, you will see that
UnixNativeDispatcher.:609
calls System.loadLibrary at line 611
which later down the road calls the
native UnixNativeDispatcher.getcwd command
from sun.nio.fs.UnixFileSystem.

But at this point UnixNativeDispatcher has not
finished initializing: we're still trying to load
the nio lib...

I don't think this has anything to do with JDK-8173607.
JDK-8173607 just splitted the JMX RMI Connector out of
java.management - and rmid has nothing to do with JMX.

rmid doesn't depend on either java.management or
java.management.rmi, and I don't see anything
on the exception path that would involve either
of these modules.
So I suspect the culprit is probably elsewhere.

best regards,

-- daniel



On 16/02/17 13:53, Langer, Christoph wrote:

Hi Daniel,



when starting „rmid“ from current builds on Linux (and probably other
Unix platforms as well) we are currently getting the exception below.



It looks as if on that path libnio was not loaded and hence we’re
getting an UnsatisfiedLinkError. In UnixNativeDispatcher.java one can
find a static initialization block which should do that, however, it
needs elevated access. Maybe rmid is missing a permission for that?



Any thoughts?



Thanks & best regards

Christoph





...>/jdk9-build-dir/images/jdk/bin/rmid

Exception in thread "main" java.lang.UnsatisfiedLinkError:
sun.nio.fs.UnixNativeDispatcher.getcwd()[B

at java.base/sun.nio.fs.UnixNativeDispatcher.getcwd(Native Method)

at
java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:66)

at
java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39)

at
java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:46)

at
java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:39)

at
java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.java:56)

at
java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvider.java:41)

at
java.base/sun.nio.fs.DefaultFileSystemProvider.create(DefaultFileSystemProvider.java:41)

at
java.base/java.io.FilePermission.(FilePermission.java:206)

at
java.base/java.lang.SecurityManager.checkRead(SecurityManager.java:899)

at java.base/java.io.File.exists(File.java:815)

at java.base/java.lang.ClassLoader$2.run(ClassLoader.java:2513)

at java.base/java.lang.ClassLoader$2.run(ClassLoader.java:2510)

at java.base/java.security.AccessController.doPrivileged(Native
Method)

at
java.base/java.lang.ClassLoader.loadLibrary0(ClassLoader.java:2509)

at
java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2478)

at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:873)

at java.base/java.lang.System.loadLibrary(System.java:1818)

at
java.base/sun.nio.fs.UnixNativeDispatcher$1.run(UnixNativeDispatcher.java:611)

at
java.base/sun.nio.fs.UnixNativeDispatcher$1.run(UnixNativeDispatcher.java:609)

at java.base/java.security.AccessController.doPrivileged(Native
Method)

at
java.base/sun.nio.fs.UnixNativeDispatcher.(UnixNativeDispatcher.java:609)

at
java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:66)

at
java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39)

at
java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:46)

at
java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:39)

at
java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.java:56)

at
java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvider.java:41)

at
java.base/sun.nio.fs.DefaultFileSystemProvider.create(DefaultFileSystemProvider.java:41)

at java.base/java.nio.file.FileSystems.(FileSystems.java:91)

at java.base/java.nio.file.Paths.get(Paths.java:84)

at
java.base/jdk.internal.jimage.ImageReaderFactory.(ImageReaderFactory.java:50)

at java.base/java.security.AccessController.doPrivileged(Native
Method)

at
java.base/sun.net.www.protocol.jrt.JavaRuntimeURLConnection.(JavaRuntimeURLConnection.java:59)

at
java.base/sun.net.www.protocol.jrt.Handler.openConnection(Handler.java:42)

at java.base/java.net.URL.openConnection(URL.java:1049)

at
java.base/jdk.internal.module.SystemModuleFinder$ImageModuleReader.checkPermissionToConnect(SystemModuleFinder.java:287)

at
java.base/jdk.internal.module.SystemModuleFinder$ImageModuleReader.(SystemModuleFinder.java:296)

at
java.base/jdk.internal.module.SystemModuleFinder$1.get(SystemModuleFinder.java:245)

at
java.base/jdk.internal.module.SystemModuleFinder$1.get(SystemModuleFinder.java:242)

at

Re: rmid on Unix fails with Exception - maybe aftermath of JDK-8173607 ??

[Daniel Fuchs]

On 16/02/17 15:22, Langer, Christoph wrote:

Hi Daniel,

thanks for your hints - I should probably have seen that myself if I would have 
had a more thorough look...

I'm cc-ing nio-dev, maybe somebody has an idea. I probably won't be able to 
analyze this short term.


No problem. Do you know when that started showing up?

best regards,

-- daniel



Best regards
Christoph


-Original Message-
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Sent: Donnerstag, 16. Februar 2017 15:53
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: Zeller, Arno <arno.zel...@sap.com>; Baesken, Matthias
<matthias.baes...@sap.com>; core-libs-dev@openjdk.java.net
Subject: Re: rmid on Unix fails with Exception - maybe aftermath of JDK-
8173607 ??

Hi Christoph,

It looks like one of the dreaded class initialization cycle
issues.

If you look at the stack trace, you will see that
UnixNativeDispatcher.:609
calls System.loadLibrary at line 611
which later down the road calls the
native UnixNativeDispatcher.getcwd command
from sun.nio.fs.UnixFileSystem.

But at this point UnixNativeDispatcher has not
finished initializing: we're still trying to load
the nio lib...

I don't think this has anything to do with JDK-8173607.
JDK-8173607 just splitted the JMX RMI Connector out of
java.management - and rmid has nothing to do with JMX.

rmid doesn't depend on either java.management or
java.management.rmi, and I don't see anything
on the exception path that would involve either
of these modules.
So I suspect the culprit is probably elsewhere.

best regards,

-- daniel



On 16/02/17 13:53, Langer, Christoph wrote:

Hi Daniel,



when starting "rmid" from current builds on Linux (and probably other
Unix platforms as well) we are currently getting the exception below.



It looks as if on that path libnio was not loaded and hence we're
getting an UnsatisfiedLinkError. In UnixNativeDispatcher.java one can
find a static initialization block which should do that, however, it
needs elevated access. Maybe rmid is missing a permission for that?



Any thoughts?



Thanks & best regards

Christoph





...>/jdk9-build-dir/images/jdk/bin/rmid

Exception in thread "main" java.lang.UnsatisfiedLinkError:
sun.nio.fs.UnixNativeDispatcher.getcwd()[B

at java.base/sun.nio.fs.UnixNativeDispatcher.getcwd(Native Method)

at
java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:66)

at
java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39)

at


java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSyste
mProvider.java:46)


at


java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSyste
mProvider.java:39)


at


java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.
java:56)


at


java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvide
r.java:41)


at


java.base/sun.nio.fs.DefaultFileSystemProvider.create(DefaultFileSystemPr
ovider.java:41)


at
java.base/java.io.FilePermission.(FilePermission.java:206)

at


java.base/java.lang.SecurityManager.checkRead(SecurityManager.java:899)


at java.base/java.io.File.exists(File.java:815)

at java.base/java.lang.ClassLoader$2.run(ClassLoader.java:2513)

at java.base/java.lang.ClassLoader$2.run(ClassLoader.java:2510)

at java.base/java.security.AccessController.doPrivileged(Native
Method)

at
java.base/java.lang.ClassLoader.loadLibrary0(ClassLoader.java:2509)

at
java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2478)

at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:873)

at java.base/java.lang.System.loadLibrary(System.java:1818)

at


java.base/sun.nio.fs.UnixNativeDispatcher$1.run(UnixNativeDispatcher.java
:611)


at


java.base/sun.nio.fs.UnixNativeDispatcher$1.run(UnixNativeDispatcher.java
:609)


at java.base/java.security.AccessController.doPrivileged(Native
Method)

at


java.base/sun.nio.fs.UnixNativeDispatcher.(UnixNativeDispatcher.jav
a:609)


at
java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:66)

at
java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39)

at


java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSyste
mProvider.java:46)


at


java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSyste
mProvider.java:39)


at


java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.
java:56)


at


java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvide
r.java:41)


at


java.base/sun.nio.fs.DefaultFileSystemProvider.create(DefaultFileSystemPr
ovider.java:41)


at java.base/java.nio.file.FileSystems.(FileSystems.java:91)

at java.base/java.nio.file.Paths.get(Paths.java:84)

at


java.base/jdk.internal.jimage.ImageReaderFactory.(ImageR

Re: [9] RFR: 8173390: Investigate SymbolTable in SAXParser

[Daniel Fuchs]

Hi Aleksej,

In the test:

  75 Assert.assertNotEquals(symTable1, symTable2, "Symbol table 
refence is the same");


I guess you meant Assert.assertNotSame - if what you want to do
is compare references.
(+ there is a typo in the message: refence => reference)

best regards,

-- daniel

On 15/02/17 16:31, Aleks Efimov wrote:

Hi,

Please, help to review the change required by JAXWS-RI code [1]:
SAXParser needs to reset internal SymbolTable to enable pooling of
parsers in SAAJ-RI code. Latest version of JAXWS-RI code (that is
currently under review [2]) doesn't provide a workaround to reset the
symbol table (it was done to remove module dependency on Xerces internal
classes). So the solution is to reset the SymbolTable during SAXParser
reset. The webrev with the changes and simple test that reproduces
SAAJ-RI behavior:
http://cr.openjdk.java.net/~aefimov/8173390/9/00/

The fix was tested with all JDK/JCK (JAX[P|B|WS] related tests. No
failures observed.


Thank you,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8173390
[2]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-February/046386.html

(JAXP) RFR: 8173111: Excessive recursion in EventFilterSupport when filtering over large number of XML events can cause StackOverflow

[Daniel Fuchs]

Hi,

Please find below a fix for:

8173111: Excessive recursion in EventFilterSupport when filtering
 over large number of XML events can cause StackOverflow
https://bugs.openjdk.java.net/browse/JDK-8173111

The fix is almost trivial: it replaces a recursion by a loop in
two places.

http://cr.openjdk.java.net/~dfuchs/webrev_8173111/webrev.00/

The test will fail without the fix (at least on my machine)
and pass with it (on my machine and in our test system).

best regards,

-- daniel

Re: RFR 8172350: Typo in Timestamp.toString()

[Daniel Fuchs]

Hi Lance,

On 19/01/17 15:20, Lance Andersen wrote:

Hi all,

Following trivial javadoc update to Timestamp.toString() needs a review


Looks good!

-- daniel



—
ljanders-mac:jdk ljanders$ hg diff
diff -r 051e7d9159a7 src/java.sql/share/classes/java/sql/Timestamp.java
--- a/src/java.sql/share/classes/java/sql/Timestamp.javaTue Jan 17 
11:34:47 2017 -0800
+++ b/src/java.sql/share/classes/java/sql/Timestamp.javaThu Jan 19 
10:07:23 2017 -0500
@@ -259,7 +259,7 @@
 /**
  * Formats a timestamp in JDBC timestamp escape format.
  * {@code -mm-dd hh:mm:ss.f},
- * where {@code ff} indicates nanoseconds.
+ * where {@code f} indicates nanoseconds.
  *
  * @return a {@code String} object in
  *   {@code -mm-dd hh:mm:ss.f} format
ljanders-mac:jdk ljanders$
—

Best
Lance
 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

8172971: java.management could use System.Logger

[Daniel Fuchs]

Hi,

Please find below a patch for:

8172971: java.management could use System.Logger
https://bugs.openjdk.java.net/browse/JDK-8172971

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8172971/webrev.00/

I have also added a new test:
test/sun/management/LoggingTest/LoggingTest.java

This is a simple smoke test that verifies that we still
get expected debug traces when creating and populating
the platform MBeanServer.
This test should always pass - whether the fix is present
or not.

best regards,

-- daniel

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Daniel Fuchs]

On 17/01/17 18:23, Naoto Sato wrote:

Thanks, Peter & Daniel.

I modified the webrevs accordingly:

http://cr.openjdk.java.net/~naoto/8171139/webrev.06/
http://cr.openjdk.java.net/~naoto/8171140/webrev.01/


+1

best regards,

-- daniel



The one for 8171139 is the same as Peter's latest one, and the latter
one for 8171140 was modified following that change.

Naoto

On 1/17/17 4:24 AM, Daniel Fuchs wrote:

+1

With Peter's proposed changes if I'm not mistaken then all methods
that operate on the CacheKey (findBundle, findBundleInCache,
putBundleInCache, loadBundle, loadBundleFromProvider) are all
called from a point originating from within the block now protected
by the reachability fences, so there should be no opportunity for the
referents of our KeyElementReference to become null while the
CacheKey is used.

Thanks Peter!

-- daniel

On 17/01/17 11:53, Peter Levart wrote:

Hi Naoto,

Sorry for joining late for review. Thanks for taking this on. I think it
is shaping well...

On 01/14/2017 01:54 AM, Naoto Sato wrote:



http://cr.openjdk.java.net/~naoto/8171139/webrev.05/



Tho things...

1. As said, the "cloning" of CacheKey has no purpose in the following
section of code (lines 1685...1709):

CacheKey constKey = new CacheKey(cacheKey);
trace("findBundle: %d %s %s formats: %s%n", index,
candidateLocales, cacheKey, formats);
try {
if (module.isNamed()) {
bundle = loadBundle(cacheKey, formats, control,
module, callerModule);
} else {
bundle = loadBundle(cacheKey, formats, control,
expiredBundle);
}
if (bundle != null) {
if (bundle.parent == null) {
bundle.setParent(parent);
}
bundle.locale = targetLocale;
bundle = putBundleInCache(cacheKey, bundle,
control);
return bundle;
}

// Put NONEXISTENT_BUNDLE in the cache as a mark that
there's no bundle
// instance for the locale.
putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, control);
} finally {
if (constKey.getCause() instanceof
InterruptedException) {
Thread.currentThread().interrupt();
}
}

The 'constKey' is copied of the 'cacheKey' and is not used anywhere
except in finally block where it is asked for .getCause() which will
always be null since copying of CacheKey never copies the cause...

2. We can make sure that CacheKey copy constructor always copies a
CacheKey that has non-cleared moduleRef and callerRef. The original
cacheKey is created in getBundleImpl() from nun-null module and
callerModule.  getBundleImpl() then calls down to findModule() with this
cacheKey. If we keep a reference to module and callerModule in
getBundleImpl() alive until the end of this method, we guarantee that
moduleRef and callerRef will not be cleared while using such cacheKey to
construct copies of it.

Here's those two changes applied to your webrev.05 and also a race in
clearCacheImpl() fixed (as reported by Daniel Fuchs):

http://cr.openjdk.java.net/~plevart/jdk9-dev/8170772_ResourceBundle.caching/webrev.06/




What do you think?

Regards, Peter

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Daniel Fuchs]

Hi Naoto,

On 14/01/17 00:54, Naoto Sato wrote:

diff -r a6d3c80ea436
src/java.base/share/classes/java/util/ResourceBundle.java
--- a/src/java.base/share/classes/java/util/ResourceBundle.java
+++ b/src/java.base/share/classes/java/util/ResourceBundle.java
@@ -2192,7 +2192,7 @@
 private static void clearCacheImpl(Module callerModule, ClassLoader
loader) {
 cacheList.keySet().removeIf(
 key -> key.getCallerModule() == callerModule &&
-   getLoader(key.getModule()) == loader
+   key.getModule() != null &&
getLoader(key.getModule()) == loader
 );
 }

This is the only occurence where CacheKey's modules are passed to
getLoader(Module). Entire webrev is here:

http://cr.openjdk.java.net/~naoto/8171139/webrev.05/


I think you need to introduce a new static method here, because
there's no guarantee that a module won't be GC'ed between
two calls to key.getModule() - the first call could return
non null and the second could return null:

private static boolean isMatching(CacheKey key, Module callerModule, 
ClassLoader loader) {

final Module module = key.getModule();
return key.getCallerModule() == callerModule &&
   module != null && getLoader(module) == loader;
}

and then use that in the lambda.

A related question is whether a CacheKey whose module is null
is stale? If so maybe clearCacheImpl could also be changed to
removed stale keys as well as matching keys?

private static boolean isMatching(CacheKey key, Module callerModule, 
ClassLoader loader) {

final Module km = key.getModule();
final Module kcm = key.getCallerModule();
return kcm == null || km == null || kcm == callerModule &&
   getLoader(km) == loader;
}


best regards,

-- daniel

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Daniel Fuchs]

+1

With Peter's proposed changes if I'm not mistaken then all methods
that operate on the CacheKey (findBundle, findBundleInCache, 
putBundleInCache, loadBundle, loadBundleFromProvider) are all

called from a point originating from within the block now protected
by the reachability fences, so there should be no opportunity for the
referents of our KeyElementReference to become null while the
CacheKey is used.

Thanks Peter!

-- daniel

On 17/01/17 11:53, Peter Levart wrote:

Hi Naoto,

Sorry for joining late for review. Thanks for taking this on. I think it
is shaping well...

On 01/14/2017 01:54 AM, Naoto Sato wrote:



http://cr.openjdk.java.net/~naoto/8171139/webrev.05/



Tho things...

1. As said, the "cloning" of CacheKey has no purpose in the following
section of code (lines 1685...1709):

CacheKey constKey = new CacheKey(cacheKey);
trace("findBundle: %d %s %s formats: %s%n", index,
candidateLocales, cacheKey, formats);
try {
if (module.isNamed()) {
bundle = loadBundle(cacheKey, formats, control,
module, callerModule);
} else {
bundle = loadBundle(cacheKey, formats, control,
expiredBundle);
}
if (bundle != null) {
if (bundle.parent == null) {
bundle.setParent(parent);
}
bundle.locale = targetLocale;
bundle = putBundleInCache(cacheKey, bundle, control);
return bundle;
}

// Put NONEXISTENT_BUNDLE in the cache as a mark that
there's no bundle
// instance for the locale.
putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, control);
} finally {
if (constKey.getCause() instanceof InterruptedException) {
Thread.currentThread().interrupt();
}
}

The 'constKey' is copied of the 'cacheKey' and is not used anywhere
except in finally block where it is asked for .getCause() which will
always be null since copying of CacheKey never copies the cause...

2. We can make sure that CacheKey copy constructor always copies a
CacheKey that has non-cleared moduleRef and callerRef. The original
cacheKey is created in getBundleImpl() from nun-null module and
callerModule.  getBundleImpl() then calls down to findModule() with this
cacheKey. If we keep a reference to module and callerModule in
getBundleImpl() alive until the end of this method, we guarantee that
moduleRef and callerRef will not be cleared while using such cacheKey to
construct copies of it.

Here's those two changes applied to your webrev.05 and also a race in
clearCacheImpl() fixed (as reported by Daniel Fuchs):

http://cr.openjdk.java.net/~plevart/jdk9-dev/8170772_ResourceBundle.caching/webrev.06/


What do you think?

Regards, Peter

RFR: 8172886: Add a test that shows how the LogManager can be implemented by a module

[Daniel Fuchs]

Hi,

Please find below a patch that adds a test that verifies that
LogManager, Handlers, and config classes can be implemented
from within a module (provided they are exported to java.logging).

JBS: https://bugs.openjdk.java.net/browse/JDK-8172886
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8172886/webrev.00/


best regards,

-- daniel

Re: 8172971: java.management could use System.Logger

[Daniel Fuchs]

Hi Roger,

On 19/01/17 18:22, Roger Riggs wrote:

Hi Daniel,

Very straight-forward.

I can see many places where the supplier form of logging could be used
instead
of the if (loggable...) {log(...) }; pattern.  But maybe not worth the
conversion effort or not part of this change.

And in some places there is both the if(loggable) and the supplier pattern
which results in a double check of isLoggable,  Explicitly calling the
sb.toString(),
instead of using the supplier form would avoid the double check. For
example,


Well - yes - I don't think it matters much. I didn't want to attempt
to capture all the parameters inside the supplier, and so I simply
changed strb.toString() into strb::toString.


new/src/java.management/share/classes/javax/management/MBeanServerFactory.java:


@@ -504,15 +502,12 @@
 throw new JMRuntimeException(msg, x);
 }
 } catch (RuntimeException x) {
-if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
+if (MBEANSERVER_LOGGER.isLoggable(Level.DEBUG)) {
 StringBuilder strb = new StringBuilder()
 .append("Failed to instantiate MBeanServerBuilder:
").append(x)
 .append("\n\t\tCheck the value of the ")
 .append(JMX_INITIAL_BUILDER).append(" property.");
-MBEANSERVER_LOGGER.logp(Level.FINEST,
-MBeanServerFactory.class.getName(),
-"checkMBeanServerBuilder",
-strb.toString());
+MBEANSERVER_LOGGER.log(Level.DEBUG, strb::toString);




There are a few files with very long lines that could be wrapped if it
was convenient.
(To make future reviews easier).
 - JmxMBeanServer.java, MLet.java, ModelMBeanAttributeInfo.java,
ModelMBeanConstructorInfo.java,
   ModelMBeanNotificationInfo.java, ModelMBeanOperationInfo.java,
RequiredModelMBean.java,
   RelationService.java, Timer.java,


Agreed - but I'd rather not do that now - as this changeset has many
files it would make the changes much more difficult to review.

best regards,

-- daniel




Roger

On 1/19/2017 12:12 PM, Mandy Chung wrote:

On Jan 19, 2017, at 7:30 AM, Daniel Fuchs <daniel.fu...@oracle.com>
wrote:

Hi,

Please find below a patch for:

8172971: java.management could use System.Logger
https://bugs.openjdk.java.net/browse/JDK-8172971

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8172971/webrev.00/


This looks good in general and pretty straight forward change.

Is it intentional to change the level FINEST to DEBUG as opposed to
TRACE in a couple places?  For example,

src/java.management/share/classes/javax/management/MBeanServerFactory.java

-if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
+if (MBEANSERVER_LOGGER.isLoggable(Level.DEBUG)) {

src/java.management/share/classes/com/sun/jmx/mbeanserver/MBeanServerDelegateImpl.java

-if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
-MBEANSERVER_LOGGER.logp(Level.FINEST,
-MBeanServerDelegateImpl.class.getName(),
-"getAttributes",
+if (MBEANSERVER_LOGGER.isLoggable(Level.TRACE)) {
+MBEANSERVER_LOGGER.log(Level.TRACE,
  "Attribute " + attn[i] + " not found");
  }


I have also added a new test:
test/sun/management/LoggingTest/LoggingTest.java

This test should have @modules java.logging and java.management.

Mandy

Re: 8172971: java.management could use System.Logger

[Daniel Fuchs]

Hi Mandy,

Thanks for the review!

On 19/01/17 17:12, Mandy Chung wrote:



On Jan 19, 2017, at 7:30 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

Hi,

Please find below a patch for:

8172971: java.management could use System.Logger
https://bugs.openjdk.java.net/browse/JDK-8172971

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8172971/webrev.00/



This looks good in general and pretty straight forward change.

Is it intentional to change the level FINEST to DEBUG as opposed to TRACE in a 
couple places?  For example,


The regular mapping would be:
  [CONFIG,FINE]  -> DEBUG,
  [FINER,FINEST] -> TRACE.

There are a couple of places were unexpected exceptions where traced
at level FINEST, and I arbitrarily decided that they should rather
be traced with level DEBUG.


src/java.management/share/classes/javax/management/MBeanServerFactory.java
-if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
+if (MBEANSERVER_LOGGER.isLoggable(Level.DEBUG)) {

src/java.management/share/classes/com/sun/jmx/mbeanserver/MBeanServerDelegateImpl.java
-if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
-MBEANSERVER_LOGGER.logp(Level.FINEST,
-MBeanServerDelegateImpl.class.getName(),
-"getAttributes",
+if (MBEANSERVER_LOGGER.isLoggable(Level.TRACE)) {
+MBEANSERVER_LOGGER.log(Level.TRACE,
 "Attribute " + attn[i] + " not found");
 }


I have also added a new test:
test/sun/management/LoggingTest/LoggingTest.java


This test should have @modules java.logging and java.management.


OK - I believed @modules java.management should already be in TEST.ROOT
I will double check.

best regards,

-- daniel


Mandy