8341090: Remove support for security manager from JavaFX

[kevinrushforth]

trafficstars

This PR removes support for running JavaFX applications with the Java Security Manager.

The initial work was done in 4 separate commits as follows:

• Fail fast at startup if the Security Manager is enabled
• Remove -Djava.security.manager and all security policy files; delete SandboxAppTest
• Delete remaining tests that depend on the Security Manager; as part of this, I renamedCustomSecurityManagerTest to StageRobotTest and removed all of the tests that depend on the security manager, since the test methods that don't depend on SM are useful functional tests of a Stage's FullScreen and AlwaysOnTop mode.
• API spec change to remove mention of security manager (including mention of throwing SecurityException)

This PR is both necessary and sufficient to remove support for running with the Java Security Manager enabled. I have tested this with JDK 23, which still has SM support (although deprecated for removal), and with a local JDK built from the jdk-sandbox:jep486 branch, which has the SM disabled. With both JDKs, I can build and run this PR branch; all tests pass. Attempting to enable the security manager with JDK 23 will now fail in <clinit> of either LauncherImpl (called by Application.launch) or PlatformImpl (called by Platform.startup) with an informative error message.

This PR should be integrated prior to the integration of JEP 486, so that we can continue to run JavaFX tests on JDK 24 after the SM is disabled. Otherwise they will fail to run with a fatal error launching the JVM (we currently pass -Djava.security.manager=allow when running our tests).

Follow-on work

I will file additional JBS issues to track follow-up work to cleanup the remaining calls to the security manager methods that are deprecated for removal. The follow-up work can be done independently of the timing of the integration of JEP 486 into JDK 24, but all of them will be targeted for JavaFX 24.

Follow-on issues for JavaFX 24:

• Remove all calls to doPrivileged (there are 350 such calls in 168 files)
• Remove all other calls to AccessController (48 calls in 33 files)
• Remove all remaining calls to System::getSecurityManager (45 calls in 27 files)
• Remove calls to deprecated SM methods from PlatformUtil and MethodUtil in javafx.base (these are lightly modified copies of those classes in java.base so I will check how they are being handled)
• Remove all uses (either throw or catch) of the deprecated AccessControlException : : 4 occurrences in 4 files
• Remove all occurrences where we throw a SecurityException; check all place where we catch it (and consider whether it is needed)
• Check and remove any remaining usages of other SM methods that are deprecated for removal by JEP 411
  • SecurityManager 2 files (in addition to the ones that call getSecurityManager)
  • AccessControlContext: 169 occurrences in 65 files (many are associated with doPriviliged calls, so should be rechecked once doPriv is removed)
• Utility method for fail fast SM check (if we keep the check, else remove it)

NOTE: I had earlier thought that the "fail fast" could be temporary until all follow-up work is done, but I now think we should leave it in for JavaFX 24, possibly adding it to two other classes -- ReflectUtil and MethodUtil -- depending on the follow-up work needed for those classes. If we do end up leaving it, I'll create a utility method in javafx.base that will use reflection to check whether the security manager is enabled, so we don't leave in a call to a method (System::getSecurityManager) that is deprecated for removal.

We might also file additional follow-up issues, not necessarily targeted for JavaFX 24:

• Consider deprecating for removal the following security-related APIs in JavaFX
  • FXPermission (this PR changes the spec to make it clear that it is no longer used to control access to resources)
  • WebErrorEvent.USER_DATA_DIRECTORY_SECURITY_ERROR (if there is no other use than to report a SecurityException that can no longer happen after the security manager is removed)

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change requires CSR request JDK-8341858 to be approved
• [x] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

• JDK-8341090: Remove support for security manager from JavaFX (Enhancement - P2)
• JDK-8341858: Remove support for security manager from JavaFX (CSR)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Reviewer)
• Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1595

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1595.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back kcr! 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[bot]]

@kevinrushforth 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:

8341090: Remove support for security manager from JavaFX

Reviewed-by: angorya, arapte, jvos

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 1 new commit pushed to the master branch:

• e128382fa305bb0a50b1f9fbb90f5237f9a0f9c5: 8329098: Support "@1x" image naming convention as fallback

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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.

[kevinrushforth]

/reviewers 2 /csr

[openjdk[bot]]

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[openjdk[bot]]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@kevinrushforth please create a CSR request for issue JDK-8341090 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[mlbridge[bot]]

Webrevs

• 03: Full - Incremental (4a262de2)
• 02: Full - Incremental (6506f774)
• 01: Full - Incremental (a34575ed)
• 00: Full (7529e414)

[johanvos]

@kevinrushforth looks good at first sight. I'm very happy to see the SandboxAppTest being removed! When you say "all tests pass", what set of tests are you referring to? We have a number of properties that determine which tests are actually executed, and I believe in general it would be good if we could standardize on what set of tests we consider crucial (e.g. not the ones marked as unstable). I'd like to run all tests as well on all supported platforms, but your explanation is very clear and makes sense, hence I don't expect issues.

[kevinrushforth]

When you say "all tests pass", what set of tests are you referring to? We have a number of properties that determine which tests are actually executed, and I believe in general it would be good if we could standardize on what set of tests we consider crucial (e.g. not the ones marked as unstable).

Agreed. By "all tests", I meant a full headful test run except unstable tests, meaning that I ran gradle test with: -PFULL_TEST=true -PUSE_ROBOT=true.

Our typical headful test run is:

gradle --info sdk shims
gradle --info --continue -PTEST_ONLY=true -PFULL_TEST=true -PUSE_ROBOT=true test

I usually run it like that so that I get a fail fast on the compilation (i.e., without the --continue) and so that repeat runs, or runs of specific tests don't need to recompile the sdk and shims, but if all you want is a single full test run, you don't need to split (in which case, omit the TEST_ONLY flag).

I'd like to run all tests as well on all supported platforms, but your explanation is very clear and makes sense, hence I don't expect issues.

That would be great. Let me know what you find.

I did a full test run using JDK 23 (our default boot JDK) on the following platforms:

Windows 11 Mac aarch64 (both macOS 13 and macOS 14) Mac x64 (both macOS 13 and macOS 14) Ubuntu 22.04

I also did the same full test run setting JDK_HOME to a test build of the jdk-sandbox:jep486 branch on the same platforms, excluding Mac x64 (I didn't have a JDK jep486 build handy on our test machine, and since there is nothing arch-specific about the SM removal, that seemed OK).

[kevinrushforth]

@johanvos Did you want a chance to review this before I integrate it?

[kevinrushforth]

/integrate

[openjdk[bot]]

Going to push as commit dc5df6c126468c2f4c35b0c8633f82ac7eda15ef. Since your change was applied there has been 1 commit pushed to the master branch:

• e128382fa305bb0a50b1f9fbb90f5237f9a0f9c5: 8329098: Support "@1x" image naming convention as fallback

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@kevinrushforth Pushed as commit dc5df6c126468c2f4c35b0c8633f82ac7eda15ef.

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