Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
adjust order of VM options
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/900c28c0..bfa955ad
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=00-01
Stats: 59 lines in 18 files changed: 8 ins; 6 del; 45 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v2]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: feedback from Sean, Phil and Alan - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/eb6c566f..bb73093a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=00-01 Stats: 9 lines in 3 files changed: 4 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 18:38:52 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/java/security/AccessController.java line 877:
>>
>>> 875: @CallerSensitive
>>> 876: public static T doPrivileged(PrivilegedExceptionAction
>>> action,
>>> 877: @SuppressWarnings("removal")
>>> AccessControlContext context, Permission... perms)
>>
>> you might find it easier if you put the Permissions parameter on a new line.
>> There are several places in AccessController where the same thing happens.
>
> I'll try to update my auto-annotating program.
Turns out this only happens in this class:
$ rg '^\s*@SuppressWarnings("removal").*?,.'
src/java.base/share/classes/java/security/AccessController.java:449:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms)
{
src/java.base/share/classes/java/security/AccessController.java:514:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms)
{
src/java.base/share/classes/java/security/AccessController.java:877:
@SuppressWarnings("removal") AccessControlContext
context, Permission... perms)
I'll fix them manually.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 17:36:55 GMT, Alan Bateman wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>
> src/java.base/share/classes/java/security/AccessController.java line 877:
>
>> 875: @CallerSensitive
>> 876: public static T doPrivileged(PrivilegedExceptionAction
>> action,
>> 877: @SuppressWarnings("removal")
>> AccessControlContext context, Permission... perms)
>
> you might find it easier if you put the Permissions parameter on a new line.
> There are several places in AccessController where the same thing happens.
My rule is that a new line is added if there's only whitespaces before the
insert position and the previous line does not end with a comma. In this case,
unless I move `Permission... perms` onto a new line that an annotation on a new
line looks like covering both `context` and `perms`.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, i18n, rmi, javadoc, swing, 2d, security,
> hotspot-runtime, nio, xml, beans, ~~core-libs~~, ~~net~~, compiler,
> ~~hotspot-gc~~). Mostly the rule is the same as how Skara adds labels, but
> there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
I reviewed non-client areas. Looks okay.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote:
> Please review this implementation of [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> The code change is divided into 3 commits. Please review them one by one.
>
> 1.
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
> The essential change for this JEP, including the `@Deprecate` annotations
> and spec change. It also update the default value of the
> `java.security.manager` system property to "disallow", and necessary test
> change following this update.
> 2.
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
> Manual changes to several files so that the next commit can be generated
> programatically.
> 3.
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
> Automatic changes to other source files to avoid javac warnings on
> deprecation for removal
>
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
> generated programmatically, see the comment below for more details. If you
> are only interested in a portion of the 3rd commit and would like to review
> it as a separate file, please comment here and I'll generate an individual
> webrev.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> any file to minimize unnecessary merge conflict.
>
> Furthermore, since the default value of `java.security.manager` system
> property is now "disallow", most of the tests calling
> `System.setSecurityManager()` need to launched with
> `-Djava.security.manager=allow`. This is covered in a different PR at
> https://github.com/openjdk/jdk/pull/4071.
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/security/AccessController.java line 877:
> 875: @CallerSensitive
> 876: public static T doPrivileged(PrivilegedExceptionAction action,
> 877: @SuppressWarnings("removal")
> AccessControlContext context, Permission... perms)
you might find it easier if you put the Permissions parameter on a new line.
There are several places in AccessController where the same thing happens.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. java.xml changes look good. Thanks. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Reviewed i18n/java.time/charset. They all look good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. The changes in the net area look fine. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 15:19:21 GMT, Alan Bateman wrote: >> It includes both: >>  > > Thanks for checking, I assumed that was the case so wondering if it should be > dropped from the deprecation text to avoid the repeated sentence. My feeling is that it is better to be more specific than the generic removal text as this is the most significant class that is being deprecated for removal. Also, this says "the Security Manager" (note the space between) which really encompasses more than the `java.lang.SecurityManager` API and which is explained more completely in the JEP. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 12:42:08 GMT, Sean Mullan wrote: >> src/java.base/share/classes/java/lang/SecurityManager.java line 315: >> >>> 313: * >>> 314: * @since 1.0 >>> 315: * @deprecated The Security Manager is deprecated and subject to >>> removal in a >> >> Javadoc will prefix, in bold, "Deprecated, for removal: This API element is >> subject to removal in a future version". I'm just wondering if the sentence >> will be repeated here, can you check? > > It includes both: >  Thanks for checking, I assumed that was the case so wondering if it should be dropped from the deprecation text to avoid the repeated sentence. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Tue, 18 May 2021 05:48:56 GMT, Alan Bateman wrote: > The changes look okay but a bit inconsistent on where -Djava...=allow is > inserted for tests that already set other system properties or other > parameters. Not a correctness issue, just looks odd in several places, e.g. > > test/jdk/java/lang/System/LoggerFinder/BaseLoggerFinderTest/BaseLoggerFinderTest.java > - the tests sets the system properties after -Xbootclasspath/a: but the > change means it sets properties before and after. > > test/jdk/java/lang/Class/getResource/ResourcesTest.java - you've added it in > the middle of the module and class path parameters. > > For uses using ProcessTools then it seems to be very random. I've updated the 3 cases you mentioned and will go through more. Yes it looks good to group system property settings together. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Tue, 18 May 2021 11:12:00 GMT, Daniel Fuchs wrote:
>> Please review the test changes for [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> With JEP 411 and the default value of `-Djava.security.manager` becoming
>> `disallow`, tests calling `System.setSecurityManager()` need
>> `-Djava.security.manager=allow` when launched. This PR covers such changes
>> for tier1 to tier3 (except for the JCK tests).
>>
>> To make it easier to focus your review on the tests in your area, this PR is
>> divided into multiple commits for different areas (~~serviceability~~,
>> ~~hotspot-compiler~~, i18n, rmi, javadoc, swing, 2d, security,
>> hotspot-runtime, nio, xml, beans, ~~core-libs~~, ~~net~~, compiler,
>> ~~hotspot-gc~~). Mostly the rule is the same as how Skara adds labels, but
>> there are several small tweaks:
>>
>> 1. When a file is covered by multiple labels, only one is chosen. I make my
>> best to avoid putting too many tests into `core-libs`. If a file is covered
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
>> `rmi` commit.
>> 4. One file not covered by any label --
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
>> the `swing` commit.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> all files to minimize unnecessary merge conflict.
>>
>> Please note that this PR can be integrated before the source changes for JEP
>> 411, as the possible values of this system property was already defined long
>> time ago in JDK 9.
>>
>> Most of the change in this PR is a simple adding of
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and
>> we add the line.
>>
>> There are several tests that launch another Java process that needs to call
>> the `System.setSecurityManager()` method, and the system property is added
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test
>> is a shell test).
>>
>> 3 langtools tests are added into problem list due to
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>>
>> 2 SQL tests are moved because they need different options on the `@run` line
>> but they are inside a directory that has a `TEST.properties`:
>>
>> rename test/jdk/java/sql/{testng/test/sql/othervm =>
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>> ```
>>
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
> line 34:
>
>> 32: * @run clean NotificationEmissionTest
>> 33: * @run build NotificationEmissionTest
>> 34: * @run main NotificationEmissionTest 1
>
> This test case (NotificationEmissionTest 1) calls
> `System.setProperty("java.security.policy", policyFile);` - even though it
> doesn't call System.setSecurityManager(); Should the @run command line for
> test case 1 be modified as well?
Or maybe the policy setting line should only be called when a security manager
is set? IIRC jtreg is able to restore system properties even in agentvm mode.
So maybe this really doesn't matter. We just want to modify as little as
possible in this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 06:31:06 GMT, Alan Bateman wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. > > src/java.base/share/classes/java/lang/SecurityManager.java line 315: > >> 313: * >> 314: * @since 1.0 >> 315: * @deprecated The Security Manager is deprecated and subject to >> removal in a > > Javadoc will prefix, in bold, "Deprecated, for removal: This API element is > subject to removal in a future version". I'm just wondering if the sentence > will be repeated here, can you check? It includes both:  - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas. Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
I looked at serviceability, net, and corelibs. The changes look good to me -
though I have one question about the NotificationEmissionTest. I believe that
regardless of whether the new property is needed, test case 1 should be run in
/othervm too.
test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
line 34:
> 32: * @run clean NotificationEmissionTest
> 33: * @run build NotificationEmissionTest
> 34: * @run main NotificationEmissionTest 1
This test case (NotificationEmissionTest 1) calls
`System.setProperty("java.security.policy", policyFile);` - even though it
doesn't call System.setSecurityManager(); Should the @run command line for test
case 1 be modified as well?
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Integrated: 8266897: com/sun/net/httpserver/FilterTest.java fails intermittently with AssertionError
On Mon, 17 May 2021 09:53:06 GMT, Julia Boes wrote: > The tests for `afterHandler` filters expect the filter operation to happen > before the assertion. Very rarely this is not the case, the calling thread > reads the response before the filter operation is executed, so the test value > is not set before it is asserted. > This fix uses a `CompletableFuture` to ensure the test value has been updated > before it is asserted. > > While here, the change increases the thread-safety of the filter state of a > context. This pull request has now been integrated. Changeset: 894547d2 Author:Julia Boes URL: https://git.openjdk.java.net/jdk/commit/894547d2c102dcbe1f9ec8a1edb11c6b31e4270e Stats: 31 lines in 3 files changed: 3 ins; 4 del; 24 mod 8266897: com/sun/net/httpserver/FilterTest.java fails intermittently with AssertionError Reviewed-by: chegar, dfuchs, michaelm - PR: https://git.openjdk.java.net/jdk/pull/4050
Re: RFR: 8266897: com/sun/net/httpserver/FilterTest.java fails intermittently with AssertionError
On Mon, 17 May 2021 09:53:06 GMT, Julia Boes wrote: > The tests for `afterHandler` filters expect the filter operation to happen > before the assertion. Very rarely this is not the case, the calling thread > reads the response before the filter operation is executed, so the test value > is not set before it is asserted. > This fix uses a `CompletableFuture` to ensure the test value has been updated > before it is asserted. > > While here, the change increases the thread-safety of the filter state of a > context. LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4050
