8344252: SM cleanup in java.util classes #22122

RogerRiggs · 2024-11-14T22:09:59Z

Remove use of doPrivileged and SecurityManager in java.util.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8344252: SM cleanup in java.util classes (Bug - P4)

Reviewers

Stuart Marks (@stuart-marks - Reviewer) Review applies to 9021ee53
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22122/head:pull/22122
$ git checkout pull/22122

Update a local copy of the PR:
$ git checkout pull/22122
$ git pull https://git.openjdk.org/jdk.git pull/22122/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22122

View PR using the GUI difftool:
$ git pr show -t 22122

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22122.diff

Using Webrev

Link to Webrev Comment

Remove use of doPrivileged and SecurityManager

bridgekeeper · 2024-11-14T22:11:13Z

👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-14T22:11:30Z

@RogerRiggs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8344252: SM cleanup in java.util classes

Reviewed-by: naoto, smarks

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 32 new commits pushed to the master branch:

ff12ff5: 8340453: C2: Improve encoding of LoadNKlass for compact headers
1866c0c: 8344259: Annotate Float16 with jdk.internal.ValueBased
0b9b82a: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io
3c38ed4: 8344314: Revert removal of jdk.internal.java.PreviewFeature.CLASSFILE_API
40a055e: 8344228: Revisit SecurityManager usage in java.net.http after JEP 486 integration
84ffb64: 8334714: Implement JEP 484: Class-File API
6cdebf0: 8343599: Kmem limit and max values swapped when printing container information
a672138: 8344161: Argument type mismatch for jfr_type_id
3245f56: 8344164: [s390x] ProblemList hotspot/jtreg/runtime/NMT/VirtualAllocCommitMerge.java
5e27608: 8344188: Cleanup sun.net.www.protocol.jar.JarFileFactory after JEP 486 integration
... and 22 more: https://git.openjdk.org/jdk/compare/752e1629555f0ec8630373ec87b049afdd709ea6...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-14T22:12:08Z

@RogerRiggs The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-14T22:17:46Z

Webrevs

src/java.base/share/classes/java/util/Properties.java

src/java.base/share/classes/java/util/spi/AbstractResourceBundleProvider.java

src/java.base/share/classes/java/util/ResourceBundle.java

stuart-marks · 2024-11-15T00:13:58Z

I mainly wanted to look at Arrays, and I looked briefly at the other stuff too. All looks good.

naotoj · 2024-11-15T17:05:12Z

src/java.base/share/classes/java/util/spi/AbstractResourceBundleProvider.java

+ * @see <a href="ResourceBundle##resource-bundle-modules">
+ * Resource Bundles and Named Modules</a>


I think HTML tag is not needed:

Suggested change

* @see <a href="ResourceBundle##resource-bundle-modules">

* Resource Bundles and Named Modules</a>

* @see ResourceBundle##resource-bundle-modules

* Resource Bundles and Named Modules

naotoj

LGTM

RogerRiggs · 2024-11-15T20:28:12Z

/integrate

openjdk · 2024-11-15T20:29:15Z

Going to push as commit c5b6ed8.
Since your change was applied there have been 38 commits pushed to the master branch:

59ffac8: 8342811: java/net/httpclient/PlainProxyConnectionTest.java failed: Unexpected connection count: 5
f316808: 8344189: Cleanup code in sun.net.www.protocol.mailto.MailToURLConnection and sun.net.smtp after JEP 486 integration
73fd891: 8344216: Remove calls to SecurityManager and and doPrivileged in java.net.Authenticator, java.net.CookieHandler, and java.net.ResponseCache after JEP 486 integration
f62e05e: 8344231: SecurityManager cleanup in java.lang.module and jdk.internal.module
1bb0d3b: 8344062: Remove doPrivileged calls from awt and beans classes in the java.desktop module
fc8fb34: 8344214: Remove Security Manager dependencies from jdk.crypto.mscapi module
ff12ff5: 8340453: C2: Improve encoding of LoadNKlass for compact headers
1866c0c: 8344259: Annotate Float16 with jdk.internal.ValueBased
0b9b82a: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io
3c38ed4: 8344314: Revert removal of jdk.internal.java.PreviewFeature.CLASSFILE_API
... and 28 more: https://git.openjdk.org/jdk/compare/752e1629555f0ec8630373ec87b049afdd709ea6...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-15T20:29:23Z

@RogerRiggs Pushed as commit c5b6ed8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

eirbjo · 2024-12-07T19:44:05Z

src/java.base/share/classes/java/util/ResourceBundle.java

@@ -3722,8 +3654,8 @@ private static String toPackageName(String bundleName) {

    }

