Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов wrote: >> Non-static classes hold a link to their parent classes, which in many cases >> can be avoided. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8261880: Remove static from declarations of Holder nested classes Any more comments? - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 04:05:23 GMT, Phil Race wrote: >> By converting JDK-8267432 to a bug, there is an extra benefit that we can >> fix it after RDP. So I'll convert it now. > > That is unfortunate, but nonetheless I think required to be done. > We have acknowledeged this can't reasonably be called an RFE, so the JEP is > introducing bugs/technical debt/call it what you will. This should generally > be part of a sandbox for the JEP and fixed before integration of the JEP. > From my point of view it is a blocker. The JEP isn't PTT for 17 so there's plenty of time isn't there ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 02:09:57 GMT, Weijun Wang wrote: >> I can make it a bug. >> >> I don't want to do it here is because it involves indefinite amount of >> manual work and we will see everyone having their preferences. The more time >> we spend on this PR the more likely there will be merge conflict. There are >> just too many files here. >> >> And no matter if we include that change in this PR or after it, it will be >> after the automatic conversion. In fact, the data about which cases are more >> worth fixing come from the automatic conversion itself. Also, as the manual >> work will be done part by part, if the automatic conversion is after it, >> there will be rounds and rounds of history rewriting and force push. This is >> quite unfriendly to the reviewers. >> >> Altogether, there are 117 class-level annotations. Unfortunately, >> `java.awt.Component` is the one with the biggest class -- estimated number >> of bytes that the annotation covers is over 380K. In the client area, the >> 2nd place is `java.awt.Container`, and then we have >> `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which >> are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller >> than 10KB. > > By converting JDK-8267432 to a bug, there is an extra benefit that we can fix > it after RDP. So I'll convert it now. That is unfortunate, but nonetheless I think required to be done. We have acknowledeged this can't reasonably be called an RFE, so the JEP is introducing bugs/technical debt/call it what you will. This should generally be part of a sandbox for the JEP and fixed before integration of the JEP. >From my point of view it is a blocker. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 02:06:46 GMT, Weijun Wang wrote: >> Well .. as an enhancement (P3 or otherwise) I can see it being dropped and >> definitely if it misses the fork, >> and I don't get why you can't do it here. And if it isn't done the JEP isn't >> really ready. >> I already pasted the patch for Component.java and it took about 60 seconds >> to do that. >> Then yes, you would have to deal with the fact that now you need to reapply >> your automated tool to the file but this is just work you'd have to have >> done anyway if it was already refactored. >> >> I only *noticed* Component and Container. And stopped there to raise the >> question. How many more cases are there ? > > I can make it a bug. > > I don't want to do it here is because it involves indefinite amount of manual > work and we will see everyone having their preferences. The more time we > spend on this PR the more likely there will be merge conflict. There are just > too many files here. > > And no matter if we include that change in this PR or after it, it will be > after the automatic conversion. In fact, the data about which cases are more > worth fixing come from the automatic conversion itself. Also, as the manual > work will be done part by part, if the automatic conversion is after it, > there will be rounds and rounds of history rewriting and force push. This is > quite unfriendly to the reviewers. > > Altogether, there are 117 class-level annotations. Unfortunately, > `java.awt.Component` is the one with the biggest class -- estimated number of > bytes that the annotation covers is over 380K. In the client area, the 2nd > place is `java.awt.Container`, and then we have `sun.font.SunFontManager`, > `java.awt.Window`, and `java.awt.Toolkit` which are over 100KB, and other 25 > classes over 10KB, and other 11 classes smaller than 10KB. By converting JDK-8267432 to a bug, there is an extra benefit that we can fix it after RDP. So I'll convert it now. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 23:50:04 GMT, Phil Race wrote: >> I just made it P3 (P4 was the default value), and I will target it to 17 >> once JEP 411 is targeted 17. But I think it's probably not a good idea to >> include it inside *this* PR. There are some middle ground where it's >> debatable if a change is worth doing (Ex: which is uglier between an >> a-liitle-faraway-annotation and a temporary variable?) so it's not obvious >> what the scope of the refactoring can be. Thus it will be divided into >> smaller sub-tasks. It's not totally impossible that part of the work will be >> delayed to next release. > > Well .. as an enhancement (P3 or otherwise) I can see it being dropped and > definitely if it misses the fork, > and I don't get why you can't do it here. And if it isn't done the JEP isn't > really ready. > I already pasted the patch for Component.java and it took about 60 seconds to > do that. > Then yes, you would have to deal with the fact that now you need to reapply > your automated tool to the file but this is just work you'd have to have done > anyway if it was already refactored. > > I only *noticed* Component and Container. And stopped there to raise the > question. How many more cases are there ? I can make it a bug. I don't want to do it here is because it involves indefinite amount of manual work and we will see everyone having their preferences. The more time we spend on this PR the more likely there will be merge conflict. There are just too many files here. And no matter if we include that change in this PR or after it, it will be after the automatic conversion. In fact, the data about which cases are more worth fixing come from the automatic conversion itself. Also, as the manual work will be done part by part, if the automatic conversion is after it, there will be rounds and rounds of history rewriting and force push. This is quite unfriendly to the reviewers. Altogether, there are 117 class-level annotations. Unfortunately, `java.awt.Component` is the one with the biggest class -- estimated number of bytes that the annotation covers is over 380K. In the client area, the 2nd place is `java.awt.Container`, and then we have `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller than 10KB. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 22:14:20 GMT, Weijun Wang wrote: >> I don't think it is a separate P4 enhancement (?) that someone will (maybe) >> do next release. >> I think it should all be taken care of as part of this proposed change. > > I just made it P3 (P4 was the default value), and I will target it to 17 once > JEP 411 is targeted 17. But I think it's probably not a good idea to include > it inside *this* PR. There are some middle ground where it's debatable if a > change is worth doing (Ex: which is uglier between an > a-liitle-faraway-annotation and a temporary variable?) so it's not obvious > what the scope of the refactoring can be. Thus it will be divided into > smaller sub-tasks. It's not totally impossible that part of the work will be > delayed to next release. Well .. as an enhancement (P3 or otherwise) I can see it being dropped and definitely if it misses the fork, and I don't get why you can't do it here. And if it isn't done the JEP isn't really ready. I already pasted the patch for Component.java and it took about 60 seconds to do that. Then yes, you would have to deal with the fact that now you need to reapply your automated tool to the file but this is just work you'd have to have done anyway if it was already refactored. I only *noticed* Component and Container. And stopped there to raise the question. How many more cases are there ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 22:04:57 GMT, Phil Race wrote: >> Correct, there are ways to modify the code to make it more >> annotation-friendly. We thought about whether it's good to do it before >> adding the annotations or after it. Our decision now is to do it after >> because it will be more easy to see why it's necessary and we can take time >> to do them little by little. A new enhancement at >> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed. > > I don't think it is a separate P4 enhancement (?) that someone will (maybe) > do next release. > I think it should all be taken care of as part of this proposed change. I just made it P3 (P4 was the default value), and I will target it to 17 once JEP 411 is targeted 17. But I think it's probably not a good idea to include it inside *this* PR. There are some middle ground where it's debatable if a change is worth doing (Ex: which is uglier between an a-liitle-faraway-annotation and a temporary variable?) so it's not obvious what the scope of the refactoring can be. Thus it will be divided into smaller sub-tasks. It's not totally impossible that part of the work will be delayed to next release. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 21:53:35 GMT, Weijun Wang wrote:
>> That's a sad limitation of the annotation stuff then, but I don't think that
>> it is insurmountable.
>> You can define a static private method to contain this and call it from the
>> static initializer block.
>> Much better than applying the annotation to an entire class.
>>
>> --- a/src/java.desktop/share/classes/java/awt/Component.java
>> +++ b/src/java.desktop/share/classes/java/awt/Component.java
>> @@ -618,6 +618,17 @@ public abstract class Component implements
>> ImageObserver, MenuContainer,
>> */
>> static boolean isInc;
>> static int incRate;
>> +
>> +private static void initIncRate() {
>> +String s = java.security.AccessController.doPrivileged(
>> + new
>> GetPropertyAction("awt.image.incrementaldraw"));
>> +isInc = (s == null || s.equals("true"));
>> +
>> +s = java.security.AccessController.doPrivileged(
>> + new GetPropertyAction("awt.image.redrawrate"));
>> +incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +}
>> +
>> static {
>> /* ensure that the necessary native libraries are loaded */
>> Toolkit.loadLibraries();
>> @@ -625,14 +636,7 @@ public abstract class Component implements
>> ImageObserver, MenuContainer,
>> if (!GraphicsEnvironment.isHeadless()) {
>> initIDs();
>> }
>> -
>> -String s = java.security.AccessController.doPrivileged(
>> - new
>> GetPropertyAction("awt.image.incrementaldraw"));
>> -isInc = (s == null || s.equals("true"));
>> -
>> -s = java.security.AccessController.doPrivileged(
>> -new
>> GetPropertyAction("awt.image.redrawrate"));
>> -incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +initIncRate();
>> }
>
> Correct, there are ways to modify the code to make it more
> annotation-friendly. We thought about whether it's good to do it before
> adding the annotations or after it. Our decision now is to do it after
> because it will be more easy to see why it's necessary and we can take time
> to do them little by little. A new enhancement at
> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.
I don't think it is a separate P4 enhancement (?) that someone will (maybe) do
next release.
I think it should all be taken care of as part of this proposed change.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 19:31:24 GMT, Phil Race wrote:
>> This happens when a deprecated method is called inside a static block. The
>> annotation can only be added to a declaration and here it must be the whole
>> class. The call in this file is
>>
>> s = java.security.AccessController.doPrivileged(
>> new
>> GetPropertyAction("awt.image.redrawrate"));
>
> That's a sad limitation of the annotation stuff then, but I don't think that
> it is insurmountable.
> You can define a static private method to contain this and call it from the
> static initializer block.
> Much better than applying the annotation to an entire class.
>
> --- a/src/java.desktop/share/classes/java/awt/Component.java
> +++ b/src/java.desktop/share/classes/java/awt/Component.java
> @@ -618,6 +618,17 @@ public abstract class Component implements
> ImageObserver, MenuContainer,
> */
> static boolean isInc;
> static int incRate;
> +
> +private static void initIncRate() {
> +String s = java.security.AccessController.doPrivileged(
> + new
> GetPropertyAction("awt.image.incrementaldraw"));
> +isInc = (s == null || s.equals("true"));
> +
> +s = java.security.AccessController.doPrivileged(
> + new GetPropertyAction("awt.image.redrawrate"));
> +incRate = (s != null) ? Integer.parseInt(s) : 100;
> +}
> +
> static {
> /* ensure that the necessary native libraries are loaded */
> Toolkit.loadLibraries();
> @@ -625,14 +636,7 @@ public abstract class Component implements
> ImageObserver, MenuContainer,
> if (!GraphicsEnvironment.isHeadless()) {
> initIDs();
> }
> -
> -String s = java.security.AccessController.doPrivileged(
> - new
> GetPropertyAction("awt.image.incrementaldraw"));
> -isInc = (s == null || s.equals("true"));
> -
> -s = java.security.AccessController.doPrivileged(
> -new
> GetPropertyAction("awt.image.redrawrate"));
> -incRate = (s != null) ? Integer.parseInt(s) : 100;
> +initIncRate();
> }
Correct, there are ways to modify the code to make it more annotation-friendly.
We thought about whether it's good to do it before adding the annotations or
after it. Our decision now is to do it after because it will be more easy to
see why it's necessary and we can take time to do them little by little. A new
enhancement at https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Tue, 18 May 2021 21:44:43 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.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> adjust order of VM options
The changes to the security tests look good.
-
Marked as reviewed by mullan (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:38:39 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Component.java line 217:
>>
>>> 215: * @author Sami Shaio
>>> 216: */
>>> 217: @SuppressWarnings("removal")
>>
>> Why is this placed on the *entire class* ??
>> This class is 10535 lines long and mentions AccessControl something like 8
>> places it uses AccessController or AcessControlContext.
>
> This happens when a deprecated method is called inside a static block. The
> annotation can only be added to a declaration and here it must be the whole
> class. The call in this file is
>
> s = java.security.AccessController.doPrivileged(
> new
> GetPropertyAction("awt.image.redrawrate"));
That's a sad limitation of the annotation stuff then, but I don't think that it
is insurmountable.
You can define a static private method to contain this and call it from the
static initializer block.
Much better than applying the annotation to an entire class.
--- a/src/java.desktop/share/classes/java/awt/Component.java
+++ b/src/java.desktop/share/classes/java/awt/Component.java
@@ -618,6 +618,17 @@ public abstract class Component implements ImageObserver,
MenuContainer,
*/
static boolean isInc;
static int incRate;
+
+private static void initIncRate() {
+String s = java.security.AccessController.doPrivileged(
+ new
GetPropertyAction("awt.image.incrementaldraw"));
+isInc = (s == null || s.equals("true"));
+
+s = java.security.AccessController.doPrivileged(
+ new GetPropertyAction("awt.image.redrawrate"));
+incRate = (s != null) ? Integer.parseInt(s) : 100;
+}
+
static {
/* ensure that the necessary native libraries are loaded */
Toolkit.loadLibraries();
@@ -625,14 +636,7 @@ public abstract class Component implements ImageObserver,
MenuContainer,
if (!GraphicsEnvironment.isHeadless()) {
initIDs();
}
-
-String s = java.security.AccessController.doPrivileged(
- new
GetPropertyAction("awt.image.incrementaldraw"));
-isInc = (s == null || s.equals("true"));
-
-s = java.security.AccessController.doPrivileged(
-new
GetPropertyAction("awt.image.redrawrate"));
-incRate = (s != null) ? Integer.parseInt(s) : 100;
+initIncRate();
}
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:44:06 GMT, Weijun Wang wrote:
>> Similar as the one above, it's because of
>>
>> static {
>> // Don't lazy-read because every app uses invalidate()
>> isJavaAwtSmartInvalidate = AccessController.doPrivileged(
>> new GetBooleanAction("java.awt.smartInvalidate"));
>> }
>
> We are thinking of more tweaks after this overall change to make the
> annotation more precise. For example, in this case we can assign the
> `doPrivileged` result to a local variable right at its declaration, and then
> assign it to `isJavaAwtSmartInvalidate`. Some people might think this is
> ugly. Such manual code changes need to done little by little to ease code
> reviewing.
I know it's not easy to read the commit and that's why I make the 3rd commit
totally automatic. Hopefully you have more confidence on the program than my
hand. All annotations are added to the nearest declarations.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:39:10 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Container.java line 97:
>>
>>> 95: * @since 1.0
>>> 96: */
>>> 97: @SuppressWarnings("removal")
>>
>> Same issue as with Component. a > 5,000 line file that uses AccessController
>> in just 4 places.
>>
>> Where else are you adding this to entire classes instead of the specific
>> site ?
>
> Similar as the one above, it's because of
>
> static {
> // Don't lazy-read because every app uses invalidate()
> isJavaAwtSmartInvalidate = AccessController.doPrivileged(
> new GetBooleanAction("java.awt.smartInvalidate"));
> }
We are thinking of more tweaks after this overall change to make the annotation
more precise. For example, in this case we can assign the `doPrivileged` result
to a local variable right at its declaration, and then assign it to
`isJavaAwtSmartInvalidate`. Some people might think this is ugly. Such manual
code changes need to done little by little to ease code reviewing.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:26:25 GMT, Phil Race wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 217:
>
>> 215: * @author Sami Shaio
>> 216: */
>> 217: @SuppressWarnings("removal")
>
> Why is this placed on the *entire class* ??
> This class is 10535 lines long and mentions AccessControl something like 8
> places it uses AccessController or AcessControlContext.
This happens when a deprecated method is called inside a static block. The
annotation can only be added to a declaration and here it must be the whole
class. The call in this file is
s = java.security.AccessController.doPrivileged(
new
GetPropertyAction("awt.image.redrawrate"));
> src/java.desktop/share/classes/java/awt/Container.java line 97:
>
>> 95: * @since 1.0
>> 96: */
>> 97: @SuppressWarnings("removal")
>
> Same issue as with Component. a > 5,000 line file that uses AccessController
> in just 4 places.
>
> Where else are you adding this to entire classes instead of the specific site
> ?
Similar as the one above, it's because of
static {
// Don't lazy-read because every app uses invalidate()
isJavaAwtSmartInvalidate = AccessController.doPrivileged(
new GetBooleanAction("java.awt.smartInvalidate"));
}
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
>
>> 57: ProcessCommunicator
>> 58: .executeChildProcess(Consumer.class, new
>> String[0]);
>> 59: if (!"Hello".equals(processResults.getStdOut())) {
>
> Who or what prompted this change ?
The child process is started with `-Djava.security.manager=allow` (after the
other PR) and a warning will be printed to stderr. Therefore I move the message
to stdout.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 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.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
src/java.desktop/share/classes/java/awt/Component.java line 217:
> 215: * @author Sami Shaio
> 216: */
> 217: @SuppressWarnings("removal")
Why is this placed on the *entire class* ??
This class is 10535 lines long and mentions AccessControl something like 8
places it uses AccessController or AcessControlContext.
src/java.desktop/share/classes/java/awt/Container.java line 97:
> 95: * @since 1.0
> 96: */
> 97: @SuppressWarnings("removal")
Same issue as with Component. a > 5,000 line file that uses AccessController in
just 4 places.
Where else are you adding this to entire classes instead of the specific site ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 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. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java This change is so large that github won't even display the list of files so I can't jump to where I want to go ! And when I try to scroll I get just a blank page. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 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.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Withdrawn: JDK-8257235: [PATCH] InetAddress.isReachable: Try to use an IPPROTO_ICMP socket type before attempting RAW_SOCK
On Sun, 29 Nov 2020 07:54:23 GMT, Jamie Le Tual wrote: > Users have been able to send ICMP packets without the need for root > privileges or the CAP_NET_RAW capability since at least kernel 3.11. > > For some time now, if the kernel parameter net.ipv4.ping_group_range included > the gid of a user sending an icmp packet with the IPPROTO_ICMP protocol, then > the packet would> > It's important to note that the both the checksum and ident field are > overwritten by the kernel when this is done. > > Newer distributions are now setting the default value of > net.ipv4.ping_group_range to be open to all possible group ids (Fedora 31 and > Ubuntu 20.04 for example) so it can b> > > Also of note is the that this is also implemented in MacOS. > > This patch proposes attempting to use IPPROTO_ICMP first, and then fall back > to attempting a raw socket and ultimately failing over to tcp echo. > This patch also alters the logic for identifying icmp reply packets, since > the kernel overwrites id ident field when using the IPPROTO_ICMP protocol. > The method is similar to that used by the ping(8) utility in the iputils > package, where we compare data in the icmp_data member of the icmp struct > to identify the packet as our response. The ping utility compares the > timeval, whereas this patch proposes to compare both the timeval and the > user's pid. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/1502
Withdrawn: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation
On Sat, 2 Jan 2021 10:11:30 GMT, Jayashree S Kumar wrote: > Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/1916
Withdrawn: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order
On Thu, 28 Jan 2021 16:42:02 GMT, Evan Whelan wrote: > Hi all, > > Please review this fix for which corrects the order in which field values are > returned from the `HttpURLConnection.getHeaderFields` and > `URLConnection.getRequestProperties` methods. > > Currently, the implementation of these methods returns the values in reverse. > This does not conform with the RFC2616 spec which outlines that the order of > these field values should not be changed. > > Thanks, > Evan This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/2294
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods [v3]
On Wed, 19 May 2021 10:21:02 GMT, Julia Boes wrote: >> The filter operation `Consumer` that is passed to the factory methods can >> throw an unchecked exception. This change adds a note on the exception >> handling in that case. It also adds a clarification to >> `Filter::afterHandler` on the relation of the filter operation and the >> client receiving the response. >> >> CSR: https://bugs.openjdk.java.net/browse/JDK-8267379 > > Julia Boes has updated the pull request incrementally with one additional > commit since the last revision: > > address PR comment: change wording CSR for review: https://bugs.openjdk.java.net/browse/JDK-8267379 - PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
> 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. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/bb73093a..c4221b5f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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 [v2]
On Tue, 18 May 2021 21:21:45 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. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > feedback from Sean, Phil and Alan A new commit fixing `awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java` test in tier4. The test compares stderr output with a known value but we have a new warning written to stderr now. It's now using stdout instead. @prrace, Please confirm this is acceptable. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods [v3]
> The filter operation `Consumer` that is passed to the factory methods can > throw an unchecked exception. This change adds a note on the exception > handling in that case. It also adds a clarification to `Filter::afterHandler` > on the relation of the filter operation and the client receiving the response. Julia Boes has updated the pull request incrementally with one additional commit since the last revision: address PR comment: change wording - Changes: - all: https://git.openjdk.java.net/jdk/pull/4085/files - new: https://git.openjdk.java.net/jdk/pull/4085/files/8d3749b7..4ea073a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4085&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4085&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4085.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4085/head:pull/4085 PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods [v2]
On Wed, 19 May 2021 09:19:56 GMT, Julia Boes wrote:
>> The filter operation `Consumer` that is passed to the factory methods can
>> throw an unchecked exception. This change adds a note on the exception
>> handling in that case. It also adds a clarification to
>> `Filter::afterHandler` on the relation of the filter operation and the
>> client receiving the response.
>
> Julia Boes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add bug id to test
Changes requested by michaelm (Reviewer).
src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 214:
> 212: * before the {@code operation} is invoked. More specifically, the
> response
> 213: * will typically be sent before the filter {@code operation} is
> executed.
> 214: *
I think I'd prefer "may be" rather than "will typically be" here as well. Best
not to make any claim about what is more likely to happen.
-
PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods [v2]
> The filter operation `Consumer` that is passed to the factory methods can > throw an unchecked exception. This change adds a note on the exception > handling in that case. It also adds a clarification to `Filter::afterHandler` > on the relation of the filter operation and the client receiving the response. Julia Boes has updated the pull request incrementally with one additional commit since the last revision: add bug id to test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4085/files - new: https://git.openjdk.java.net/jdk/pull/4085/files/ef8ffbfe..8d3749b7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4085&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4085&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4085.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4085/head:pull/4085 PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods
On Tue, 18 May 2021 09:34:25 GMT, Julia Boes wrote: > The filter operation `Consumer` that is passed to the factory methods can > throw an unchecked exception. This change adds a note on the exception > handling in that case. It also adds a clarification to `Filter::afterHandler` > on the relation of the filter operation and the client receiving the response. I suspect this need a CSR... - PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods
On Tue, 18 May 2021 09:49:26 GMT, Daniel Fuchs wrote:
>> The filter operation `Consumer` that is passed to the factory methods can
>> throw an unchecked exception. This change adds a note on the exception
>> handling in that case. It also adds a clarification to
>> `Filter::afterHandler` on the relation of the filter operation and the
>> client receiving the response.
>
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 213:
>
>> 211: * Doing so is likely to fail, since the exchange has commonly been
>> handled
>> 212: * before the {@code operation} is invoked. Correspondingly, the
>> client
>> 213: * commonly receives the response before the filter operation is
>> executed.
>
> I'd rather replace `commonly` with `may`: this is essentially racy and you
> cannot assume that any of the outcome is more likely than the other.
>
>
> * [...] Correspondingly, the client
> * may receive the response before the filter operation is executed.
Maybe rephrase? Who knows what the client will received, if anything - since
the bytes will be sent over the network.
Maybe something like:
More specifically, the response will typically be sent before the filter
operation is executed.
-
PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods
On Tue, 18 May 2021 09:34:25 GMT, Julia Boes wrote: > The filter operation `Consumer` that is passed to the factory methods can > throw an unchecked exception. This change adds a note on the exception > handling in that case. It also adds a clarification to `Filter::afterHandler` > on the relation of the filter operation and the client receiving the response. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4085
Re: RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods
On Tue, 18 May 2021 09:34:25 GMT, Julia Boes wrote:
> The filter operation `Consumer` that is passed to the factory methods can
> throw an unchecked exception. This change adds a note on the exception
> handling in that case. It also adds a clarification to `Filter::afterHandler`
> on the relation of the filter operation and the client receiving the response.
Marked as reviewed by dfuchs (Reviewer).
src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 213:
> 211: * Doing so is likely to fail, since the exchange has commonly been
> handled
> 212: * before the {@code operation} is invoked. Correspondingly, the
> client
> 213: * commonly receives the response before the filter operation is
> executed.
I'd rather replace `commonly` with `may`: this is essentially racy and you
cannot assume that any of the outcome is more likely than the other.
* [...] Correspondingly, the client
* may receive the response before the filter operation is executed.
-
PR: https://git.openjdk.java.net/jdk/pull/4085
RFR: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods
The filter operation `Consumer` that is passed to the factory methods can throw an unchecked exception. This change adds a note on the exception handling in that case. It also adds a clarification to `Filter::afterHandler` on the relation of the filter operation and the client receiving the response. - Commit messages: - address PR comment: update wording - Merge branch 'master' into 8267262 - initial change Changes: https://git.openjdk.java.net/jdk/pull/4085/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4085&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267262 Stats: 49 lines in 2 files changed: 44 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4085.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4085/head:pull/4085 PR: https://git.openjdk.java.net/jdk/pull/4085
