-
Notifications
You must be signed in to change notification settings - Fork 517
8264140: Add -Djava.security.manager=allow to JavaFX tests calling setSecurityManager #510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8264140: Add -Djava.security.manager=allow to JavaFX tests calling setSecurityManager #510
Conversation
👋 Welcome back kcr! A progress list of the required criteria for merging this PR into |
@kevinrushforth |
Note that this does not provide fine-grained setting of |
@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:
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 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/integrate |
@kevinrushforth Since your change was applied there have been 4 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 9507ea4. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The security manager is being deprecated for removal by JEP 411, which is proposed for JDK 17. The implementation of JEP 411 is out for review here: openjdk/jdk#4073.
Along with the deprecation of most security manager classes and methods, the default value of the
java.security.manager
system property will be treated as "disallow
". This means that the deprecatedSystem::setSecurityManager
method will throwUnsupportedOperationException
unless-Djava.security.manager=allow
is set on the command line. A few system tests and javafx.web unit tests set a security manager and will fail without setting that system property.The fix is to set the
-Djava.security.manager=allow
inbuild.gradle
when running tests in the:systemTests
and:web
projects. Support for settingjava.security.manager
to "allow
" was added in JDK 12, so we also check that the boot JDK is >= 12 (the check is needed because JDK 11 is the minimum boot JDK).As part of this PR I am removing the legacy
ServiceWithSecurityManagerTest
to address JDK-8234175.ServiceWithSecurityManagerTest
is the only unit test in thejavafx.graphics
module that sets a security manager, and the test has skipped via@Ignore
since FX 8 with the comment that the test "appears to be bogus". Since there are no activejavafx.graphics
tests that set the security manager, I don't see the need to set-Djava.security.manager=allow
forjavafx.graphics
tests. Better to just remove the test.I have done a full build / test using JDK 15 as a boot JDK. I then did a full build / test using a locally-built JDK 17-ea with the fix for JEP 411, and setting
JDK_HOME
to that JDK. With that, I can confirm that the tests that set the security manager fail without this fix and pass with this fix./issue add 8234175
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/510/head:pull/510
$ git checkout pull/510
Update a local copy of the PR:
$ git checkout pull/510
$ git pull https://git.openjdk.java.net/jfx pull/510/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 510
View PR using the GUI difftool:
$ git pr show -t 510
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/510.diff