-    private static final boolean TRACE_ON = Boolean.parseBoolean(
-        GetPropertyAction.privilegedGetProperty("resource.bundle.debug", "false"));
+    private static final boolean TRACE_ON = Boolean.getBoolean(


This update seems to have broken the tracing feature of ResourceBundle.

The previous code called GetPropertyAction::privilegedGetProperty to get the system property "resource.bundle.debug" with a default of "false". It then used Boolean::parseBoolean to compare it to "true", ignoring case.

The new code uses System::getProperty to get the same property, then calls Boolean::getBoolean which calls System::getProperty to get the value of the property which name is either "false" or the result of looking up "resource.bundle.debug"

From what I can tell, it is now not possible to enable tracing.

Seems like we could simply use Boolean.getBoolean("resource.bundle.debug") instead?

Seems like we could simply use Boolean.getBoolean("resource.bundle.debug") instead?

Naoto may know the history on this property. It may have been introduced for debugging when working on the RB implementation or maybe it was introduced to allow developers to debug, not sure. If the latter then it's important to preserve long standing behavior. If the former, and it was never documented, there is a lot more flexibility to change.

If the former, and it was never documented, there is a lot more flexibility to change.

Fair enough. However, this SM change seems to accidentally have introduced a bug where the system property is read twice, first calling System.getProperty to look up the name of the system property which is then looked up by Boolean.getBoolean. That behavior just seems broken.

@RogerRiggs may have intended to use Boolean::parseBoolean instead:

Boolean.parseBoolean(System.getProperty("resource.bundle.debug", "false"))

which is equivalent to:

Boolean.getBoolean("resource.bundle.debug")

Filed https://bugs.openjdk.org/browse/JDK-8345818.

8344252: SM cleanup in java.util classes

9021ee5

Remove use of doPrivileged and SecurityManager

openjdk bot added the rfr label Nov 14, 2024

openjdk bot added core-libs i18n labels Nov 14, 2024

naotoj reviewed Nov 14, 2024

View reviewed changes

stuart-marks approved these changes Nov 15, 2024

View reviewed changes

openjdk bot added the ready label Nov 15, 2024

Applied suggestions from review comments

f8e518f

openjdk bot removed the ready label Nov 15, 2024

naotoj reviewed Nov 15, 2024

View reviewed changes

Correct @see link syntax in AbstractResourceBundleProvider

1a0814c

naotoj approved these changes Nov 15, 2024

View reviewed changes

openjdk bot added the ready label Nov 15, 2024

openjdk bot added the integrated label Nov 15, 2024

openjdk bot closed this Nov 15, 2024

openjdk bot removed ready rfr labels Nov 15, 2024

eirbjo reviewed Dec 7, 2024

View reviewed changes

RogerRiggs deleted the 8344252-sm-util branch February 24, 2025 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8344252: SM cleanup in java.util classes #22122

8344252: SM cleanup in java.util classes #22122

RogerRiggs commented Nov 14, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 14, 2024

openjdk bot commented Nov 14, 2024 •

edited

Loading

openjdk bot commented Nov 14, 2024

mlbridge bot commented Nov 14, 2024 •

edited

Loading

stuart-marks commented Nov 15, 2024

naotoj Nov 15, 2024

naotoj left a comment

RogerRiggs commented Nov 15, 2024

openjdk bot commented Nov 15, 2024

openjdk bot commented Nov 15, 2024

eirbjo Dec 7, 2024 •

edited

Loading

AlanBateman Dec 9, 2024

eirbjo Dec 9, 2024 •

edited

Loading

RogerRiggs Dec 9, 2024

		* @see <a href="ResourceBundle##resource-bundle-modules">
		* Resource Bundles and Named Modules</a>

8344252: SM cleanup in java.util classes #22122

8344252: SM cleanup in java.util classes #22122

Conversation

RogerRiggs commented Nov 14, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 14, 2024

openjdk bot commented Nov 14, 2024 • edited Loading

openjdk bot commented Nov 14, 2024

mlbridge bot commented Nov 14, 2024 • edited Loading

Webrevs

stuart-marks commented Nov 15, 2024

naotoj Nov 15, 2024

Choose a reason for hiding this comment

naotoj left a comment

Choose a reason for hiding this comment

RogerRiggs commented Nov 15, 2024

openjdk bot commented Nov 15, 2024

openjdk bot commented Nov 15, 2024

eirbjo Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

AlanBateman Dec 9, 2024

Choose a reason for hiding this comment

eirbjo Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

RogerRiggs Dec 9, 2024

Choose a reason for hiding this comment

RogerRiggs commented Nov 14, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 14, 2024 •

edited

Loading

mlbridge bot commented Nov 14, 2024 •

edited

Loading

eirbjo Dec 7, 2024 •

edited

Loading

eirbjo Dec 9, 2024 •

edited

Loading