RFR: 8341090: Remove support for security manager from JavaFX [v2]

Johan Vos jvos at openjdk.org
Fri Oct 18 10:15:20 UTC 2024


On Thu, 17 Oct 2024 21:47:00 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> 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 renamed`CustomSecurityManagerTest` 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](https://github.com/openjdk/jdk-sandbox/tree/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 handle...
>
> Kevin Rushforth has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Merge branch 'master' into 8341090-remove-sm
>  - SM removal part 5: Update API spec to remove mention of security manager
>  - SM removal part 4: delete remaining tests that depend on security manager
>  - SM removal part 2: remove -Djava.security.manager and security policy files
>    SM removal part 3: remove SandboxAppTest
>  - SM removal part 1: Fail fast at startup if the Security Manager is enabled

@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.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1595#issuecomment-2422083777


More information about the openjfx-dev mailing list