RE: RFR (14) [testbug]: runs zero test, 8230002 and 8230010
Amy found them by grepping in all jtr log, she will setup a regular check since the issue raised from time to time. For this patch, then I will push it. Thanks Frank -Original Message- From: Joe Wang [mailto:huizhe.w...@oracle.com] Sent: Wednesday, August 28, 2019 1:05 AM To: Frank Yuan; 'core-libs-dev' Subject: Re: RFR (14) [testbug]: runs zero test, 8230002 and 8230010 Hi Frank, Thanks for fixing this. How did you find this out? Are you running some kind of tools? I hope that's the case since then we could be sure there were no other cases in the test suite. The patch looks good. It's fortunate we haven't broken anything ;-) The test passed just fine. Best, Joe On 8/27/19 2:06 AM, Frank Yuan wrote: > Hi all > > > > We found 2 jaxp tests, which didn't run indeed. > > > > Bugs: > > https://bugs.openjdk.java.net/browse/JDK-8230002 > > Annotation @Test was missed in TestNG test. > > > > https://bugs.openjdk.java.net/browse/JDK-8230010 > > The old test was left during it was converted to TestNG test. > > > > Webrev: > > http://cr.openjdk.java.net/~fyuan/8230002_8230010/webrev.00/ > > > > Thanks > > Frank >
RFR (14) [testbug]: runs zero test, 8230002 and 8230010
Hi all We found 2 jaxp tests, which didn't run indeed. Bugs: https://bugs.openjdk.java.net/browse/JDK-8230002 Annotation @Test was missed in TestNG test. https://bugs.openjdk.java.net/browse/JDK-8230010 The old test was left during it was converted to TestNG test. Webrev: http://cr.openjdk.java.net/~fyuan/8230002_8230010/webrev.00/ Thanks Frank
RE: RFR [14]: 8227438: [TESTLIB] Determine if file exists by Files.exists in function FileUtils.deleteFileIfExistsWithRetry
Thank you, Joe and Lance!
Pushed the change.
Frank
> -Original Message-
> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> Subject: Re: RFR [14]: 8227438: [TESTLIB] Determine if file exists by
> Files.exists in function FileUtils.deleteFileIfExistsWithRetry
>
> +1
>
> -Joe
>
>
> On 7/11/19 11:32 PM, Frank Yuan wrote:
> > Hi
> >
> >
> >
> > Would you like to review the following patch for
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8227438
> >
> >
> >
> > --- a/test/lib/jdk/test/lib/util/FileUtils.java Thu Jul 11 15:58:54
> > 2019 +
> >
> > +++ b/test/lib/jdk/test/lib/util/FileUtils.java Fri Jul 12 13:33:30 2019
> > +0800
> >
> > @@ -96,7 +96,7 @@
> >
> >*/
> >
> > public static void deleteFileIfExistsWithRetry(Path path) throws
> > IOException {
> >
> > try {
> >
> > -if (Files.exists(path)) {
> >
> > +if (!Files.notExists(path)) {
> >
> > deleteFileWithRetry0(path);
> >
> > }
> >
> > } catch (InterruptedException x) {
> >
> >
> >
> >
> >
> > Files.exists returns false if the existence cannot be determined, so
> > replace it with "!Files.notExists" like the change in
> > JDK-8184961.
> >
> >
> >
> > Thanks
> >
> > Frank
> >
> >
> >
RFR [14]: 8227438: [TESTLIB] Determine if file exists by Files.exists in function FileUtils.deleteFileIfExistsWithRetry
Hi
Would you like to review the following patch for
Bug: https://bugs.openjdk.java.net/browse/JDK-8227438
--- a/test/lib/jdk/test/lib/util/FileUtils.java Thu Jul 11 15:58:54 2019
+
+++ b/test/lib/jdk/test/lib/util/FileUtils.java Fri Jul 12 13:33:30 2019 +0800
@@ -96,7 +96,7 @@
*/
public static void deleteFileIfExistsWithRetry(Path path) throws
IOException {
try {
-if (Files.exists(path)) {
+if (!Files.notExists(path)) {
deleteFileWithRetry0(path);
}
} catch (InterruptedException x) {
Files.exists returns false if the existence cannot be determined, so replace it
with "!Files.notExists" like the change in
JDK-8184961.
Thanks
Frank
RE: VarHandle instance methods performance
Hi Martin There's a good chance COWAL access to the array can be optimized as you suggest using VarHandles. Write using release; read using acquire, or plain if holding the lock. I am not very sure, setRelease only ensures the ordering or has the effect that update the writing data to the memory immediately? If it only ensures the ordering, we may still need to volatile writing for this case CopyOnWriteArrayList… Doesn't buy much on x86 but performance improvement should be measurable on ppc. Oh, I realized there is no memory barrier for my volatile reading code on x86. Anyway there is performance benefit if we replace volatile writing with setRelease or releaseFence on x86. I would look for the scenarios in the library. We expect some potential performance benefit from keeping minimal memory access constraint in theory, but because the overhead of MethodHandle we may not take VarHandle api in some cases for the performance. Thanks YC On Wed, Apr 24, 2019 at 10:33 PM Frank Yuan mailto:frank.y...@oracle.com> > wrote: > > Hi Frank, > a VarHandle is a glorified integer value that correspond to the number of > bytes from a pointer to a field address, > so you give it a reference and you have access to the field at the address: > reference + shift, given that the shift value is the same for all the > instances of the same class, I know, but I thought they were ready once the VarHandle instance was constructed... Actually, I was looking up some way to improve library performance by VarHandle API. For example, CopyOnWriteArrayList uses a volatile variable referring to Object[] array, the writing procedure updates the volatile variable once it makes a new array completely, and then the reading procedure can read the new array by the volatile variable. Here, I think in theory, we can replace the volatile reading by getOpaque, which only requires the program does read the variable from the memory. But the actual performance of getOpaque is not better than volatile reading due to the overhead(with using static VH instance), I have to hold on my idea now. Thanks YC > it should be declared static (and final so it's a constant for the VM). > > Rémi > > - Mail original - > > De: "Frank Yuan" mailto:frank.y...@oracle.com> > > > À: "Aleksey Shipilev" mailto:sh...@redhat.com> > > > Cc: "core-libs-dev" > <mailto:core-libs-dev@openjdk.java.net> > > > Envoyé: Mercredi 24 Avril 2019 12:11:11 > > Objet: RE: VarHandle instance methods performance > > >> On 4/24/19 11:51 AM, Frank Yuan wrote: > >> > My test code is as below: > >> > ... > >> > final VarHandle vhf; > >> > final VarHandle vhvf; > >> > ... > >> > >> Make these two "static final", initialize them in class initializer, then > >> try > >> again. > >> > >> VarHandle (like Atomic*FieldUpdaters, MethodHandle, etc), have internal > >> checks > >> that can only be > >> folded away when the VH/MH/A*FU instance is constant. > >> > > > > Thank you, it works! > > > > YC > > > > > -Aleksey
RE: VarHandle instance methods performance
> > Hi Frank, > a VarHandle is a glorified integer value that correspond to the number of > bytes from a pointer to a field address, > so you give it a reference and you have access to the field at the address: > reference + shift, given that the shift value is the same for all the > instances of the same class, I know, but I thought they were ready once the VarHandle instance was constructed... Actually, I was looking up some way to improve library performance by VarHandle API. For example, CopyOnWriteArrayList uses a volatile variable referring to Object[] array, the writing procedure updates the volatile variable once it makes a new array completely, and then the reading procedure can read the new array by the volatile variable. Here, I think in theory, we can replace the volatile reading by getOpaque, which only requires the program does read the variable from the memory. But the actual performance of getOpaque is not better than volatile reading due to the overhead(with using static VH instance), I have to hold on my idea now. Thanks YC > it should be declared static (and final so it's a constant for the VM). > > Rémi > > - Mail original - > > De: "Frank Yuan" > > À: "Aleksey Shipilev" > > Cc: "core-libs-dev" > > Envoyé: Mercredi 24 Avril 2019 12:11:11 > > Objet: RE: VarHandle instance methods performance > > >> On 4/24/19 11:51 AM, Frank Yuan wrote: > >> > My test code is as below: > >> > ... > >> > final VarHandle vhf; > >> > final VarHandle vhvf; > >> > ... > >> > >> Make these two "static final", initialize them in class initializer, then > >> try > >> again. > >> > >> VarHandle (like Atomic*FieldUpdaters, MethodHandle, etc), have internal > >> checks > >> that can only be > >> folded away when the VH/MH/A*FU instance is constant. > >> > > > > Thank you, it works! > > > > YC > > > > > -Aleksey
RE: VarHandle instance methods performance
> On 4/24/19 11:51 AM, Frank Yuan wrote: > > My test code is as below: > > ... > > final VarHandle vhf; > > final VarHandle vhvf; > > ... > > Make these two "static final", initialize them in class initializer, then try > again. > > VarHandle (like Atomic*FieldUpdaters, MethodHandle, etc), have internal > checks that can only be > folded away when the VH/MH/A*FU instance is constant. > Thank you, it works! YC > -Aleksey
VarHandle instance methods performance
Hi Aleksey
I happened to see the performance to access a field by VarHandle API is much
worse than the native access.
I tested the following situations:
1. reading a volatile field directly
2. calling getVolatile against this volatile field
3. calling getVolatile against another non-volatile field
As my expectation, Situation 2 has similar performance with situation 3, but
both of them have 100 times of situation 1 execution time in my test.
I think it should be due to some overhead, and it doesn't violate the JEP 193
performance requirement. But I still can't figure out why it's so slow after
runtime compiling? Hope you can give me some point. Thanks!
My test code is as below:
import java.lang.invoke.MethodHandles;
import java.lang.invoke.VarHandle;
import java.lang.management.ManagementFactory;
import java.lang.management.ThreadMXBean;
public class Test5 {
static final int iter_num = 1;
private volatile int vf = 1;
private int f = 1;
final VarHandle vhf;
final VarHandle vhvf;
final ThreadMXBean threadMXBean;
public Test5 () throws Exception {
vhf = MethodHandles.lookup().findVarHandle(
Test5.class, "f", int.class);
vhvf = MethodHandles.lookup().findVarHandle(
Test5.class, "vf", int.class);
threadMXBean = ManagementFactory.getThreadMXBean();
//System.out.println(threadMXBean.isCurrentThreadCpuTimeSupported());
//System.out.println(threadMXBean.isThreadCpuTimeEnabled());
}
public void measGetVolatileField() {
int tmpProgress = 0;
long startCpu = threadMXBean.getCurrentThreadCpuTime();
long startUser = threadMXBean.getCurrentThreadUserTime();
for (int i = 0; i < iter_num; ++i) {
tmpProgress += vf;
}
long endCpu = threadMXBean.getCurrentThreadCpuTime();
long endUser = threadMXBean.getCurrentThreadUserTime();
long durCpu = endCpu -startCpu;
long durUser = endUser - startUser;
System.out.println("measGetVolatileField");
System.out.println("cpu time: " + durCpu);
System.out.println("user time: " + durUser);
System.out.println(tmpProgress);
}
public void measGetVolatileFieldByVHWithVolatile() {
int tmpProgress = 0;
long startCpu = threadMXBean.getCurrentThreadCpuTime();
long startUser = threadMXBean.getCurrentThreadUserTime();
for (int i = 0; i < iter_num; ++i) {
tmpProgress += (int) vhvf.getVolatile(this);
}
long endCpu = threadMXBean.getCurrentThreadCpuTime();
long endUser = threadMXBean.getCurrentThreadUserTime();
long durCpu = endCpu -startCpu;
long durUser = endUser - startUser;
System.out.println("measGetVolatileFieldByVHWithVolatile");
System.out.println("cpu time: " + durCpu);
System.out.println("user time: " + durUser);
System.out.println(tmpProgress);
}
public void measGetFieldByVHWithVolatile() {
int tmpProgress = 0;
long startCpu = threadMXBean.getCurrentThreadCpuTime();
long startUser = threadMXBean.getCurrentThreadUserTime();
for (int i = 0; i < iter_num; ++i) {
tmpProgress += (int) vhf.getVolatile(this);
}
long endCpu = threadMXBean.getCurrentThreadCpuTime();
long endUser = threadMXBean.getCurrentThreadUserTime();
long durCpu = endCpu -startCpu;
long durUser = endUser - startUser;
System.out.println("measGetFieldByVHWithVolatile");
System.out.println("cpu time: " + durCpu);
System.out.println("user time: " + durUser);
System.out.println(tmpProgress);
}
public static void main(String[] args) throws Exception {
for (int i = 0; i < 5; i++) {
Test5 test = new Test5 ();
Thread threadA = new Thread(() -> test.measGetVolatileField());
Thread threadB = new Thread(() ->
test.measGetVolatileFieldByVHWithVolatile());
Thread threadC = new Thread(() ->
test.measGetFieldByVHWithVolatile());
threadA.start();
threadA.join();
threadB.start();
threadB.join();
threadC.start();
threadC.join();
}
}
}
RE: RFR (12): 8213300: jaxp/unittest/transform/CR6551600Test.java fails due to exception in jdk/jdk CI
Thank you, Joe! Pushed the change. Frank > > Hi Frank, > > The change looks good to me. Thanks for fixing the failure! > > Best, > Joe > > On 12/4/18, 6:19 PM, Frank Yuan wrote: > > Hi all > > > > > > > > Would you like to review this patch? > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8213300 > > > > Webrev: http://cr.openjdk.java.net/~fyuan/8213300/webrev.00/ > > > > > > > > This patch made the following changes: > > > > 1. change the path of test file from the root directory of drive C to > > C:\temp directory > > > > 2. check if the UNC path is accessible before running the jaxp test > > > > 3. skip the test if it's not Windows because the UNC test is only > > valid on Windows > > > > > > > > Thanks > > > > Frank > > > > > >
RFR (12): 8213300: jaxp/unittest/transform/CR6551600Test.java fails due to exception in jdk/jdk CI
Hi all Would you like to review this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8213300 Webrev: http://cr.openjdk.java.net/~fyuan/8213300/webrev.00/ This patch made the following changes: 1. change the path of test file from the root directory of drive C to C:\temp directory 2. check if the UNC path is accessible before running the jaxp test 3. skip the test if it's not Windows because the UNC test is only valid on Windows Thanks Frank
RE: RFR (12): 8210819: Update the host name in CNameTest.java
Thank you! Pushed.
Frank
> -Original Message-
> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
> Sent: Monday, September 17, 2018 6:16 PM
> To: Frank Yuan; 'core-libs-dev'
> Subject: Re: RFR (12): 8210819: Update the host name in CNameTest.java
>
> Looks fine Frank. Thanks,
> -Chris.
>
> On 17/09/18 08:12, Frank Yuan wrote:
> > Hi
> >
> >
> >
> > Would you like to review the following patch for
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210819
> >
> >
> >
> > --- a/test/jdk/sun/net/InetAddress/nameservice/dns/CNameTest.java
> > Wed Sep 12 21:56:59 2018 -0700
> >
> > +++ b/test/jdk/sun/net/InetAddress/nameservice/dns/CNameTest.java Mon
> > Sep 17 15:00:55 2018 +0800
> >
> > @@ -1,5 +1,5 @@
> >
> > /*
> >
> > - * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> >
> > + * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights
> > reserved.
> >
> >* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> >
> >*
> >
> >* This code is free software; you can redistribute it and/or modify it
> >
> > @@ -38,7 +38,7 @@
> >
> >* @summary Test DNS provider's handling of CNAME records
> >
> >*/
> >
> > public class CNameTest {
> >
> > -private static final String HOST = "www-proxy.us.oracle.com";
> >
> > +private static final String HOST = "www.w3c.org";
> >
> >
> >
> > CNameTest.java is to test DNS provider's handling of CNAME records, so the
> > test requires dns has cname record for the hostname. The
> > old name is not appropriate, I would replace it with www.w3c.org
> > <http://www.w3c.org> .
> >
> >
> >
> > Thanks
> >
> > Frank
> >
RFR (12): 8210819: Update the host name in CNameTest.java
Hi
Would you like to review the following patch for
Bug: https://bugs.openjdk.java.net/browse/JDK-8210819
--- a/test/jdk/sun/net/InetAddress/nameservice/dns/CNameTest.java Wed
Sep 12 21:56:59 2018 -0700
+++ b/test/jdk/sun/net/InetAddress/nameservice/dns/CNameTest.java Mon Sep
17 15:00:55 2018 +0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -38,7 +38,7 @@
* @summary Test DNS provider's handling of CNAME records
*/
public class CNameTest {
-private static final String HOST = "www-proxy.us.oracle.com";
+private static final String HOST = "www.w3c.org";
CNameTest.java is to test DNS provider's handling of CNAME records, so the test
requires dns has cname record for the hostname. The
old name is not appropriate, I would replace it with www.w3c.org
.
Thanks
Frank
RE: [10] [testbug] RFR of JDK-8187700 SetAuthenticator tests should handle the proxy port
Hi Daniel Thank you very much! I pushed it. Frank -Original Message- From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] Subject: Re: [10] [testbug] RFR of JDK-8187700 SetAuthenticator tests should handle the proxy port Hi Frank, This looks good to me. best regards, -- daniel On 28/09/2017 09:37, Frank Yuan wrote: > Hi Daniel and all > > Would you like to review > http://cr.openjdk.java.net/~fyuan/8187700/webrev.00/? > > Bug: https://bugs.openjdk.java.net/browse/JDK-8187700 > > I applied the fix for proxy port reusing issue, which was reviewed in > bug JDK-8187507. And made a few code cleaning in > HTTPSetAuthenticatorTest.java > > Thanks > > Frank >
RE: [10] [testbug] RFR of JDK-8187700 SetAuthenticator tests should handle the proxy port
Hi Daniel and all Would you like to have a look on this review? It's the same issue as JDK-8188086 but in SetAuthenticator tests. The fix's verified by running the test in loop. Thanks Frank From: Frank Yuan [mailto:frank.y...@oracle.com] Subject: [10] [testbug] RFR of JDK-8187700 SetAuthenticator tests should handle the proxy port Hi Daniel and all Would you like to review http://cr.openjdk.java.net/~fyuan/8187700/webrev.00/? Bug: https://bugs.openjdk.java.net/browse/JDK-8187700 I applied the fix for proxy port reusing issue, which was reviewed in bug JDK-8187507. And made a few code cleaning in HTTPSetAuthenticatorTest.java Thanks Frank
[10] [testbug] RFR of JDK-8187700 SetAuthenticator tests should handle the proxy port
Hi Daniel and all Would you like to review http://cr.openjdk.java.net/~fyuan/8187700/webrev.00/? Bug: https://bugs.openjdk.java.net/browse/JDK-8187700 I applied the fix for proxy port reusing issue, which was reviewed in bug JDK-8187507. And made a few code cleaning in HTTPSetAuthenticatorTest.java Thanks Frank
RE: RFR 8177932 (process) java/lang/ProcessHandle/OnExitTest.java failed with "Process A should not be alive"
We have not see any issues with pid re-use and I would prefer to avoid over engineering the code for a non-issue. Yes, I agree. With the proposed fix, the onExit completion handler and isAlive are consistent. I mean, in ProcessHandleImpl.isAlive(), startTime is the final filed of the ProcessHandleImpl instance,in onExit(), the pid of the ProcessHandleImpl instance is passed to completion function, and then origStart is assigned with the return value of isAlive0(pid), at this time, the value of origStart may be different with startTime of the ProcessHandleImpl instance, so onExit() of the ProcessHandleImpl instance is inconsistent with isAlive()of the same ProcessHandleImpl instance. Anyway, it may be a non-issue:) b. Should we rollback the change of JDK-8184808, at least, update the comment in that change? I'll update the comment. Double checking using kill seems to be more reliable. c. Should we remove @key intermittent and the debug info added in JDK-8183019? I'd prefer to leave them for some to make sure the issue does not re-appear. I created a subtask to re-check in 2 months. Thank you for your follow-up! Frank Roger Thanks Frank -Original Message- From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf Of Roger Riggs Subject: RFR 8177932 (process) java/lang/ProcessHandle/OnExitTest.java failed with "Process A should not be alive" Please review a fix for an intermittent issue with ProcessHandle.onExit. On Solaris, the start time of a process reported through /proc/pid/psinfo changes to zero when the process is exiting. The onExit implementation incorrectly interpreted zero meaning the pid had been re-used and the process was no longer alive. However, ProcessHandle.isAlive considered zero to be missing information and the process was still alive. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-onexit-8177932/ Issue: https://bugs.openjdk.java.net/browse/JDK-8177932 Thanks, Roger
RE: RFR 8177932 (process) java/lang/ProcessHandle/OnExitTest.java failed with "Process A should not be alive"
Hi Roger I think this is the cause and I believe the patch will work. However, I have the following concern and minor comments: a. Is it possible that following scenario happens? 1. process A exits 2. Completion thread in onExist runs and gets 0 as origStart, and then sleeps 5s 3. process reaper thread reaps the process A, and then the pid is reused 4. Completion thread gets a value as startTime, and then has to wait the new process termination I am not sure if the startTIme may be zero except the process is a zombie, if the startTIme may be zero only when the process is a zombie, we can change the condition to: if (startTime > 0 && startTime != origStart), otherwise we can't avoid such scenario, but if ProcessHandleImpl.onExit() passes startTime field to function completion, it may be a little bit better, at least, onExit() can be consistent with isAlive(). b. Should we rollback the change of JDK-8184808, at least, update the comment in that change? c. Should we remove @key intermittent and the debug info added in JDK-8183019? Thanks Frank > -Original Message- > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf > Of Roger Riggs > Subject: RFR 8177932 (process) java/lang/ProcessHandle/OnExitTest.java failed > with "Process A should not be alive" > > Please review a fix for an intermittent issue with ProcessHandle.onExit. > On Solaris, the start time of a process reported through > /proc/pid/psinfo changes to > zero when the process is exiting. The onExit implementation incorrectly > interpreted zero > meaning the pid had been re-used and the process was no longer alive. > However, > ProcessHandle.isAlive considered zero to be missing information and the > process was still alive. > > Webrev: >http://cr.openjdk.java.net/~rriggs/webrev-onexit-8177932/ > > Issue: >https://bugs.openjdk.java.net/browse/JDK-8177932 > > Thanks, Roger
RE: RFR(jdk10/jaxp) 8163121: BCEL: update to the latest 6.0 release
Hi Joe and Daniel > > So far so good, most of them still seem to be present in > > your changes, but I have a doubt about these two: > > > > https://bugs.openjdk.java.net/browse/JDK-8062608 > > https://bugs.openjdk.java.net/browse/JDK-8064516 > > > > Has the issue been fixed upstream in a different way, or > > is the issue no longer relevant, or is it going to appear > > again with this patch? > > Good catch! The original patch came from BCEL, so I indeed assumed this > update would get things the right way. As I double-checked, it turns out > the additional patches as you listed above fixed issues that BCEL 6.0 > hasn't. These issues were revealed by JRocket, did not affect JAXP, > however, they are still good changes that we shall keep. I've gone > through both of the above to put these changes back into the code. > IIUC JDK-8062608 and JDK-8064516 are related to JDK-8003147 but for JRocket, the first webrev(http://cr.openjdk.java.net/~joehw/jdk10/8163121/webrev/) looks different with the fix of JDK-8003147, however we have the relevant test in the repo, i.e. http://hg.openjdk.java.net/jdk9/jdk9/jaxp/file/tip/test/javax/xml/jaxp/unittest/parsers/Bug8003147Test.java, since all tests were passed, I assume the first webrev worked for JDK-8003147, certainly the second webrev looks good too :) Thanks Frank
RE: RFR(S): 8180195: remove jaxp testlibrary
Looks fine, although I am not a reviewer. Thanks Frank > -Original Message- > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf > Of Igor Ignatyev > Subject: RFR(S): 8180195: remove jaxp testlibrary > > http://cr.openjdk.java.net/~iignatyev//8180195/webrev.00/index.html > > 3049 lines changed: 114 ins; 2927 del; 8 mod; > > Hi all, > > could you please review this small patch which removes a fork of testlibrary > from jaxp repo? there were a few differences b/w the testlibraries: > top level testlibrary did not have CompilerUtils class, its ProcessTools did > not have executeTestJava (which is basically an alias for > executeTestJvm) and its OutputAnalyzer did not have methods to dump stdout, > stderr into specific streams. > > this fix is a part of ongoing effort on merging and cleaning up our test > libraries[1]. > > bug: https://bugs.openjdk.java.net/browse/JDK-8180195 > webrev: http://cr.openjdk.java.net/~iignatyev//8180195/webrev.00/index.html > testing: :jaxp_all > > [1] https://bugs.openjdk.java.net/browse/JDK-8075327 > > Thanks, > -- Igor
RE: RFR [JAXP] [TESTBUG] JDK-8175043 Multiple jaxp tests failing across platforms
Since no one has more comment, and Joe agree this solution, I will push the change. Thanks Frank > -Original Message- > From: Weijun Wang [mailto:weijun.w...@oracle.com] > Sent: Thursday, February 16, 2017 4:35 PM > To: Frank Yuan; 'Daniel Fuchs'; core-libs-dev@openjdk.java.net > Subject: Re: RFR [JAXP] [TESTBUG] JDK-8175043 Multiple jaxp tests failing > across platforms > > Hi Frank > > You got the idea here correctly, that is to say, the path used in > permission granting must match the style of how you access the file. The > fix should work. > > On the other hand, the code change touches too many files. You'll need > someone in the JAXP field to confirm if this is the best way. > > Thanks > Max > > On 02/16/2017 04:03 PM, Frank Yuan wrote: > > Hi Max and All > > > > Would you like to review > > http://cr.openjdk.java.net/~fyuan/8175043/webrev.00/? > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8175043 > > > > Some JAXP tests are impacted by JDK-8168410, to fix the issue, I added > > the pull path name to the file name in the tests, and added a temporary > > FilePermission for the specified file names in XSLTFunctionsTest.java. > > > > To Max > > > > I didn't take your proposal because we do want to assign FilePermission > > for the specified userdir, not for ".". To avoid assigning potentially > > extra permission now or future, I used the explicit full file path in > > the test. > > > > Thanks > > Frank > > > > > >
RE: RFR [JAXP] [TESTBUG] JDK-8175043 Multiple jaxp tests failing across platforms
> -Original Message- > From: Weijun Wang [mailto:weijun.w...@oracle.com] > Subject: Re: RFR [JAXP] [TESTBUG] JDK-8175043 Multiple jaxp tests failing > across platforms > > Hi Frank > > You got the idea here correctly, that is to say, the path used in > permission granting must match the style of how you access the file. The > fix should work. > > On the other hand, the code change touches too many files. You'll need > someone in the JAXP field to confirm if this is the best way. Alright, thank you very much for the review! Frank > > Thanks > Max > > On 02/16/2017 04:03 PM, Frank Yuan wrote: > > Hi Max and All > > > > Would you like to review > > http://cr.openjdk.java.net/~fyuan/8175043/webrev.00/? > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8175043 > > > > Some JAXP tests are impacted by JDK-8168410, to fix the issue, I added > > the pull path name to the file name in the tests, and added a temporary > > FilePermission for the specified file names in XSLTFunctionsTest.java. > > > > To Max > > > > I didn't take your proposal because we do want to assign FilePermission > > for the specified userdir, not for ".". To avoid assigning potentially > > extra permission now or future, I used the explicit full file path in > > the test. > > > > Thanks > > Frank > > > > > >
RFR [JAXP] [TESTBUG] JDK-8175043 Multiple jaxp tests failing across platforms
Hi Max and All Would you like to review http://cr.openjdk.java.net/~fyuan/8175043/webrev.00/? Bug: https://bugs.openjdk.java.net/browse/JDK-8175043 Some JAXP tests are impacted by JDK-8168410, to fix the issue, I added the pull path name to the file name in the tests, and added a temporary FilePermission for the specified file names in XSLTFunctionsTest.java. To Max I didn't take your proposal because we do want to assign FilePermission for the specified userdir, not for ".". To avoid assigning potentially extra permission now or future, I used the explicit full file path in the test. Thanks Frank
RE: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by JDK-8087303
Thank you very much for the check! Pushed.
Frank
> -Original Message-
> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> Sent: Wednesday, February 15, 2017 11:35 AM
> To: Frank Yuan; 'Daniel Fuchs'
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by
> JDK-8087303
>
> Sounds good. The change to check LF only looks good.
>
> Thanks,
> Joe
>
> On 2/14/2017 7:22 PM, Frank Yuan wrote:
> > Hi Joe and Daniel
> >
> > I tested more and checked the code, both of you are right.
> >
> > CRLF, CR, LF in the xml file are all normalized to LF, and finally output
> > to the platform-related line.separator if it's
serialized.
> > The serializer only handle LF as newline. User can create CR text node
> > programmatically or there may be some case that CR is
sent to
> > the serializer intentionally, but anyway in these cases, CR is not regarded
> > as a newline. So the change as Joe's comment and I
> > stated in previous mail is made:
> > http://cr.openjdk.java.net/~fyuan/8174025/webrev.02/
> >
> > Thanks
> > Frank
> >
> >> -Original Message-
> >> From: Frank Yuan [mailto:frank.y...@oracle.com]
> >> Subject: RE: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by
> >> JDK-8087303
> >>
> >>
> >>> -Original Message-
> >>> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> >>> Subject: Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused
> >> by JDK-8087303
> >>> Looks good to me as well.
> >>>
> >>> For the CR and LF question, XML processors are required by the
> >>> specification to normalize and translate both CRLF sequence and any CR
> >>> not followed by LF to a single LF. For that reason, you don't have
> >>> check CR since the content the serializer gets is parsed.
> >>>
> >> Thank you very much for explanation, since that, I will remove the check
> >> for CR as below:
> >> 3439c3439
> >> < while (skipBeginningNewlines && (text[start] == '\n'
> >> || text[start] == '\r')) {
> >> ---
> >>> while (skipBeginningNewlines && text[start] == '\n')
> >> {
> >> 3498c3498
> >> < while (skipBeginningNewlines && (text[start] ==
> >> '\n' || text[start] == '\r')) {
> >> ---
> >>> while (skipBeginningNewlines && text[start] ==
> >> '\n') {
> >>
> >> Once it is passed by all tests, I will push the change.
> >>
> >> Thanks
> >> Frank
> >>
RE: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by JDK-8087303
Hi Joe and Daniel
I tested more and checked the code, both of you are right.
CRLF, CR, LF in the xml file are all normalized to LF, and finally output to
the platform-related line.separator if it's serialized.
The serializer only handle LF as newline. User can create CR text node
programmatically or there may be some case that CR is sent to
the serializer intentionally, but anyway in these cases, CR is not regarded as
a newline. So the change as Joe's comment and I
stated in previous mail is made:
http://cr.openjdk.java.net/~fyuan/8174025/webrev.02/
Thanks
Frank
> -Original Message-
> From: Frank Yuan [mailto:frank.y...@oracle.com]
> Subject: RE: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by
> JDK-8087303
>
>
> > -Original Message-
> > From: huizhe wang [mailto:huizhe.w...@oracle.com]
> > Subject: Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused
> by JDK-8087303
> >
> > Looks good to me as well.
> >
> > For the CR and LF question, XML processors are required by the
> > specification to normalize and translate both CRLF sequence and any CR
> > not followed by LF to a single LF. For that reason, you don't have
> > check CR since the content the serializer gets is parsed.
> >
> Thank you very much for explanation, since that, I will remove the check
> for CR as below:
> 3439c3439
> < while (skipBeginningNewlines && (text[start] == '\n'
> || text[start] == '\r')) {
> ---
> > while (skipBeginningNewlines && text[start] == '\n')
> {
> 3498c3498
> < while (skipBeginningNewlines && (text[start] ==
> '\n' || text[start] == '\r')) {
> ---
> > while (skipBeginningNewlines && text[start] ==
> '\n') {
>
> Once it is passed by all tests, I will push the change.
>
> Thanks
> Frank
>
> > Best,
> > Joe
> >
> > On 2/14/2017 6:58 AM, 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,
> > >>
> > >> 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. ;-)
> > > Hmm, wait for Joe to confirm.
> > >
> > >> 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'.
> > >>
> > > Agree.
> > >
> > >> 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.
> > >>
> > > Yes, understand!
> > >
> > > Thanks
> > > Frank
> > >
> > >> 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/
> > >>>>>
> > >>>&
RE: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by JDK-8087303
> -Original Message-
> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> Subject: Re: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by
> JDK-8087303
>
> Looks good to me as well.
>
> For the CR and LF question, XML processors are required by the
> specification to normalize and translate both CRLF sequence and any CR
> not followed by LF to a single LF. For that reason, you don't have
> check CR since the content the serializer gets is parsed.
>
Thank you very much for explanation, since that, I will remove the check for CR
as below:
3439c3439
< while (skipBeginningNewlines && (text[start] == '\n' ||
text[start] == '\r')) {
---
> while (skipBeginningNewlines && text[start] == '\n') {
3498c3498
< while (skipBeginningNewlines && (text[start] == '\n'
|| text[start] == '\r')) {
---
> while (skipBeginningNewlines && text[start] == '\n') {
Once it is passed by all tests, I will push the change.
Thanks
Frank
> Best,
> Joe
>
> On 2/14/2017 6:58 AM, 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,
> >>
> >> 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. ;-)
> > Hmm, wait for Joe to confirm.
> >
> >> 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'.
> >>
> > Agree.
> >
> >> 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.
> >>
> > Yes, understand!
> >
> > Thanks
> > Frank
> >
> >> 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
RE: RFR [JAXP] JDK-8174025 Regression in XML Transform caused by JDK-8087303
> -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, > > 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. ;-) Hmm, wait for Joe to confirm. > 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'. > Agree. > 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. > Yes, understand! Thanks Frank > 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-8174025 Regression in XML Transform caused by JDK-8087303
> -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'? > 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-8174025 Regression in XML Transform caused by JDK-8087303
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
Hi Joe and Daniel Thank you very much for your review! Frank -Original Message- From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Saturday, February 11, 2017 5:52 AM To: Daniel Fuchs; Frank Yuan; core-libs-dev@openjdk.java.net 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 >> >> >> >
RFR [JAXP] JDK-8170192 Regression in XML Transform caused by JDK-8087303
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) 8169827: javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed
Thank you, Christoph Frank > -Original Message- > From: Langer, Christoph [mailto:christoph.lan...@sap.com] > Sent: Tuesday, January 24, 2017 6:30 PM > To: Frank Yuan > Cc: 'Daniel Fuchs'; core-libs-dev@openjdk.java.net > Subject: RE: RFR (JAXP) 8169827: > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > Hi, > > I submitted my fix under Bug https://bugs.openjdk.java.net/browse/JDK-8173261: > http://hg.openjdk.java.net/jdk9/dev/jaxp/rev/620d0c38581f > > I also added a comment to https://bugs.openjdk.java.net/browse/JDK-8169827 > > Best regards > Christoph > > > > -Original Message- > > From: Frank Yuan [mailto:frank.y...@oracle.com] > > Sent: Dienstag, 24. Januar 2017 09:45 > > To: Langer, Christoph > > Cc: 'Daniel Fuchs' ; core-libs-dev@openjdk.java.net > > Subject: RE: RFR (JAXP) 8169827: > > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > > > > From: Langer, Christoph [mailto:christoph.lan...@sap.com] > > > To: Frank Yuan > > > Cc: 'Daniel Fuchs'; core-libs-dev@openjdk.java.net > > > Subject: RE: RFR (JAXP) 8169827: > > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > > > > > Hi Frank, > > > > > > thanks for your comments. > > > > > > I already thought it might be better to create a new bug for my repair > > > work - > > as 8169827 actually is about the intermittent > > failure which will > > > perhaps not be solved with my changes. So I'll create a new item and > > > submit > > my changes for that one. > > > > > > Furthermore, I would also support creating Java code to do the copy work > > instead of using the shell script, though doing this is > > rather out of > > > scope for me, at the moment. > > > > Just a suggestion, please feel free to leave it:) > > > > >This should be done in a common library, e.g. > > /test/javax/xml/jaxp/libs/jaxp/library. I also see that the same > > > requirement exists for tests in the jdk repo - unfortunately there is no > > > shared > > testing library with 'jdk'. So we'll probably end > > up with some > > > duplicated code :( > > > > > > By the way: In the jaxp test tree, there exists a lib > > /test/javax/xml/jaxp/libs/jdk/testlibrary out of which at least some stuff > > seems to be > > > obsolete for jaxp testing and/or significantly behind jdk's > > > test/lib/testlibrary. I > > guess here's some room for > > cleanup/modernization. > > > > > > > Yes, testlibrary is a copy from jdk repo, there are a few JAXP tests using > > the > > library, so we didn't keep it synced with JDK. In my > > mind, we don't need spend too much effort on it unless it impacts JAXP > > tests. > > > > > As for the idea with the links - it sounds interesting, but it also adds > > > more > > complexity. And maybe it is not such a good idea for > > Windows > > > systems... > > > > I didn't test it in Windows, please leave it to me. I will update to you > > and Joe > > when I have the result. Thanks > > > > Frank > > > > > > > > Best regards > > > Christoph > > > > > > > -Original Message- > > > > From: Frank Yuan [mailto:frank.y...@oracle.com] > > > > Sent: Dienstag, 24. Januar 2017 08:59 > > > > To: Langer, Christoph ; 'Daniel Fuchs' > > > > ; core-libs-dev@openjdk.java.net > > > > Subject: RE: RFR (JAXP) 8169827: > > > > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > > > > > > > Hi Christoph > > > > > > > > Many thanks for your effort! > > > > > > > > It's really better than the old version! However I have 2 comments > > > > although > > I > > > > am not a reviewer(as you often stated :)) > > > > 1. we could also write java code to copy/delete JDK. > > > > 2. 8169827 is to track PropertiesTest failed in copying JDK > > > > intermittently. I > > > > suggest to use another bug for your improvement > > > > because I am not sure if this patch can improve the issue of 8169827. > > > > > > > > To Christoph and Daniel > > > > > > > > Btw, Christoph's patch inspired me. You know, Prop
RE: RFR (JAXP) 8169827: javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed
> From: Langer, Christoph [mailto:christoph.lan...@sap.com] > To: Frank Yuan > Cc: 'Daniel Fuchs'; core-libs-dev@openjdk.java.net > Subject: RE: RFR (JAXP) 8169827: > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > Hi Frank, > > thanks for your comments. > > I already thought it might be better to create a new bug for my repair work - > as 8169827 actually is about the intermittent failure which will > perhaps not be solved with my changes. So I'll create a new item and submit > my changes for that one. > > Furthermore, I would also support creating Java code to do the copy work > instead of using the shell script, though doing this is rather out of > scope for me, at the moment. Just a suggestion, please feel free to leave it:) >This should be done in a common library, e.g. >/test/javax/xml/jaxp/libs/jaxp/library. I also see that the same > requirement exists for tests in the jdk repo - unfortunately there is no > shared testing library with 'jdk'. So we'll probably end up with some > duplicated code :( > > By the way: In the jaxp test tree, there exists a lib > /test/javax/xml/jaxp/libs/jdk/testlibrary out of which at least some stuff seems to be > obsolete for jaxp testing and/or significantly behind jdk's > test/lib/testlibrary. I guess here's some room for cleanup/modernization. > Yes, testlibrary is a copy from jdk repo, there are a few JAXP tests using the library, so we didn't keep it synced with JDK. In my mind, we don't need spend too much effort on it unless it impacts JAXP tests. > As for the idea with the links - it sounds interesting, but it also adds more > complexity. And maybe it is not such a good idea for Windows > systems... I didn't test it in Windows, please leave it to me. I will update to you and Joe when I have the result. Thanks Frank > > Best regards > Christoph > > > -Original Message- > > From: Frank Yuan [mailto:frank.y...@oracle.com] > > Sent: Dienstag, 24. Januar 2017 08:59 > > To: Langer, Christoph ; 'Daniel Fuchs' > > ; core-libs-dev@openjdk.java.net > > Subject: RE: RFR (JAXP) 8169827: > > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > > > Hi Christoph > > > > Many thanks for your effort! > > > > It's really better than the old version! However I have 2 comments although > > I > > am not a reviewer(as you often stated :)) > > 1. we could also write java code to copy/delete JDK. > > 2. 8169827 is to track PropertiesTest failed in copying JDK intermittently. > > I > > suggest to use another bug for your improvement > > because I am not sure if this patch can improve the issue of 8169827. > > > > To Christoph and Daniel > > > > Btw, Christoph's patch inspired me. You know, PropertiesTest copies a JDK in > > order to isolate the file change of the JDK, actually > > there are some other similar tests(to test some other JDK property or config > > files) in JDK repo, they take the same way as well as > > have similar code... > > I have another idea for this test, that is we can only make symbolic link > > to the > > JDK files/directories except conf directory, create > > a directory named with "conf" under the new jdk directory, and then make > > symbolic link to the files/directories in conf to the real > > jdk/conf except jaxp.properties. This will reduce the most of copying work > > and > > may reduce the risk of copying work. What's your > > opinion? > > > > Thanks > > Frank > > > > > -Original Message- > > > From: Langer, Christoph [mailto:christoph.lan...@sap.com] > > > To: Daniel Fuchs; Frank Yuan; core-libs-dev@openjdk.java.net > > > Subject: RE: RFR (JAXP) 8169827: > > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > > > > > Hi Daniel, > > > > > > thanks for checking/reviewing. So I'll submit with removing the > > ProblemList.txt entry and I'll also remove the intermittent flag. > > > > > > Sounds fair to check later if problems will still show up. Although I > > > have the > > feeling that the issue of > > > https://bugs.openjdk.java.net/browse/JDK-8147431 might appear again... > > > > > > Best regards > > > Christoph > > > > > > > -Original Message- > > > > From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] > > > > Sent: Montag, 23. Januar 2017 17:12 > > > > To: Langer, Christop
RE: RFR (JAXP) 8169827: javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed
Hi Christoph Many thanks for your effort! It's really better than the old version! However I have 2 comments although I am not a reviewer(as you often stated :)) 1. we could also write java code to copy/delete JDK. 2. 8169827 is to track PropertiesTest failed in copying JDK intermittently. I suggest to use another bug for your improvement because I am not sure if this patch can improve the issue of 8169827. To Christoph and Daniel Btw, Christoph's patch inspired me. You know, PropertiesTest copies a JDK in order to isolate the file change of the JDK, actually there are some other similar tests(to test some other JDK property or config files) in JDK repo, they take the same way as well as have similar code... I have another idea for this test, that is we can only make symbolic link to the JDK files/directories except conf directory, create a directory named with "conf" under the new jdk directory, and then make symbolic link to the files/directories in conf to the real jdk/conf except jaxp.properties. This will reduce the most of copying work and may reduce the risk of copying work. What's your opinion? Thanks Frank > -Original Message- > From: Langer, Christoph [mailto:christoph.lan...@sap.com] > To: Daniel Fuchs; Frank Yuan; core-libs-dev@openjdk.java.net > Subject: RE: RFR (JAXP) 8169827: > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > Hi Daniel, > > thanks for checking/reviewing. So I'll submit with removing the > ProblemList.txt entry and I'll also remove the intermittent flag. > > Sounds fair to check later if problems will still show up. Although I have > the feeling that the issue of > https://bugs.openjdk.java.net/browse/JDK-8147431 might appear again... > > Best regards > Christoph > > > -Original Message- > > From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] > > Sent: Montag, 23. Januar 2017 17:12 > > To: Langer, Christoph ; Frank Yuan > > ; core-libs-dev@openjdk.java.net > > Subject: Re: RFR (JAXP) 8169827: > > javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh copied JDK failed > > > > 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: Does jvisualvm work in JDK 9?
Thank you for tne clarification! Frank > -Original Message- > From: Seán Coffey [mailto:sean.cof...@oracle.com] > Subject: Re: Does jvisualvm work in JDK 9? > > I don't believe jvisualvm will ship bundled with JDK 9 and is due for > removal. Probably best to download the latest visualvm binaries from the > VisualVM project website. > > See https://blogs.oracle.com/java-platform-group/entry/visual_vm_in_jdk_9 > > regards, > Sean. > > On 29/12/2016 02:39, Frank Yuan wrote: > > Hi Dmitry > > > > Thank you very much for your help! > > > > Unluckily visualvm doesn't work for 147 and 150 as well, I have to give up > > using this tool in current stage... > > > > > > Frank > > > > > >> -Original Message- > >> From: Dmitry Buzdin [mailto:buz...@gmail.com] > >> Sent: Wednesday, December 28, 2016 4:57 PM > >> To: Frank Yuan; code-tools-...@openjdk.java.net; > >> core-libs-dev@openjdk.java.net > >> Subject: Re: Does jvisualvm work in JDK 9? > >> > >> Hi, > >> > >> I had a similar problem on Mac. Downloaded the latest version of > >> VisualVM (https://visualvm.github.io/) and it did work. > >> > >> It seems that the version bundled with JDK 9 is out of date. > >> > >> Dmitry > >> > >> > >> On 28/12/16 06:47, Frank Yuan wrote: > >>> Hi all > >>> > >>> > >>> > >>> I tried to run jvisualvm in Linux with both b147 and b150(by adding > >>> add-opens option), it always quit quietly after showing a > > splash > >>> screen. Does it still work? > >>> > >>> > >>> > >>> > >>> > >>> Thanks > >>> > >>> Frank > >>> > >
RE: Does jvisualvm work in JDK 9?
Hi Dmitry Thank you very much for your help! Unluckily visualvm doesn't work for 147 and 150 as well, I have to give up using this tool in current stage... Frank > -Original Message- > From: Dmitry Buzdin [mailto:buz...@gmail.com] > Sent: Wednesday, December 28, 2016 4:57 PM > To: Frank Yuan; code-tools-...@openjdk.java.net; > core-libs-dev@openjdk.java.net > Subject: Re: Does jvisualvm work in JDK 9? > > Hi, > > I had a similar problem on Mac. Downloaded the latest version of > VisualVM (https://visualvm.github.io/) and it did work. > > It seems that the version bundled with JDK 9 is out of date. > > Dmitry > > > On 28/12/16 06:47, Frank Yuan wrote: > > Hi all > > > > > > > > I tried to run jvisualvm in Linux with both b147 and b150(by adding > > add-opens option), it always quit quietly after showing a splash > > screen. Does it still work? > > > > > > > > > > > > Thanks > > > > Frank > >
Does jvisualvm work in JDK 9?
Hi all I tried to run jvisualvm in Linux with both b147 and b150(by adding add-opens option), it always quit quietly after showing a splash screen. Does it still work? Thanks Frank
RE: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work anymore and JDK-8114834 LSSerializerImpl always serializes an entity reference node to" &entityName; " even if "entities" property is
Thank you all! I have pushed the change with the license header update.
Frank
-Original Message-
From: Joe Wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work
anymore and JDK-8114834 LSSerializerImpl always
serializes an entity reference node to" &entityName;" even if "entities"
property is false
Hi Frank,
Thanks for the update. It looks good.
The license header for output_html.properties can be updated as the follows:
###
# Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
###
##
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
##
#
# Specify defaults when method="html". These defaults use
output_xml.properties
# as a base.
#
Best,
Joe
On 12/15/16, 2:52 AM, Frank Yuan wrote:
> Hi Christoph
>
> Thank you for the review!
>
> Please check http://cr.openjdk.java.net/~fyuan/8087303_8114834/webrev.01/.
>
> I have updated the code as your comments except output_html.properties, I am
> not sure for the license of this file.
>
> To Joe
> Could you confirm Christoph's comment for output_html.properties?
>
>
> Thanks
> Frank
>
>> -Original Message-
>> From: Langer, Christoph [mailto:christoph.lan...@sap.com]
>> Subject: RE: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work
>> anymore and JDK-8114834 LSSerializerImpl always
> serializes an
>> entity reference node to"&entityName;" even if "entities" property is false
>>
>> Hi Frank,
>>
>> this is awesome.
>>
>> Some minor things I saw while looking through the webrev:
>>
>> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/runtime/AbstractTranslet.java
>> Update copryright year
>> Remove:
>>20 /*
>>21 * $Id: AbstractTranslet.java,v 1.6 2006/06/19 19:49:03 spericas Exp $
>>22 */
>>
>> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java
>> Remove:
>>20 /*
>>21 * $Id: TransformerImpl.java,v 1.10 2007/06/13 01:57:09 joehw Exp $
>>22 */
>>
>> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/SerializerBase.java
>> 462 protected boolean isInEntityRef()
>> 463 {
>> Put the brace on line 462 as well
>>
>> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToHTMLStream.java
>> -> sort import statements
>> Method
>> 773 public void startElement(
>> -> use SAXException without package name. It is imported on top. This can
>> be done in various places throughout the file.
>> 780 //will add extra one if having namespace but no matter
>> -> space between // and will...
>> 821 if ((null != elemContext.m_elementName)
>> -> For me it reads better if it were if ((elemContext.m_elementName != null)
>>
>> 1971 private void initToHTMLStream()
>> 1972 {
>> 1973 //m_elementDesc = null;
>> 1974 m_isprevblock = false;
>> 1975 m_inDTD = false;
>> 1976 //m_isRawStack.clear();
>> 1977 m_omitMetaTag = false;
>> 1978 m_specialEscapeURLs = true;
>> 1979 }
>> -> I guess you could remove the commented statements
>>
>> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToStream.java
>> 116 protected boolean m_ispreserveSpace = false;
>> 117
>> 118
>> -> remove one empty line (117)
>>
>> 1894 m_ispreserve = false;
>> 1895
>> 1896
>> 1897
>> -> newly inserted lines 1896 and 1897 should be removed
>>
>> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/output_html.properties
>> -> mayb
RE: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work anymore and JDK-8114834 LSSerializerImpl always serializes an entity reference node to" &entityName; " even if "entities" property is
Hi Christoph
Thank you for the review!
Please check http://cr.openjdk.java.net/~fyuan/8087303_8114834/webrev.01/.
I have updated the code as your comments except output_html.properties, I am
not sure for the license of this file.
To Joe
Could you confirm Christoph's comment for output_html.properties?
Thanks
Frank
> -Original Message-
> From: Langer, Christoph [mailto:christoph.lan...@sap.com]
> Subject: RE: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work
> anymore and JDK-8114834 LSSerializerImpl always
serializes an
> entity reference node to" &entityName;" even if "entities" property is false
>
> Hi Frank,
>
> this is awesome.
>
> Some minor things I saw while looking through the webrev:
>
> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/runtime/AbstractTranslet.java
> Update copryright year
> Remove:
> 20 /*
> 21 * $Id: AbstractTranslet.java,v 1.6 2006/06/19 19:49:03 spericas Exp $
> 22 */
>
> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java
> Remove:
> 20 /*
> 21 * $Id: TransformerImpl.java,v 1.10 2007/06/13 01:57:09 joehw Exp $
> 22 */
>
> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/SerializerBase.java
> 462 protected boolean isInEntityRef()
> 463 {
> Put the brace on line 462 as well
>
> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToHTMLStream.java
> -> sort import statements
> Method
> 773 public void startElement(
> -> use SAXException without package name. It is imported on top. This can be
> done in various places throughout the file.
> 780 //will add extra one if having namespace but no matter
> -> space between // and will...
> 821 if ((null != elemContext.m_elementName)
> -> For me it reads better if it were if ((elemContext.m_elementName != null)
>
> 1971 private void initToHTMLStream()
> 1972 {
> 1973 //m_elementDesc = null;
> 1974 m_isprevblock = false;
> 1975 m_inDTD = false;
> 1976 //m_isRawStack.clear();
> 1977 m_omitMetaTag = false;
> 1978 m_specialEscapeURLs = true;
> 1979 }
> -> I guess you could remove the commented statements
>
> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToStream.java
> 116 protected boolean m_ispreserveSpace = false;
> 117
> 118
> -> remove one empty line (117)
>
> 1894 m_ispreserve = false;
> 1895
> 1896
> 1897
> -> newly inserted lines 1896 and 1897 should be removed
>
> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/output_html.properties
> -> maybe the Oracle copyright header can be inserted and the "$Id:
> output_html.properties..." part can be removed?
>
> Best regards
> Christoph
>
> > -Original Message-
> > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On
> > Behalf
> > Of Joe Wang
> > Sent: Mittwoch, 14. Dezember 2016 04:09
> > To: Frank Yuan
> > Cc: core-libs-dev@openjdk.java.net
> > Subject: Re: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work
> > anymore and JDK-8114834 LSSerializerImpl always serializes an entity
> > reference node to" &entityName;" even if "entities" property is false
> >
> > Hi Frank,
> >
> > Thanks for the diligent work! I think we've made a great improvement
> > over the PrettyPrint (tremendous ;-) )
> >
> > Could you look into extracting the XML literal strings in the test into
> > plain files (similar to the other 'gold' files)? What I'm hoping to see
> > is that they'd look easily prettier in a text editor, and for that
> > matter, the original xml files were a mess.
> >
> > The tests set PrettyPrint, and by default for html. It would be good to
> > test the cases when it's turned off, that would help verify the
> > non-pretty format was not changed.
> >
> > Thanks,
> > Joe
> >
> > On 12/13/16, 6:55 AM, Frank Yuan wrote:
> > > Hi all
> > >
> > > Webrev http://cr.openjdk.java.net/~fyuan/8087303_8114834/webrev.00/ is
> > for JDK-8087303 and JDK-8114834, I have to combine the fix
> > > because there is some interaction between them.
> > > Bugs: https://bugs.openjdk.java.net/browse/JDK-8087303
> > https://bugs.openjdk.java.net/browse/JDK-8114834
> > >
> > > Besides fixed some issues in xml serializer, I made the following changes
> > > in
> > this patch:
> > > 1. refined the handling for whitespace text
> > > 2. added support for xml:space attribute
> > > 3. changed the default indentation to 4
> > > 4. refined the handling for HTML block element and inline element
> > >
> > > Would you like to have a look at this review?
> > >
> > > Thanks,
> > >
> > > Frank
> > >
> > >
RE: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work anymore and JDK-8114834 LSSerializerImpl always serializes an entity reference node to" &entityName; " even if "entities" property is
Hi Joe Thank you very much for your comments! I have made a new webrev http://cr.openjdk.java.net/~fyuan/8087303_8114834/webrev.01/ as your suggestions, please check. Thanks Frank > -Original Message- > From: Joe Wang [mailto:huizhe.w...@oracle.com] > Subject: Re: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work > anymore and JDK-8114834 LSSerializerImpl always serializes an > entity reference node to" &entityName;" even if "entities" property is false > > Hi Frank, > > Thanks for the diligent work! I think we've made a great improvement > over the PrettyPrint (tremendous ;-) ) > > Could you look into extracting the XML literal strings in the test into > plain files (similar to the other 'gold' files)? What I'm hoping to see > is that they'd look easily prettier in a text editor, and for that > matter, the original xml files were a mess. > > The tests set PrettyPrint, and by default for html. It would be good to > test the cases when it's turned off, that would help verify the > non-pretty format was not changed. > > Thanks, > Joe > > On 12/13/16, 6:55 AM, Frank Yuan wrote: > > Hi all > > > > Webrev http://cr.openjdk.java.net/~fyuan/8087303_8114834/webrev.00/ is for > > JDK-8087303 and JDK-8114834, I have to combine the fix > > because there is some interaction between them. > > Bugs: https://bugs.openjdk.java.net/browse/JDK-8087303 > > https://bugs.openjdk.java.net/browse/JDK-8114834 > > > > Besides fixed some issues in xml serializer, I made the following changes > > in this patch: > > 1. refined the handling for whitespace text > > 2. added support for xml:space attribute > > 3. changed the default indentation to 4 > > 4. refined the handling for HTML block element and inline element > > > > Would you like to have a look at this review? > > > > Thanks, > > > > Frank > > > >
RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work anymore and JDK-8114834 LSSerializerImpl always serializes an entity reference node to" &entityName; " even if "entities" property is fal
Hi all Webrev http://cr.openjdk.java.net/~fyuan/8087303_8114834/webrev.00/ is for JDK-8087303 and JDK-8114834, I have to combine the fix because there is some interaction between them. Bugs: https://bugs.openjdk.java.net/browse/JDK-8087303 https://bugs.openjdk.java.net/browse/JDK-8114834 Besides fixed some issues in xml serializer, I made the following changes in this patch: 1. refined the handling for whitespace text 2. added support for xml:space attribute 3. changed the default indentation to 4 4. refined the handling for HTML block element and inline element Would you like to have a look at this review? Thanks, Frank
RFR JDK-8169948 [JAXP] Update ServiceProviderTest for newDefaultInstance() methods in JAXP factories
Hi all Would you like to review http://cr.openjdk.java.net/~fyuan/8169948/webrev.00/? Bug: https://bugs.openjdk.java.net/browse/JDK-8169948 This test update is because of JDK-8169778 Add new public methods to get new instances of the JAXP factories builtin system-default implementations. DefaultFactoryWrapperTest.java tests a provider that return a custom factory instance wrapping the built-in system-default implementation. Thanks Frank
RE: RFR JDK-8170192 [JAXP] [TESTBUG] test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java should grant permissions to jtreg, javatest, and testng jars
Hi Jon Thank you for your advice! Please check the update http://cr.openjdk.java.net/~fyuan/8170192/webrev.01/ , which contains jcommander.jar and removes the extra blank lines following Christoph's suggestion. Frank From: Jonathan Gibbons [mailto:jonathan.gibb...@oracle.com] Sent: Thursday, November 24, 2016 4:26 AM Subject: Re: RFR JDK-8170192 [JAXP] [TESTBUG] test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java should grant permissions to jtreg, javatest, and testng jars Frank, More recent builds of testng.jar, such as the builds available on Maven, have separated out the jcommander component so that two jar files are required: testng.jar and jcommander.jar. You should consider taking jcommander.jar into account. This will be more important/noticeable to folk outside Oracle who build their own copy of jtreg to use. -- Jon On 11/22/2016 08:41 PM, Frank Yuan wrote: Hi All Would you like to review http://cr.openjdk.java.net/~fyuan/8170192/webrev.00/ <http://cr.openjdk.java.net/%7Efyuan/8170192/webrev.00/> ? Bug: https://bugs.openjdk.java.net/browse/JDK-8170192 This patch is fully same as Daniel provided except a few lines of additional cleaning, thanks to Daniel for providing the patch! Thanks Frank
Did the tests fail due to TestNG dataprovider parameter type check?
Hi Jon
I run the whole jdk regression tests with jtreg-4.2-b03 [1], and then found
lots of tests in different test groups failed with the error message like
following:
test test.java.time.format.TestNumberPrinter.test_pad_ALWAYS(): failure
org.testng.internal.reflect.MethodMatcherException:
Data provider mismatch
Method: test_pad_ALWAYS([Parameter{index=0, type=int, declaredAnnotations=[]},
Parameter{index=1, type=int, declaredAnnotations=[]}, Parameter{index=2,
type=long, declaredAnnotations=[]}, Parameter{index=3, type=java.lang.String,
declaredAnnotations=[]}])
Arguments:
[(java.lang.Integer)1,(java.lang.Integer)1,(java.lang.Integer)-10,null]
at
org.testng.internal.reflect.DataProviderMethodMatcher.getConformingArguments(DataProviderMethodMatcher.java:52)
at org.testng.internal.Invoker.injectParameters(Invoker.java:1228)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1125)
at
org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
at org.testng.TestRunner.privateRun(TestRunner.java:778)
at org.testng.TestRunner.run(TestRunner.java:632)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:366)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:361)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:319)
at org.testng.SuiteRunner.run(SuiteRunner.java:268)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1225)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1150)
at org.testng.TestNG.runSuites(TestNG.java:1075)
at org.testng.TestNG.run(TestNG.java:1047)
at
com.sun.javatest.regtest.TestNGAction$TestNGRunner.main(TestNGAction.java:220)
at
com.sun.javatest.regtest.TestNGAction$TestNGRunner.main(TestNGAction.java:184)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:537)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110)
at java.base/java.lang.Thread.run(Thread.java:844)
This test methd signature is:
public void test_pad_ALWAYS(int minPad, int maxPad, long value, String result)
The provider return the following data
Object[][] provider_pad() {
return new Object[][] {
{1, 1, -10, null},
{1, 1, -9, "9"},
{1, 1, -1, "1"},
...
However, I got message "Cannot find class
java/lang/reflect/JTRegModuleHelper.class to init patch directory" when I run
jtreg, although I didn't find any code related to testng dataprovider in jtreg
source, I would double confirm with you if this is a definite issue or anything
I made incorrectly?
To Christoph
Except above issue, I didn't find any other issue in jdk and langtools repo so
far.
[1]
https://adopt-openjdk.ci.cloudbees.com/job/jtreg/lastSuccessfulBuild/artifact/jtreg-4.2-b03.tar.gz
Thanks
Frank
> -Original Message-
> From: Langer, Christoph [mailto:christoph.lan...@sap.com]
> Sent: Wednesday, November 23, 2016 3:41 PM
> To: Frank Yuan
> Cc: code-tools-...@openjdk.java.net; core-libs-dev@openjdk.java.net;
> jtreg-...@openjdk.java.net; 'Volker Simonis'; 'Daniel Fuchs'
> Subject: RE: Issues running JAXP jtreg tests ("java.lang.RuntimePermission"
> "accessDeclaredMembers")
>
> Thanks, Frank.
>
> we run scheduled jtreg tests for jdk every night so we should have
> encountered issues if there were some. But langtools could be interesting, I
> don't think those run automatically for OpenJDK in our environment.
>
> Best regards
> Christoph
>
> > -Original Message-
> > From: Frank Yuan [mailto:frank.y...@oracle.com]
> > Sent: Mittwoch, 23. November 2016 06:26
> > To: Langer, Christoph ; 'Volker Simonis'
> > ; 'Daniel Fuchs'
> > Cc: code-tools-...@openjdk.java.net; core-libs-dev@openjdk.java.net; jtreg-
> > u...@openjdk.java.net
> > Subject: RE: Issues running JAXP jtreg tests ("java.lang.RuntimePermission"
> > "accessDeclaredMembers")
> >
> > Hi Christoph and Volker
> >
> > I have been launching jdk and langtools tests with the new jtreg, will
> > update to
> > you once I get the result.
> > Hope jaxp test is special because m
RE: Issues running JAXP jtreg tests ("java.lang.RuntimePermission" "accessDeclaredMembers")
Hi Christoph and Volker
I have been launching jdk and langtools tests with the new jtreg, will update
to you once I get the result.
Hope jaxp test is special because most of tests should control the Security
Manager setting inside the test methods.
Thanks
Frank
> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
> Of Langer, Christoph
> Sent: Wednesday, November 23, 2016 3:51 AM
> Subject: RE: Issues running JAXP jtreg tests ("java.lang.RuntimePermission"
> "accessDeclaredMembers")
>
> Thanks a lot Volker and Daniel for the big support to analyze and fix this.
>
> It seems to me that the proposed fix
> (http://cr.openjdk.java.net/~dfuchs/webrev_8170192/webrev.00/ ) looks like
> the best that can be done at the moment. I agree that it would be nicer if
> jtreg would leave the jtreg lib path as java property to be able to elevate
> all of its contents. But the current proposal with a set of TEST_JARS of
> jtreg, javatest and testng is probably sufficient for jaxp testing.
>
> The best thing to find out about other issues with the new version of testng
> would certainly be if Oracle's internal version of jtreg be updated to use
> the latest testng :-)
>
> Best regards
> Christoph
>
>
>
> > -Original Message-
> > From: Volker Simonis [mailto:volker.simo...@gmail.com]
> > Sent: Dienstag, 22. November 2016 20:25
> > To: Daniel Fuchs
> > Cc: Langer, Christoph ; code-tools-
> > d...@openjdk.java.net; core-libs-dev@openjdk.java.net; jtreg-
> > u...@openjdk.java.net
> > Subject: Re: Issues running JAXP jtreg tests
> > ("java.lang.RuntimePermission"
> > "accessDeclaredMembers")
> >
> > Hi Daniel,
> >
> > thanks for your patch!
> >
> > I've meanwhile tried to better understand the root cause of the problem.
> >
> > I don't think that the invocation order of the data provider the
> > listener have changed. If you look at the the two version 6.9.5 and
> > 6.9.13 of testng, the org.testng.TestRunner.run() methods look exactly
> > the same in both 6.9.5 [1] and 6.9.13 [2]:
> >
> > public void run() {
> > beforeRun();
> >
> > try {
> > XmlTest test= getTest();
> > if(test.isJUnit()) {
> > privateRunJUnit(test);
> > }
> > else {
> > privateRun(test);
> > }
> > }
> > finally {
> > afterRun();
> > }
> >
> > I think the problem is in
> > org.testng.internal.ClassHelper.getAvailableMethods() where we testng
> > only collected the methods until (i.e. excluding) java.lang.Object in
> > 6.9.5 [3] but including java.lang.Object in 6.9.13 [4]:
> >
> > 6.9.5
> > =
> > public static Set getAvailableMethods(Class clazz) {
> > Set methods = Sets.newHashSet();
> > methods.addAll(Arrays.asList(clazz.getDeclaredMethods()));
> >
> > Class parent = clazz.getSuperclass();
> > while (Object.class != parent) {
> > methods.addAll(extractMethods(clazz, parent, methods));
> > parent = parent.getSuperclass();
> > }
> >
> > 6.9.13
> > =
> > public static Set getAvailableMethods(Class clazz) {
> > Set methods = Sets.newHashSet();
> > methods.addAll(Arrays.asList(clazz.getDeclaredMethods()));
> >
> > Class parent = clazz.getSuperclass();
> > while (null != parent) {
> > methods.addAll(extractMethods(clazz, parent, methods));
> > parent = parent.getSuperclass();
> > }
> >
> > But java.lang.Object has a different class loader (null) compared to
> > the test class (which is loaded by the application class loader),
> > which leads to the AccessControlException with 6.9.13.
> >
> > As far as I can see, this was changed in testng 6.9.10 [5] to fix
> > https://github.com/cbeust/testng/issues/876
> >
> > This behavior may potentially also affect other tests which are
> > running with a security manger so I'm not sure you fix will help for
> > all of them. And I also wonder why this hasn't been detected by other
> > who run testng with a security manager (but maybe nobody is doing that
> > :)
> >
> > Regards,
> > Volker
> >
> > [1] https://github.com/cbeust/testng/blob/testng-
> > 6.9.5/src/main/java/org/testng/TestRunner.java
> > [2]
> > https://github.com/cbeust/testng/blob/6.9.13/src/main/java/org/testng/TestRu
> > nner.java
> > [3] https://github.com/cbeust/testng/blob/testng-
> > 6.9.5/src/main/java/org/testng/internal/ClassHelper.java
> > [4]
> > https://github.com/cbeust/testng/blob/6.9.13/src/main/java/org/testng/interna
> > l/ClassHelper.java
> > [5]
> > https://github.com/cbeust/testng/pull/886/commits/fefedec34706e40ff2bf780b
> > ff7716fca29daaab
> >
> >
> > On Tue, Nov 22, 2016 at 5:24 PM, Daniel Fuchs
> > wrote:
> > > Hi Christoph,
> > >
> > > I have logged https://bugs.openjdk.java.net/browse/JDK-8170192
> > >
> > > best regards,
> > >
> > > -- daniel
> > >
> > >
> > > On 22/11/16 14:47, Daniel Fuchs wrote:
> > >>
> > >> On 22/11/16 14:43, Langer, Christoph wrote:
> > >>>
> > >>> But, as for fixing with the new testng
RFR JDK-8170192 [JAXP] [TESTBUG] test/javax/xml/jaxp/libs/jaxp/library/JAXPPolicyManager.java should grant permissions to jtreg, javatest, and testng jars
Hi All Would you like to review http://cr.openjdk.java.net/~fyuan/8170192/webrev.00/? Bug: https://bugs.openjdk.java.net/browse/JDK-8170192 This patch is fully same as Daniel provided except a few lines of additional cleaning, thanks to Daniel for providing the patch! Thanks Frank
RFR JDK-8170170 Problem list ExternalEditorTest.java on all platforms
Hi All This is a problem list update in langtools repo: diff -r f4b6b78a1200 test/ProblemList.txt --- a/test/ProblemList.txt Mon Nov 21 12:28:56 2016 -0800 +++ b/test/ProblemList.txt Tue Nov 22 16:34:24 2016 +0800 @@ -38,7 +38,7 @@ jdk/jshell/EditorPadTest.java 8161276windows-allTest set-up cannot press buttons jdk/jshell/ToolBasicTest.java 8139873generic-allJShell tests failing -jdk/jshell/ExternalEditorTest.java 8170108windows-all +jdk/jshell/ExternalEditorTest.java 8169828generic-all Bug: https://bugs.openjdk.java.net/browse/JDK-8170170 Anyone would like to have a look? Thanks Frank
RFR (JAXP) JDK-8169829 ProblemList update for javax/xml/jaxp/isolatedjdk/catalog/PropertiesTest.sh
Hi all This is to correct the bugid in ProblemList.txt, see Bug: https://bugs.openjdk.java.net/browse/JDK-8169829 Would you like to have a review http://cr.openjdk.java.net/~fyuan/8169829/webrev.00/? Thanks, Frank
RE: RFR (JAXP) JDK-8167478 javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with "java.security.AccessControlException: access denied ("java.io.FilePermission" "sko?ice")"
Alright, thank you! Then I will check it in.
Frank
> -Original Message-
> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8167478
> javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with
> "java.security.AccessControlException:
> access denied ("java.io.FilePermission" "sko?ice")"
>
> Looks fine. Paths.get is the way to go.
>
> -Joe
>
> On 10/14/16, 2:28 AM, Frank Yuan wrote:
> > Hi Joe
> >
> > After some testing, I am sure current Windows platforms support Unicode
> > although the default charset is Windows-1252. So now I check it by
> trying Paths.get method, which was also suggested by Max.
> >
> > Please check the new webrev
> > http://cr.openjdk.java.net/~fyuan/8167478/webrev.01/
> >
> > Thanks
> > Frank
> >
> >> -Original Message-
> >> From: Leo Jiang [mailto:li.ji...@oracle.com]
> >> Subject: Re: RFR (JAXP) JDK-8167478
> >> javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with
> >> "java.security.AccessControlException:
> >> access denied ("java.io.FilePermission" "sko?ice")"
> >>
> >> Hi Frank,
> >>
> >> I suggest you to use the following code snippet to make your code more
> >> readable.
> >>
> >> CharsetEncoder charsetEncoder =
> >> Charset.defaultCharset().newEncoder();
> >> boolean b = charsetEncoder.canEncode(path);// alpha
> >>
> >> Any character in Unicode range would be valid as path if current OS
> >> charset is UTF-8 (Unicode series encoding). But if
> >> current encoding setting (code page in Windows) is not UTF, e.g. cp936,
> >> the string in Java with inside UTF-16
> >> representation need to be mapped to the local encoding, that might be
> >> failed.
> >>
> >> After modification, the test should be passed on UTF-8 encoding, and the
> >> encoding which support the character '\u0159'.
> >>
> >> Thanks,
> >> Leo
> >>
> >> On 10/14/2016 06:49 AM, Joe Wang wrote:
> >>> Hi Frank,
> >>>
> >>> Does this work as expected? The method doesn't seem to validate whether a
> >>> character is legal as a file name. For
> >>> example, on Windows, the original char (e.g. \u0159) used in the test is
> >>> legal in a file name, but it didn't pass that
> >>> decode/encode test by the Windows' default encoding (Windows-1252). Does
> >>> that mean this test will no longer run on Windows?
> >>>
> >>> Thanks,
> >>> Joe
> >>>
> >>> On 10/13/16, 2:05 AM, Frank Yuan wrote:
> >>>> Hi all
> >>>>
> >>>>
> >>>>
> >>>> Would you like to review
> >>>> http://cr.openjdk.java.net/~fyuan/8167478/webrev.00/
> >>>> <http://cr.openjdk.java.net/%7Efyuan/8167478/webrev.00/> ?
> >>>>
> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8167478
> >>>>
> >>>>
> >>>>
> >>>> This is a test bug, because Bug6341770.java is invalid when the system
> >>>> environment doesn’t support non-ascii
> >>>> characters, the test will exit immediately in this condition.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>
> >>>>
> >>>> Frank
> >>>>
> >>>>
> >>>>
RE: RFR (JAXP) JDK-8167478 javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with "java.security.AccessControlException: access denied ("java.io.FilePermission" "sko?ice")"
Hi Joe
After some testing, I am sure current Windows platforms support Unicode
although the default charset is Windows-1252. So now I check it by trying
Paths.get method, which was also suggested by Max.
Please check the new webrev http://cr.openjdk.java.net/~fyuan/8167478/webrev.01/
Thanks
Frank
> -Original Message-
> From: Leo Jiang [mailto:li.ji...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8167478
> javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with
> "java.security.AccessControlException:
> access denied ("java.io.FilePermission" "sko?ice")"
>
> Hi Frank,
>
> I suggest you to use the following code snippet to make your code more
> readable.
>
> CharsetEncoder charsetEncoder =
> Charset.defaultCharset().newEncoder();
> boolean b = charsetEncoder.canEncode(path);// alpha
>
> Any character in Unicode range would be valid as path if current OS charset
> is UTF-8 (Unicode series encoding). But if
> current encoding setting (code page in Windows) is not UTF, e.g. cp936, the
> string in Java with inside UTF-16
> representation need to be mapped to the local encoding, that might be failed.
>
> After modification, the test should be passed on UTF-8 encoding, and the
> encoding which support the character '\u0159'.
>
> Thanks,
> Leo
>
> On 10/14/2016 06:49 AM, Joe Wang wrote:
> > Hi Frank,
> >
> > Does this work as expected? The method doesn't seem to validate whether a
> > character is legal as a file name. For
> > example, on Windows, the original char (e.g. \u0159) used in the test is
> > legal in a file name, but it didn't pass that
> > decode/encode test by the Windows' default encoding (Windows-1252). Does
> > that mean this test will no longer run on Windows?
> >
> > Thanks,
> > Joe
> >
> > On 10/13/16, 2:05 AM, Frank Yuan wrote:
> >>
> >> Hi all
> >>
> >>
> >>
> >> Would you like to review
> >> http://cr.openjdk.java.net/~fyuan/8167478/webrev.00/
> >> <http://cr.openjdk.java.net/%7Efyuan/8167478/webrev.00/> ?
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8167478
> >>
> >>
> >>
> >> This is a test bug, because Bug6341770.java is invalid when the system
> >> environment doesn’t support non-ascii
> >> characters, the test will exit immediately in this condition.
> >>
> >>
> >>
> >>
> >>
> >> Thanks,
> >>
> >>
> >>
> >> Frank
> >>
> >>
> >>
RE: RFR (JAXP) JDK-8167478 javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with "java.security.AccessControlException: access denied ("java.io.FilePermission" "sko?ice")"
Thank you, will change it.
Frank
> -Original Message-
> From: Felix Yang [mailto:felix.y...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8167478
> javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with
"java.security.AccessControlException:
> access denied ("java.io.FilePermission" "sko?ice")"
>
> A comment on naming, alpha to ALPHA ?
> -Felix
>
> > 在 2016年10月13日，17:05，Frank Yuan 写道：
> >
> > Hi all
> >
> >
> >
> > Would you like to review
> > http://cr.openjdk.java.net/~fyuan/8167478/webrev.00/ ?
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8167478
> >
> >
> >
> > This is a test bug, because Bug6341770.java is invalid when the system
> > environment doesn't support non-ascii characters, the
test
> > will exit immediately in this condition.
> >
> >
> >
> >
> >
> > Thanks,
> >
> >
> >
> > Frank
> >
> >
> >
RFR (JAXP) JDK-8167478 javax/xml/jaxp/unittest/parsers/Bug6341770.java failed with "java.security.AccessControlException: access denied ("java.io.FilePermission" "sko?ice")"
Hi all Would you like to review http://cr.openjdk.java.net/~fyuan/8167478/webrev.00/ ? Bug: https://bugs.openjdk.java.net/browse/JDK-8167478 This is a test bug, because Bug6341770.java is invalid when the system environment doesn't support non-ascii characters, the test will exit immediately in this condition. Thanks, Frank
RE: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work anymore
Hi Aleksey Thank you very much for your review an comments! > -Original Message- > From: Aleksey Shipilev [mailto:sh...@redhat.com] > Sent: Monday, September 12, 2016 6:45 PM > To: Frank Yuan; 'core-libs-dev' > Subject: Re: RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work > anymore > > On 09/12/2016 12:54 PM, Frank Yuan wrote: > > Would you like to review > > http://cr.openjdk.java.net/~fyuan/8087303/webrev.01/? > > Not an expert in the XML parsing area, so only a cursory code review: > > ToStream.java: > > *) Bad camel casing: > 113 protected boolean m_ispreserveSpace = false; > It's to keep consistent with other members, you know, our jaxp library comes from Apache project. > *) shouldHandleText(chars, start, length) predicates within a method > can be computed once at the beginning? > Yes, thanks > *) I wonder if you want to check "length() > 0" before doing delete in > clearPendingWhiteSpaceText() which is frequently called > Yes, good suggestion, it has better performance! > *) "this." inconsistency here: > > 3240 this.m_preserves.clear(); > 3241 m_ispreserveSpace = false; > 3242 m_preserveSpaces.clear(); > Will change to be consistent, thanks. > Thanks, > -Aleksey Anyway, because Joe suggest to make further enhancement, I will get back after making more changes. Thanks Frank
RFR (JAXP) JDK-8087303 LSSerializer pretty print does not work anymore
Hi Would you like to review http://cr.openjdk.java.net/~fyuan/8087303/webrev.01/? Bug: https://bugs.openjdk.java.net/browse/JDK-8087303 In this patch, I add handling for whitespace text node and support for xml:space attribute in xml serializer. Thanks, Frank
RE: RFR JDK-8165617: Cleanup whitespace in jaxp/test
The normalizer tool will remove any extra LFs, only leave one. But it may be an internal tool, so I think it's not a rule. Thanks Frank > -Original Message- > From: Langer, Christoph [mailto:christoph.lan...@sap.com] > Subject: RE: RFR JDK-8165617: Cleanup whitespace in jaxp/test > > Hi Frank, > > just out of interest: Is it a rule not to have any LFs at the end of java > files? > > Best regards > Christoph > > > -Original Message- > > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On > > Behalf > > Of Frank Yuan > > Sent: Donnerstag, 8. September 2016 06:35 > > To: 'Joe Wang' > > Cc: 'core-libs-dev' > > Subject: RE: RFR JDK-8165617: Cleanup whitespace in jaxp/test > > > > Thank you! Pushed. > > > > Frank > > > > -Original Message- > > From: Joe Wang [mailto:huizhe.w...@oracle.com] > > Sent: Thursday, September 08, 2016 12:11 PM > > To: Frank Yuan > > Cc: 'core-libs-dev' > > Subject: Re: RFR JDK-8165617: Cleanup whitespace in jaxp/test > > > > Hi Frank, > > > > Looks good. Thanks for getting this cleaned up! > > > > Best, > > Joe > > > > On 9/7/16, 8:33 PM, Frank Yuan wrote: > > > Hi > > > > > > This bug is to remove the extra LF from the end of the java files, that > > > will help > > to conform with the normalizer. > > > > > > Anyone would like to take a look? > > http://cr.openjdk.java.net/~fyuan/8165617/webrev.00/ > > > > > > > > > Thanks > > > Frank > > >
RE: RFR JDK-8165617: Cleanup whitespace in jaxp/test
Thank you! Pushed. Frank -Original Message- From: Joe Wang [mailto:huizhe.w...@oracle.com] Sent: Thursday, September 08, 2016 12:11 PM To: Frank Yuan Cc: 'core-libs-dev' Subject: Re: RFR JDK-8165617: Cleanup whitespace in jaxp/test Hi Frank, Looks good. Thanks for getting this cleaned up! Best, Joe On 9/7/16, 8:33 PM, Frank Yuan wrote: > Hi > > This bug is to remove the extra LF from the end of the java files, that will > help to conform with the normalizer. > > Anyone would like to take a look? > http://cr.openjdk.java.net/~fyuan/8165617/webrev.00/ > > > Thanks > Frank >
RFR JDK-8165617: Cleanup whitespace in jaxp/test
Hi This bug is to remove the extra LF from the end of the java files, that will help to conform with the normalizer. Anyone would like to take a look? http://cr.openjdk.java.net/~fyuan/8165617/webrev.00/ Thanks Frank
RE: RFR (JAXP) JDK-8163468: javax/xml/jaxp/unittest/validation/Bug6773084Test.java fails intermittently
> -Original Message- > From: Joe Wang [mailto:huizhe.w...@oracle.com] > Subject: Re: RFR (JAXP) JDK-8163468: > javax/xml/jaxp/unittest/validation/Bug6773084Test.java fails intermittently > > +1, good to identify the cause. Amy helped me to identify it, so thanks to Amy! Frank > > -Joe > > On 8/10/16, 4:17 AM, Daniel Fuchs wrote: > > 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: RFR (JAXP) JDK-8163468: javax/xml/jaxp/unittest/validation/Bug6773084Test.java fails intermittently
Hi Daniel Thank you very much for review! > -Original Message- > From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] > Subject: Re: RFR (JAXP) JDK-8163468: > javax/xml/jaxp/unittest/validation/Bug6773084Test.java fails intermittently > > 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? > I searched yesterday, there is no other such issue in jaxp tests. E.g. common.WarningsTestBase also starts multiple threads in its test method, but it already awaitTermination in the test method. Another one is stream.Bug6688002Test, which uses Thread.join() in its test method. > 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). > Because the System SecurityManager and the java.security.Policy instance are global, the children threads have the same SM and Policy as the main thread. In this test failure, the childrent threads had enough permission until TestNG invoked onFinish(because the code in test method finished), in the call-back function JAXPPolicyManager restored the default SM and Policy, which had no enough permission. In the race condition, children thread got a non-null SecurityManager and then got the default Policy. So the challenge is how to keep the TestPolicy till the children threads finish. I can use a read-write lock, the children threads hold the read lock and JAXPPolicyManager needs to get the write lock before it sets SM and Policy, but this is actually equivalent to Thread.join() in the test method, and I think it's reasonable to wait for all children threads finished in the test method. This test failure is very frequent in Mach5, since current proposed fix is also OK to you and Joe, I pushed it at first :) Thanks Frank > 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 > >
RFR (JAXP) JDK-8163468: javax/xml/jaxp/unittest/validation/Bug6773084Test.java fails intermittently
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: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Thank you! I pushed the change into jaxp repo. Frank > -Original Message- > From: Joe Wang [mailto:huizhe.w...@oracle.com] > Sent: Saturday, August 06, 2016 1:59 AM > To: Frank Yuan > Cc: 'Daniel Fuchs'; 'core-libs-dev' > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > Hi Frank, > > Looks good overall. Thanks for adding runs with/without SM, that's a lot > of tedious work! I wish there's a way to do this in a general > configuration. But it's fine since it does serve the purpose. > > Thanks, > Joe > > On 8/5/16, 6:26 AM, Frank Yuan wrote: > > Hi Joe and Daniel > > > > Please review the update: > > http://cr.openjdk.java.net/~fyuan/8067170/webrev.04/ > > In this version, I > > 1. made all tests run twice, to pass in the different argument to the jtreg > > TestNG test, it has to run in othervm(jaxp test also run > > in othervm before this but it's possible to run in agentvm), however, I > > didn't delete the code supporting agentvm from > > JAXPPolicyManager.java because jtreg may provide more support in agentvm > > mode someday:) > > 2. enabled sm in unittest/catalog/CatalogTest.java as jdk-8161176's > > conclusion > > 3. fixed the bug in runWithTmpPermission methods, Daniel mentioned it but I > > didn't understand at that time :P > > > > Thanks > > Frank > > > >> -Original Message- > >> From: Frank Yuan [mailto:frank.y...@oracle.com] > >> Sent: Thursday, August 04, 2016 6:06 PM > >> To: 'Joe Wang'; 'Daniel Fuchs' > >> Cc: 'core-libs-dev' > >> Subject: RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > >> tests > >> > >> > >> > >>> -Original Message- > >>> From: Joe Wang [mailto:huizhe.w...@oracle.com] > >>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP > >> unit tests > >>> > >>> > >>> On 8/3/16, 3:45 AM, Daniel Fuchs wrote: > >>>> Hi Frank, > >>>> > >>>> I looked at the diffs of the diffs between webrev.02 and webrev.03 and > >>>> it looks good to me. > >>>> > >>>> +1 - provided all tests pass :-) > >>> +1, thanks for re-enabling the tests that had dependencies on 8162848. I > >>> also closed 8162848. > >>> > >>>> But I have the same question than Joe: aren't all the test supposed > >>>> to run twice: once with security manager and once without? > >>>> (except for those test that might explicitly require a security > >> manager) > >>> I agree, all of the tests need to run with and without security manager. > >>> It would be good to combine the runWithSecurityManager and > >>> runWithoutSecurityManager methods into one with a condition that > >>> determines whether a security manager is present. > >>> > >> Alright, I will make the tests run twice. To impl this, I have to add > >> jtreg tags like the following: > >> /* > >> * @test > >> * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest > >> * @run testng/othervm -DrunSecMngr=true common.Bug6350682 > >> * @run testng/othervm common.Bug6350682 > >> */ > >> > >> And modify the Policy class accordingly. I am writing a small program to > >> update the tests, will send the new version tomorrow... > >> > >> Frank > >> > >> > >>> Best, > >>> Joe > >>> > >>>> best regards, > >>>> > >>>> -- daniel > >>>> > >>>> On 03/08/16 11:12, Frank Yuan wrote: > >>>>> Hi Joe > >>>>> > >>>>> Thank you for your review! > >>>>> > >>>>> I updated the tests according to the latest comments as well as had > >>>>> unittest/transform/TransformerTest.java up to date, please check > >>>>> http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/ > >>>>> > >>>>> > >>>>> Thanks > >>>>> Frank > >>>>> > >>>>> -Original Message- > >>>>> From: Joe Wang [mailto:huizhe.w...@oracle.com] > >>>>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP > >>>>> unit tests &
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Joe and Daniel Please review the update: http://cr.openjdk.java.net/~fyuan/8067170/webrev.04/ In this version, I 1. made all tests run twice, to pass in the different argument to the jtreg TestNG test, it has to run in othervm(jaxp test also run in othervm before this but it's possible to run in agentvm), however, I didn't delete the code supporting agentvm from JAXPPolicyManager.java because jtreg may provide more support in agentvm mode someday:) 2. enabled sm in unittest/catalog/CatalogTest.java as jdk-8161176's conclusion 3. fixed the bug in runWithTmpPermission methods, Daniel mentioned it but I didn't understand at that time :P Thanks Frank > -Original Message- > From: Frank Yuan [mailto:frank.y...@oracle.com] > Sent: Thursday, August 04, 2016 6:06 PM > To: 'Joe Wang'; 'Daniel Fuchs' > Cc: 'core-libs-dev' > Subject: RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > > > > -Original Message- > > From: Joe Wang [mailto:huizhe.w...@oracle.com] > > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP > unit tests > > > > > > > > On 8/3/16, 3:45 AM, Daniel Fuchs wrote: > > > Hi Frank, > > > > > > I looked at the diffs of the diffs between webrev.02 and webrev.03 and > > > it looks good to me. > > > > > > +1 - provided all tests pass :-) > > > > +1, thanks for re-enabling the tests that had dependencies on 8162848. I > > also closed 8162848. > > > > > > > > But I have the same question than Joe: aren't all the test supposed > > > to run twice: once with security manager and once without? > > > (except for those test that might explicitly require a security > manager) > > > > I agree, all of the tests need to run with and without security manager. > > It would be good to combine the runWithSecurityManager and > > runWithoutSecurityManager methods into one with a condition that > > determines whether a security manager is present. > > > > Alright, I will make the tests run twice. To impl this, I have to add > jtreg tags like the following: > /* > * @test > * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest > * @run testng/othervm -DrunSecMngr=true common.Bug6350682 > * @run testng/othervm common.Bug6350682 > */ > > And modify the Policy class accordingly. I am writing a small program to > update the tests, will send the new version tomorrow... > > Frank > > > > Best, > > Joe > > > > > > > > best regards, > > > > > > -- daniel > > > > > > On 03/08/16 11:12, Frank Yuan wrote: > > >> Hi Joe > > >> > > >> Thank you for your review! > > >> > > >> I updated the tests according to the latest comments as well as had > > >> unittest/transform/TransformerTest.java up to date, please check > > >> http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/ > > >> > > >> > > >> Thanks > > >> Frank > > >> > > >> -Original Message- > > >> From: Joe Wang [mailto:huizhe.w...@oracle.com] > > >> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP > > >> unit tests > > >> > > >> > > >> > > >> On 8/2/16, 5:30 AM, Daniel Fuchs wrote: > > >>> 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/fu > nctional/org/xml/sax/ptests/XMLReaderNSTableTest.java.fram > > >> > > >> es.html > > >>> > > >>> > > >>> 118 spf.setNamespaceAware(false); > > >> > > >> Not sure why this was changed. > > >>> > > >>> > > >> > http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu > nctional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra > > >> > > >> mes.html > > >>> > > >> > > >> The test itself wasn't very clear on the content it tests. If it only > > >> verifies whether a resolver is used as is said,
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
> -Original Message- > From: Joe Wang [mailto:huizhe.w...@oracle.com] > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > > > On 8/3/16, 3:45 AM, Daniel Fuchs wrote: > > Hi Frank, > > > > I looked at the diffs of the diffs between webrev.02 and webrev.03 and > > it looks good to me. > > > > +1 - provided all tests pass :-) > > +1, thanks for re-enabling the tests that had dependencies on 8162848. I > also closed 8162848. > > > > > But I have the same question than Joe: aren't all the test supposed > > to run twice: once with security manager and once without? > > (except for those test that might explicitly require a security manager) > > I agree, all of the tests need to run with and without security manager. > It would be good to combine the runWithSecurityManager and > runWithoutSecurityManager methods into one with a condition that > determines whether a security manager is present. > Alright, I will make the tests run twice. To impl this, I have to add jtreg tags like the following: /* * @test * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest * @run testng/othervm -DrunSecMngr=true common.Bug6350682 * @run testng/othervm common.Bug6350682 */ And modify the Policy class accordingly. I am writing a small program to update the tests, will send the new version tomorrow... Frank > Best, > Joe > > > > > best regards, > > > > -- daniel > > > > On 03/08/16 11:12, Frank Yuan wrote: > >> Hi Joe > >> > >> Thank you for your review! > >> > >> I updated the tests according to the latest comments as well as had > >> unittest/transform/TransformerTest.java up to date, please check > >> http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/ > >> > >> > >> Thanks > >> Frank > >> > >> -Original Message- > >> From: Joe Wang [mailto:huizhe.w...@oracle.com] > >> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP > >> unit tests > >> > >> > >> > >> On 8/2/16, 5:30 AM, Daniel Fuchs wrote: > >>> 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.fram > >> > >> es.html > >>> > >>> > >>> 118 spf.setNamespaceAware(false); > >> > >> Not sure why this was changed. > >>> > >>> > >> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra > >> > >> mes.html > >>> > >> > >> The test itself wasn't very clear on the content it tests. If it only > >> verifies whether a resolver is used as is said, then this and related > >> changes in ResolverTest and publish.xml are fine. To the extent it > >> verifies, it didn't have to output to a file. > >>> > >>> > >> > http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.h > >> > >> tml > >>> > >> > >> We have to let Frank explain why namespace was turned off for these > >> tests :-) In this case, XMLReaderNSTableTest. In general, I would say > >> enabling security shouldn't change tests or gold files in this case. > >> > >>> > >>> 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? > >> > >> Probably to remove the reference to that particular server? Seems to be > >> fine as a cleanup. Again, it (the original test) only verifies a resolve > >> is used, it didn't even need to write out a file to prove that. > >>> > >>> Also: > >>> > >>> > >>> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Joe
Thank you for your review!
I updated the tests according to the latest comments as well as had
unittest/transform/TransformerTest.java up to date, please check
http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/
Thanks
Frank
-Original Message-
From: Joe Wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
On 8/2/16, 5:30 AM, Daniel Fuchs wrote:
> 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.fram
es.html
>
>
> 118 spf.setNamespaceAware(false);
Not sure why this was changed.
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra
mes.html
>
The test itself wasn't very clear on the content it tests. If it only
verifies whether a resolver is used as is said, then this and related
changes in ResolverTest and publish.xml are fine. To the extent it
verifies, it didn't have to output to a file.
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.h
tml
>
We have to let Frank explain why namespace was turned off for these
tests :-) In this case, XMLReaderNSTableTest. In general, I would say
enabling security shouldn't change tests or gold files in this case.
>
> 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?
Probably to remove the reference to that particular server? Seems to be
fine as a cleanup. Again, it (the original test) only verifies a resolve
is used, it didn't even need to write out a file to prove that.
>
> 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?
Yes, when Security Manager is present, this feature is turned off by
default.
>
>
>
> 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!
Very long patch, indeed! Thanks for the great effort! Very well done
with the unified Policy/Permission management.
Best,
Joe
>
> 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/
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel
> -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 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:
I corrected, cleaned up some code during I addressed the major task, there are
also some others besides you found, I even couldn't
recall all of them now. Sorry it confused you...
>
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/ptests/XMLReaderNSTableTest.java.fram
> es.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.fra
me
> s.html
>
>
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/functional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.h
t
> ml
>
> 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?
>
They are unrelated to sm enabling, XMLReaderNSTableTest was not added with
@Test, so it was never run, after I added, I had to
correct the golden file to let it passed.
spf.setNamespaceAware(false); is redundant, I will replace with a comment line
to state NamespaceAware is false by default.
> 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.
>
>
Yes, I will. Forgot yesterday...
>
>
> 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?
>
The new destination directory which CLITest.java is moved to, contains same
files. So don't need to move 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!
>
Really thank you very much for having reviewed my changes and given me so much
valuable comments!
Frank
> 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 (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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 (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel
Thank you very much for your comments! Please check my reply inline below:
>
> Hi Frank,
>
> Please see my comments inline.
>
> On 27/07/16 10:27, Frank Yuan wrote:
> > Hi Daniel
> >
> > Would you like to have a look at the following changes before I finish all
> > rework?
> > It shows runWithAllPerm, and the handling for user.dir property in
> > FilePolicy.java. The code like runWithTmpPermission is still
> > kept, please ignore them, I will remove them finally.
> >
> > diff -r 1bfe60e61bad test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java
> > --- /dev/null Thu Jan 01 00:00:00 1970 +
> > +++ b/test/javax/xml/jaxp/libs/jaxp/library/FilePolicy.java Wed Jul 27
> > 02:23:58 2016 -0700
> > @@ -0,0 +1,44 @@
...
> > +addPermission(new RuntimePermission("setIO"));
> > +addPermission(new RuntimePermission("setContextClassLoader"));
> > +addPermission(new RuntimePermission("accessDeclaredMembers"));*/
>
> Good to see these permissions are now commented: make sure to remove
> the commented code in the final version.
>
Corrected, thanks!
> > +/*
> > + * set allowAll in current thread context.
> > + */
> > +void setAllowAll(boolean allow) {
> > +policy.setAllowAll(allow);
> > +}
>
> I'd suggest to return the previous value of allowAll here.
> This will be more convenient to restore the previous value
> in the finally block.
> Since setters usually don't return values you might name it e.g.
>
> /**
> * Switches allowAll to the given value and return its
> * previous value in current thread context.
> *
> * Example of use:
> * {@code
> *final boolean before = switchAllowAll(true);
> *try {
> * // do some privileged operation here
> *} finally {
> * // restore allowAll to its previous value
> * switchAllowAll(before);
> *}
> * }
> */
> boolean switchAllowAll(boolean allowAll) {
> // alternatively you could add a switchAllowAll method
> // to TestPolicy
> final boolean before = policy.allowAll();
> policy.setAllowAll(allowAll);
> return before;
> }
>
> [..]
switchAllowAll is more flexible, but I would take only one use case, that is
setAllowAll(true));
try {
do something
} finally {
setAllowAll(false);
}
> > +/**
> > + * Run the RunnableWithException with creating a JAXPPolicyManager and
> > + * assigning temporary permissions. It's not thread-safe to use this
> > + * function.
> > + *
> > + * @param r
> > + *RunnableWithException to execute
> > + * @param ps
> > + *assigning permissions to add.
> > + */
> > +public static void tryRunWithPolicyManager(RunnableWithException r,
> > Permission... ps) throws Exception {
> > +JAXPPolicyManager policyManager =
> > JAXPPolicyManager.getJAXPPolicyManager(true);
> > +if (policyManager != null)
> > +Stream.of(ps).forEach(p -> policyManager.addTmpPermission(p));
> > +try {
> > +r.run();
> > +} finally {
> > +JAXPPolicyManager.teardownPolicyManager();
> > +}
> > +}
>
> This method is suspicious because if there existed a JAXPPolicyManager
> before entering it it will be removed after the method finishes.
> I'd suggest removing this method. You can accomplish the same things
> by calling:
>
> JAXPPolicyManager policyManager =
> JAXPPolicyManager.getJAXPPolicyManager(true);
> runWithTmpPermission(r, ps);
>
> since runWithTmpPermission seems to do things right.
>
JAXPTestUtilities.tryRunWithPolicyManager(Runnable, Permission.)
is used for cases where some test methods need to be run with security
manager and some others need to be run without security manager because
they have different behaviors when having sm or not. E.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/FactoryFindTest.java.sdiff.html.
Such cases must run in single thread, all other cases don't require it, are
thread safe.
> > +
> > +/**
> > + * Run the runnable with assigning temporary permissions. This won't
> > impact
> > + * global policy.
> > + *
> > + * @param r
> > + *Runnable to run
> > + * @param ps
> > + *assigning permissions to add.
> > + */
> > +public static void runWithTmpPerm
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
> > +/*
> > + * Install a SecurityManager along with a default Policy to allow
> > testNG to
> > + * run when there is a security manager.
> > + */
> > +private JAXPPolicyManager() {
> > +// Backing up policy and security manager for restore
> > +policyBackup = Policy.getPolicy();
> > +smBackup = System.getSecurityManager();
> > +
> > +// Set customized policy
> > +setDefaultPermissions();
> > +Policy.setPolicy(policy);
> > +System.setSecurityManager(new SecurityManager());
> > +}
>
> Will the test suite be configured to run with and without
> SecurityManager? It seems to me, with a TestNG test listener, a
> SecurityManager is always installed. I don't seem to see it checks
> whether the test shall run with a SecurityManager.
>
> -Joe
>
It's same as the original code, I think it's to be ready for running in agent
mode.
Frank
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
ng name) {
+return runWithAllPerm(() -> System.getProperty(name));
+}
+
+/**
+ * Set a system property by given system value.
+ *
+ * @param name
+ *System property name to be set.
+ * @param value
+ *System property value to be set.
+ */
+public static void setSystemProperty(String name, String value) {
+runWithAllPerm(() -> System.setProperty(name, value));
+}
+
+/**
+ * Clear a system property.
+ *
+ * @param name
+ *System property name to be cleared.
+ */
+public static void clearSystemProperty(String name) {
+runWithAllPerm(() -> clearSystemProperty(name));
+}
+
+/**
+ * Run the RunnableWithException with assigning temporary permissions. This
+ * won't impact global policy.
+ *
+ * @param r
+ *RunnableWithException to execute
+ * @param ps
+ *assigning permissions to add.
+ */
+public static void tryRunWithTmpPermission(RunnableWithException r,
Permission... ps) throws Exception {
+JAXPPolicyManager policyManager =
JAXPPolicyManager.getJAXPPolicyManager(false);
+List tmpPermissionIndexes = new ArrayList();
+if (policyManager != null)
+Stream.of(ps).forEach(p ->
tmpPermissionIndexes.add(policyManager.addTmpPermission(p)));
+try {
+r.run();
+} finally {
+tmpPermissionIndexes.forEach(index ->
policyManager.removeTmpPermission(index));
+}
+}
+
+@FunctionalInterface
+public interface RunnableWithException {
+void run() throws Exception;
+}
+
+/**
+ * Current test directory.
+ */
+public static final String USER_DIR = getSystemProperty("user.dir") +
FILE_SEP;;
+
}
diff -r 1bfe60e61bad test/javax/xml/jaxp/unittest/transform/Bug6490921.java
--- a/test/javax/xml/jaxp/unittest/transform/Bug6490921.javaMon Apr 04
14:54:38 2016 -0700
+++ b/test/javax/xml/jaxp/unittest/transform/Bug6490921.javaWed Jul 27
02:22:40 2016 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2016, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -23,6 +23,8 @@
package transform;
+import static jaxp.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) {
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 (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Daniel I didn't state clear, actually, I want/wanted to take the absolutely same way as your allowAll except I am going to add a common function(called runWithAllPerm) to run it, no Permission arguments any longer. I will send you a draft to you and Joe to make it clear before I finish all rework. 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 (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi Joe and Daniel
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?
Frank
> -Original Message-
> From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
> Sent: Tuesday, July 26, 2016 8:47 AM
> To: huizhe wang; Frank Yuan
> Cc: 'Amy Lu'; 'core-libs-dev'
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> tests
>
> 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
> -Original Message-
> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit
> tests
>
>
>
> On 7/22/2016 5:53 AM, Daniel Fuchs wrote:
> > 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:
> >>>
> >>>
> >>> 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.
>
> Very true. Some of these permissions came from our standalone JAXP tests
> that were granted to ant and junit.
>
> >> 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"));
>
> These are safe for the current code base. So may be add a note to remind
> for any future changes.
>
> >> 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("modifyThread"));
> >>
> >> How about you think?
>
> My understanding is that you intend to grand specific permissions
> limited to the test that will extend these policies, e.g. FilePolicy. I
> think this is ok and an improvement.
>
> >
> > 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 will reduce the scope of the extra permissions as possible, and make sure the
safety, e.g. surround the user code(i.e. the test
code) which requires a permission explicitly with corresponding permission
assignment
> Yeah, it would be good to make sure a policy is safe with the code. If
> both the test code and the code may need a permission, we may have a
> conflict that we need to solve. It may be good to start with the basic
> permission, and add only if required by the test code, with a note
> preferably noting what exactly is needed. "DefaultFeaturesTest" in this
> regard, doesn't seem to require FilePolicy, but
> CatalogReferCircularityTest requires permission to where the source
> files are located, in this case, it would be good to make it specific.
> For example, instead of being called "FilePolicy", it may be clearer to
> add a parameter so that it's known where the File permission is given
> (e.g. the source dir in this case).
Currently FilePolicy(maybe it's better to rename to TestFilePolicy) has full
access permission to user.dir and read permission to
test.src, I think they belong to user permission, jaxp lib won't doPrivileged
on this.
Btw, it's unable to add parameter in @Listeners({ xxx.class }) unless using
more tricky and complex means.
I believe I will meet problem to identify if a doPrivileged is missing when I
strip the extra permissions and then solve the test
failures case by case, so I would ask you when should we add doPrivileged in
jaxp/jdk library code?
1. Should doPrivileged only for where the permission can't be granted to the
user code, e.g. read some secret system property
2. Or should doPrivileged for where the permission needn't be granted to the
user code, e.g. read some internal property
Frank
>
> Best,
> Joe
>
> >
RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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?
>
> 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
> 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 (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
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 (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
> -Original Message- > From: Amy Lu [mailto:amy...@oracle.com] > Sent: Monday, July 18, 2016 5:42 PM > To: Frank Yuan; 'core-libs-dev' > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > On 7/18/16 5:32 PM, Frank Yuan wrote: > > Btw, I moved internaltest into unittest because it's unnecessary to > > separate them. > > Maybe you'd like to regenerate the webrev with hg move for those files? > Yes, I will. Thank you very much! Frank > Thanks, > Amy
RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Hi
Would you like to review http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/?
Bug: https://bugs.openjdk.java.net/browse/JDK-8067170
In this change, I enabled security manager for JAXP unit tests with improving
the implementation approach and fixing some defects.
Now jaxp tests use TestNG annotation to enable security manager and apply
policies combination, like the following example(this full
example is at:
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/Bug8003147Test.java.html):
@Listeners({ jaxp.library.FilePolicy.class,
jaxp.library.InternalAPIPolicy.class })
public class Bug8003147Test {
There are also 2 additional patterns for some special cases:
1. JAXPTestUtilities.runWithTmpPermission(Runnable, Permission.) is used
for some cases which require their own special
permissions, e.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/transform/CR6551600Test.java.sdiff.html.
2. JAXPTestUtilities.tryRunWithPolicyManager(Runnable, Permission.) is
used for cases where some test methods need to be run
with security manager and some others need to be run without security manager
because they have different behaviors when having sm
or not. E.g.
http://cr.openjdk.java.net/~fyuan/8067170/webrev.00/test/javax/xml/jaxp/unittest/parsers/FactoryFindTest.java.sdiff.html.
Such cases
must run in single thread, all other cases don't require it, are thread safe.
Btw, I moved internaltest into unittest because it's unnecessary to separate
them.
Thanks,
Frank
RFR (JAXP) JDK-8156119: Update ServiceProviderTest for XMLReaderFactory
Hi Would you like to review http://cr.openjdk.java.net/~fyuan/8156119/webrev.00/? Bug: https://bugs.openjdk.java.net/browse/JDK-8156119 This change is to add/update some tests for verifying JDK-8152912: SAX XMLReaderFactory needs to be ServiceLoader compliant, see Joe's RFR mail thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-May/040731.html XMLReaderFactoryTest.java tests 1. XMLReaderFactory can create XMLReader as legacy way, and fix JDK-8015099 Classloading boundary crossing in Java 7 2. XMLReaderFactory can load XMLReader impl by service resource I also updated existing tests to verify XMLReaderFactory can load XMLReader impl from provider module. Thanks, Frank
RE: RFR: 8155600: jaxp.library.TestPolicy should extend the default security policy
Hi Mandy and Joe I have re-fixed the bug with your comment, would you like to check again? http://cr.openjdk.java.net/~fyuan/8155514/webrev.01/ Thanks, Frank > -Original Message- > From: Mandy Chung [mailto:mandy.ch...@oracle.com] > Sent: Friday, April 29, 2016 4:00 AM > To: Frank Yuan > Cc: core-libs-dev ; huizhe wang > ; Xueming Shen > > Subject: Re: RFR: 8155600: jaxp.library.TestPolicy should extend the default > security policy > > Hi Frank, > > The fix looks fine. > > Thanks for verifying it with my patch to deprivilege jdk.charasets. > > Mandy > > > On Apr 28, 2016, at 1:11 AM, Frank Yuan wrote: > > > > Hi Mandy, Joe and all > > > > Would you like to review the fix for bug > > https://bugs.openjdk.java.net/browse/JDK-8155514? > > > > The webrev is at: http://cr.openjdk.java.net/~fyuan/8155514/webrev.00/. > > > > > > Thanks, > > > > Frank
RE: RFR: 8155600: jaxp.library.TestPolicy should extend the default security policy
Thank you! Pushed. Best Regards Frank -Original Message- From: Mandy Chung [mailto:mandy.ch...@oracle.com] Sent: Friday, April 29, 2016 4:00 AM To: Frank Yuan Cc: core-libs-dev ; huizhe wang ; Xueming Shen Subject: Re: RFR: 8155600: jaxp.library.TestPolicy should extend the default security policy Hi Frank, The fix looks fine. Thanks for verifying it with my patch to deprivilege jdk.charasets. Mandy > On Apr 28, 2016, at 1:11 AM, Frank Yuan wrote: > > Hi Mandy, Joe and all > > Would you like to review the fix for bug > https://bugs.openjdk.java.net/browse/JDK-8155514? > > The webrev is at: http://cr.openjdk.java.net/~fyuan/8155514/webrev.00/. > > It’s verified with the source bundle in > http://scaaa637.us.oracle.com/archive/2016/04/2016-04-26-045641.mlchun > g.jdk9-deprivilege > > > Thanks, > > Frank
RFR: 8155600: jaxp.library.TestPolicy should extend the default security policy
Hi Mandy, Joe and all Would you like to review the fix for bug https://bugs.openjdk.java.net/browse/JDK-8155514? The webrev is at: http://cr.openjdk.java.net/~fyuan/8155514/webrev.00/. It's verified with the source bundle in http://scaaa637.us.oracle.com/archive/2016/04/2016-04-26-045641.mlchung.jdk9-deprivilege Thanks, Frank
RE: Review request for JDK-8133924: NPE may be thrown when xsltc select a non-existing node after JDK-8062518
Hi Aleksej and Lance Many thanks for your reminder! I have run JCK test today, all xml tests are passed. To Aleksej Please help me to handle the backport, thank you very much! Best Regards Frank -Original Message- From: Aleksej Efimov [mailto:aleksej.efi...@oracle.com] Sent: Tuesday, August 25, 2015 7:18 PM To: Frank Yuan Cc: 'core-libs-dev' ; 'Joe Wang' Subject: Re: Review request for JDK-8133924: NPE may be thrown when xsltc select a non-existing node after JDK-8062518 Hi Frank, Fix looks good to me (not a reviewer). Did you have a chance to run JCK tests with proposed fix? Also we'll need to backport this change. Do you have a plan to do it? If not then I can handle it. With Best Regards, Aleksej On 08/25/2015 12:43 PM, Frank Yuan wrote: > Hi, Joe and all > > > > Would you like to have a review for bug > https://bugs.openjdk.java.net/browse/JDK-8133924? > > > > The webrev is at: http://cr.openjdk.java.net/~fyuan/8133924/webrev.00/. > > > > I have verified this fix, the corresponding test(it will be pushed > with another test suite) and jaxp test are passed in JPRT. > > > > Thanks, > > > > Frank > > >
Review request for JDK-8133924: NPE may be thrown when xsltc select a non-existing node after JDK-8062518
Hi, Joe and all Would you like to have a review for bug https://bugs.openjdk.java.net/browse/JDK-8133924? The webrev is at: http://cr.openjdk.java.net/~fyuan/8133924/webrev.00/. I have verified this fix, the corresponding test(it will be pushed with another test suite) and jaxp test are passed in JPRT. Thanks, Frank
RE: Review request for JDK-8132660: Change jaxp unit test package name to be different with jaxp api
Hi Joe Thank you very much! Really good advice! I have adjusted as it, please re-check at: http://cr.openjdk.java.net/~fyuan/8132660/webrev.01/ Btw, I only applied this practice on unit test, not for functional test because Tristan still has a functional suite pending, I would unify the functional part sometime after that suite is finished. Best Regards Frank From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Wednesday, August 05, 2015 2:26 AM To: Frank Yuan Cc: 'core-libs-dev' ; 'Alan Bateman' ; 'Jan Lahoda' Subject: Re: Review request for JDK-8132660: Change jaxp unit test package name to be different with jaxp api Hi Frank, That looks fine. However, instead of appending an additional directory "utests", you could make the paths shorter by removing "javax/xml" and "org/w3c" or "org/xml". The short names are good enough to represent the API names, test/javax/xml/jaxp/unittest/parsers for example, is easily recognizable as a package that holds the tests for javax/xml/parsers. JAXP tests currently in the jdk test repositories are named using this approach. Thanks, Joe On 8/3/2015 11:51 PM, Frank Yuan wrote: Hi, Joe and all Would you like to have a review for bug https://bugs.openjdk.java.net/browse/JDK-8132660? This is already on our plan for a while, but I have to finish it now because these unit tests failed with latest Jigsaw build. However I made the changes based on 9-dev repo, I tested them with both Jigsaw build and 9-dev build. The JPRT successful mail is enclosed in the bug. The webrev is at: http://cr.openjdk.java.net/~fyuan/8132660/webrev.00/ <http://cr.openjdk.java.net/%7Efyuan/8132660/webrev.00/> . It seems to be a big change but actually there are only some directories changed. Thanks, Frank
Review request for JDK-8132660: Change jaxp unit test package name to be different with jaxp api
Hi, Joe and all Would you like to have a review for bug https://bugs.openjdk.java.net/browse/JDK-8132660? This is already on our plan for a while, but I have to finish it now because these unit tests failed with latest Jigsaw build. However I made the changes based on 9-dev repo, I tested them with both Jigsaw build and 9-dev build. The JPRT successful mail is enclosed in the bug. The webrev is at: http://cr.openjdk.java.net/~fyuan/8132660/webrev.00/. It seems to be a big change but actually there are only some directories changed. Thanks, Frank
RE: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules
Alright, thanks a lot! :) Best Regards Frank -Original Message- From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] Sent: Thursday, June 25, 2015 2:34 PM To: Frank Yuan; 'huizhe wang' Cc: 'core-libs-dev'; 'Lance Andersen'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline'; 'Alan Bateman' Subject: Re: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules Hi Frank, I could push it for you. -- daniel On 6/25/15 5:05 AM, Frank Yuan wrote: > So, would you like to push the code for me? > > Best Regards > Frank > > -Original Message- > From: huizhe wang [mailto:huizhe.w...@oracle.com] > Sent: Thursday, June 25, 2015 12:57 AM > To: Daniel Fuchs > Cc: Frank Yuan; 'core-libs-dev'; 'Lance Andersen'; 'jibing chen'; > 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) > Iline'; 'Alan Bateman' > Subject: Re: Review request for JDK-8080266: Failed to create CharInfo > due to ResourceBundle update for modules > > +1. > > -Joe > > On 6/24/2015 1:58 AM, Daniel Fuchs wrote: >> Hi Frank, >> >> The proposed changes look good to me. >> >> best regards, >> >> -- daniel >> >> On 24/06/15 09:58, Frank Yuan wrote: >>> Hi, >>> >>> Would you like to have a review for bug >>> https://bugs.openjdk.java.net/browse/JDK-8080266? >>> >>> This bug is caused by jigsaw change, the context class loader can't > load >>> internal resource which is in a named module any more. >>> >>> To fix it, LSSerializerImpl shall invoke >>> ResourceBundle.getBundle(resourceName) instead of >>> ResourceBundle.getBundle(resourceName, locale, classloader) to >>> create CharInfo instance, that will getBundle with the module of the >>> caller(here it's java.xml module). This patch also forces to use the >>> internal XMLEntities.properties because the default xml character > entity >>> reference should always be applied. >>> >>> The webrev is at: >>> http://cr.openjdk.java.net/~fyuan/8080266/webrev.00/ >>> >>> Any comment will be appreciated. >>> >>> Thanks, >>> >>> Frank >>> >
RE: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules
So, would you like to push the code for me? Best Regards Frank -Original Message- From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Thursday, June 25, 2015 12:57 AM To: Daniel Fuchs Cc: Frank Yuan; 'core-libs-dev'; 'Lance Andersen'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline'; 'Alan Bateman' Subject: Re: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules +1. -Joe On 6/24/2015 1:58 AM, Daniel Fuchs wrote: > Hi Frank, > > The proposed changes look good to me. > > best regards, > > -- daniel > > On 24/06/15 09:58, Frank Yuan wrote: >> Hi, >> >> Would you like to have a review for bug >> https://bugs.openjdk.java.net/browse/JDK-8080266? >> >> This bug is caused by jigsaw change, the context class loader can't load >> internal resource which is in a named module any more. >> >> To fix it, LSSerializerImpl shall invoke >> ResourceBundle.getBundle(resourceName) instead of >> ResourceBundle.getBundle(resourceName, locale, classloader) to create >> CharInfo instance, that will getBundle with the module of the >> caller(here it's java.xml module). This patch also forces to use the >> internal XMLEntities.properties because the default xml character entity >> reference should always be applied. >> >> The webrev is at: >> http://cr.openjdk.java.net/~fyuan/8080266/webrev.00/ >> >> Any comment will be appreciated. >> >> Thanks, >> >> Frank >> >
RE: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules
Hi Many thanks for your review! Best Regards Frank -Original Message- From: Daniel Fuchs [mailto:daniel.fu...@oracle.com] Sent: Wednesday, June 24, 2015 4:59 PM To: Frank Yuan; 'core-libs-dev'; 'Joe Wang' Cc: 'Lance Andersen'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline'; 'Alan Bateman' Subject: Re: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules Hi Frank, The proposed changes look good to me. best regards, -- daniel On 24/06/15 09:58, Frank Yuan wrote: > Hi, > > Would you like to have a review for bug > https://bugs.openjdk.java.net/browse/JDK-8080266? > > This bug is caused by jigsaw change, the context class loader can't > load internal resource which is in a named module any more. > > To fix it, LSSerializerImpl shall invoke > ResourceBundle.getBundle(resourceName) instead of > ResourceBundle.getBundle(resourceName, locale, classloader) to create > CharInfo instance, that will getBundle with the module of the > caller(here it's java.xml module). This patch also forces to use the > internal XMLEntities.properties because the default xml character > entity reference should always be applied. > > The webrev is at: http://cr.openjdk.java.net/~fyuan/8080266/webrev.00/ > > Any comment will be appreciated. > > Thanks, > > Frank >
Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules
Hi, Would you like to have a review for bug https://bugs.openjdk.java.net/browse/JDK-8080266? This bug is caused by jigsaw change, the context class loader can't load internal resource which is in a named module any more. To fix it, LSSerializerImpl shall invoke ResourceBundle.getBundle(resourceName) instead of ResourceBundle.getBundle(resourceName, locale, classloader) to create CharInfo instance, that will getBundle with the module of the caller(here it's java.xml module). This patch also forces to use the internal XMLEntities.properties because the default xml character entity reference should always be applied. The webrev is at: http://cr.openjdk.java.net/~fyuan/8080266/webrev.00/ Any comment will be appreciated. Thanks, Frank
Review request for JDK-8080907 - Develop test for Xerces Update: XML Schema Validation
Hi Joe, Lance and all Would you like to have a review for bug: https://bugs.openjdk.java.net/browse/JDK-8080907? I added some new tests and updated some existing tests to add more coverage for JAXP library. The webrev is at http://cr.openjdk.java.net/~fyuan/8080907/webrev.00/, your comment will be appreciated. Best Regards Frank
Review request for JDK-8080906 & JDK-8080908: Develop tests for JEP 255 Xerces Updates
Hi Joe and all I have been working on the test task of JEP 255 Xerces Updates. Here I would invite you to review the changes for 2 bugs of this task: 1. JDK-8080906 Develop test for Xerces Update: DOM L3 Serializer To verify default LSSerializer is Xalan dom 3 serializer 2. JDK-8080908 Develop test for Xerces Update: XPointer To verify Xerces revision 415823: XERCESJ-1134. It should have been fixed in JDK-6794483, however, the test Bug6794483Test.java need to be revised to really cover this bug. I also added module dependencies for exported API in this path, it's recommended for Jigsaw change, refer to https://wiki.se.oracle.com/display/JPG/@modules+in+JTReg+tests The webrev is at http://cr.openjdk.java.net/~fyuan/8080906_8080908/webrev.00/, your comment will be appreciated. Best Regards Frank
Review request for JDK-8078596: jaxp tests failed in modular jdk due to internal class access
Hi, Joe and All This is a test bug on 9-repo-jigsaw, jaxp tests failed due to internal class access. To fix this bug, I made the following changes: 1. moved the tests which test internal APIs to separate directory and added @modules for them 2. for other tests which don't intend to test internal APIs but have some internal API dependencies: 2.1. replaced internal APIs usage with public APIs 2.2. replaced the constants defined in internal classes with local constants As Alan's suggestion, I would push the changes to jdk9/dev and ask an open review. Would you like to take a look? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8078596 webrev: http://cr.openjdk.java.net/~fyuan/8078596/webrev.00/ Thanks, Frank
RE: Review request for JDK-8051559: JAXP function dom tests conversion
Hi Joe Do you have any comment for dom suite co-location? Best Regards Frank From: Frank Yuan [mailto:frank.y...@oracle.com] Sent: Wednesday, March 25, 2015 5:46 PM To: 'huizhe wang'; 'Core-Libs-Dev' Cc: 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' Subject: RE: Review request for JDK-8051559: JAXP function dom tests conversion Hi, Joe and All We are working on moving internal jaxp functional tests to open jdk repo. This is the dom suite. Would you please review these test? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8051559 webrev: http://cr.openjdk.java.net/~fyuan/8051559/webrev.00/ Thanks, Frank
RE: Review request for JDK-8051559: JAXP function dom tests conversion
Hi, Joe and All We are working on moving internal jaxp functional tests to open jdk repo. This is the dom suite. Would you please review these test? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8051559 webrev: http://cr.openjdk.java.net/~fyuan/8051559/webrev.00/ Thanks, Frank
Review request for JDK-8051560: JAXP function astro tests conversion
Hi, Joe and All We are working on moving internal jaxp functional tests to open jdk repo. This is the astro suite. Would you please review these test? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8051560 webrev: http://cr.openjdk.java.net/~fyuan/8051560/webrev.00/ AstroTest is the primary test in this suite, it transforms an xml file(which includes astro data) with several xsl files, sets different filtering condition by these xsl files and different filtering range, finally compares the result with golden files. And there are 5 permutations of InputSourceFactory and FilterFactory(I uses template method pattern for the variant FilterFactoryImpls), each permutation will be applied to above transforming processes. Thanks, Frank
RFR: 8061293: Update javax/xml tests to remove references of jre dir
Hi All Would you like to review the code change for bug: https://bugs.openjdk.java.net/browse/JDK-8061293, this is a Jigsaw related bug, it's to clean up the reference to "jre" dir. webrev: http://cr.openjdk.java.net/~fyuan/8061293/webrev/ Best Regards Frank
RE: Review request for JDK-8051710: Convert JAXP function tests: javax.xml.jaxp14.* to jtreg (testng) tests
Hi Joe I have moved the jaxp14 tests to the corresponding packages, would you like to have a check? http://cr.openjdk.java.net/~fyuan/8051710/webrev.01/ There is an issue, SchemaFactoryTest is merged to validation.SchemaFactoryTest, but the validation package(Lance reviewed it) is waiting to push. I am not sure how to handle for your convenience, so this webrev also includes validation package. Maybe we can wait for pushing validation, or push this patch for both jaxp14 and validation if you think these code change is ok. Best Regards Frank -Original Message- From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Wednesday, January 28, 2015 12:44 PM To: Frank Yuan Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' Subject: Re: Review request for JDK-8051710: Convert JAXP function tests: javax.xml.jaxp14.* to jtreg (testng) tests On 1/27/2015 7:06 PM, Frank Yuan wrote: > Thank you, Joe. > > I will sort the tests as your suggestion, and have 3 questions to confirm > with you: > 1. Should I also sort the gap tests? > 2. Should I put astro suite at side of auctionportal? Ah, in that case, you may put gap tests alongside the auctionportal as well. > 3. If a package has very few test, e.g. I may put XMLEventFactoryTest and > something else in javax.xml.stream.ptest, is it ok? (I would rename > XMLEventFactoryTest as its small coverage) Sounds good to me. Thanks, Joe > > Best Regards > Frank > > > -Original Message- > From: huizhe wang [mailto:huizhe.w...@oracle.com] > Sent: Wednesday, January 28, 2015 10:27 AM > To: Frank Yuan > Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo Galimberti'; > sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' > Subject: Re: Review request for JDK-8051710: Convert JAXP function tests: > javax.xml.jaxp14.* to jtreg (testng) tests > > Hi Frank, > > Nice refactoring the original tests, esp. the TransformerTest! > > jaxp14 is legacy in the jaxp standalone world. While we are at this, > you may > want to move these tests to their relevant packages since in the JDK world, > jaxp14 is no longer relevant (jaxp 1.4 was integrated into JDK 6). As > you've already split FactoryTest into various Factory tests, you may find > them a bit thin in terms of test coverage now that they are named > *FactoryTest since they cover just one of the two newInstance methods. > I would think it makes sense to move them into / combine with the > Factory tests of their own packages. > > Thanks, > Joe > > On 1/27/2015 1:09 AM, Frank Yuan wrote: >> Hi, Joe, Lance and All >> >> We are working on moving internal jaxp functional tests to open jdk > repo. >> This is the jaxp14 suite. Would you please review these test? Any > comment >> will be appreciated. >> >> bug: https://bugs.openjdk.java.net/browse/JDK-8051710 >> webrev: http://cr.openjdk.java.net/~fyuan/8051710/webrev.00/ >> >> >> Thanks, >> >> Frank >> >
RE: Review request for JDK-8052401: JAXP function gap tests conversion
Thank you, Joe. I have moved gatest to test.gatest, would you like to check again? http://cr.openjdk.java.net/~fyuan/8052401/webrev.01/ If the code has no problem, would you like to be the sponsor? Best Regards Frank -Original Message- From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Wednesday, January 28, 2015 12:36 PM To: Frank Yuan Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' Subject: Re: Review request for JDK-8052401: JAXP function gap tests conversion Looks good. Joe On 1/27/2015 1:17 AM, Frank Yuan wrote: > Hi, Joe, Lance and All > > We are working on moving internal jaxp functional tests to open jdk repo. > This is the gaptest suite. Would you please review these test? Any comment > will be appreciated. > > bug: https://bugs.openjdk.java.net/browse/JDK-8052401 > webrev: http://cr.openjdk.java.net/~fyuan/8052401/webrev.00/ > > > Thanks, > > Frank >
RE: Review request for JDK-8051710: Convert JAXP function tests: javax.xml.jaxp14.* to jtreg (testng) tests
Thank you, Joe. I will sort the tests as your suggestion, and have 3 questions to confirm with you: 1. Should I also sort the gap tests? 2. Should I put astro suite at side of auctionportal? 3. If a package has very few test, e.g. I may put XMLEventFactoryTest and something else in javax.xml.stream.ptest, is it ok? (I would rename XMLEventFactoryTest as its small coverage) Best Regards Frank -Original Message- From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Wednesday, January 28, 2015 10:27 AM To: Frank Yuan Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' Subject: Re: Review request for JDK-8051710: Convert JAXP function tests: javax.xml.jaxp14.* to jtreg (testng) tests Hi Frank, Nice refactoring the original tests, esp. the TransformerTest! jaxp14 is legacy in the jaxp standalone world. While we are at this, you may want to move these tests to their relevant packages since in the JDK world, jaxp14 is no longer relevant (jaxp 1.4 was integrated into JDK 6). As you've already split FactoryTest into various Factory tests, you may find them a bit thin in terms of test coverage now that they are named *FactoryTest since they cover just one of the two newInstance methods. I would think it makes sense to move them into / combine with the Factory tests of their own packages. Thanks, Joe On 1/27/2015 1:09 AM, Frank Yuan wrote: > Hi, Joe, Lance and All > > We are working on moving internal jaxp functional tests to open jdk repo. > This is the jaxp14 suite. Would you please review these test? Any comment > will be appreciated. > > bug: https://bugs.openjdk.java.net/browse/JDK-8051710 > webrev: http://cr.openjdk.java.net/~fyuan/8051710/webrev.00/ > > > Thanks, > > Frank >
Review request for JDK-8052401: JAXP function gap tests conversion
Hi, Joe, Lance and All We are working on moving internal jaxp functional tests to open jdk repo. This is the gaptest suite. Would you please review these test? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8052401 webrev: http://cr.openjdk.java.net/~fyuan/8052401/webrev.00/ Thanks, Frank
RE: Review request for JDK-8051710: Convert JAXP function tests: javax.xml.jaxp14.* to jtreg (testng) tests
Hi, Joe, Lance and All We are working on moving internal jaxp functional tests to open jdk repo. This is the jaxp14 suite. Would you please review these test? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8051710 webrev: http://cr.openjdk.java.net/~fyuan/8051710/webrev.00/ Thanks, Frank
Review request for JDK-8051709: Convert JAXP function tests: javax.xml.datatype to jtreg (testng) tests
Hi, Joe and All We are working on moving internal jaxp functional tests to open jdk repo. This is the datatype suite. Would you please review these test? Any comment will be appreciated. bug: https://bugs.openjdk.java.net/browse/JDK-8051709 webrev: http://cr.openjdk.java.net/~fyuan/8051709/webrev.00/ Thanks, Frank
