On Thu, 31 Oct 2024 14:09:52 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
>> This PR removes uses of `AccessControler.doPrivileged()` from >> `javafx.graphics` package. All uses of `doPrivileged()` were removed >> _excluding_ `com.sun.javafx.tk` and `com.sun.ui.glass` subpackages - those >> are handled by [JDK-8342453](https://bugs.openjdk.org/browse/JDK-8342453) >> (already completed) and >> [JDK-8342454](https://bugs.openjdk.org/browse/JDK-8342454) respectively. >> >> Most of these changes are fairly straightforward and follow the standard >> replacement from `AccessController.doPrivileged(LAMBDA)` to just simply >> `LAMBDA` with a couple exceptions: >> >> - `StyleManager.java` where `loadStylesheet()` followed two steps - >> attempting straightforward stylesheet load via >> `loadStylesheetUnPrivileged()` and in case of `AccessControlException` >> retrying the load via `AccessController.doPrivileged` with read-only access >> to stylesheet JAR. Now that `doPrivileged` is deprecated and not used >> anymore, `loadStylesheetUnPrivileged()` was always successful and the >> fallback became essentially dead code. Fallback was removed and >> `loadStylesheetUnPrivileged()` was renamed to `loadStylesheet()`. >> - `Scene.java` integrates with `com.sun.javafx.tk` so only straightforward >> `doPrivileged()` calls were removed - `AccessControlContext` remained, as it >> needs to be removed from both tk and Scene via a follow-up >> ([JDK-8342993](https://bugs.openjdk.org/browse/JDK-8342993)) >> >> Building graphics module now produces less warnings than before this change >> with no new warning messages (all warnings refer to the use of `Unsafe` >> which is out of scope of this change). > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Scene: Remove missed doPrivileged use modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleManager.java line 1063: > 1061: ** That way there in no information leaked. > 1062: */ > 1063: catch (java.net.URISyntaxException e) { are you sure the change is equivalent? For example, the old code catches `URISyntaxException` and `PrivilegedActionException` returning `null`, but the new code does not, unless I am mistaken. modules/javafx.graphics/src/main/java/com/sun/javafx/font/Disposer.java line 62: > 60: tgn != null; > 61: tg = tgn, tgn = tg.getParent()); > 62: Thread t = new Thread(tg, disposerInstance, "Prism Font > Disposer"); very minor: I would have separated `for()` from L62 by a newline. This `for` is already confusing enough. modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java line 137: > 135: isCopy = isDecoded = false; > 136: } catch (Exception e) { > 137: } finally { even though the code change is equivalent, it will print "temp file not deleted" followed by "temp file deleted". modules/javafx.graphics/src/main/java/com/sun/javafx/util/ModuleHelper.java line 39: > 37: > 38: static { > 39: verbose = Boolean.getBoolean("javafx.verbose"); minor I would rather moved it to L36 modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 689: > 687: (PrivilegedAction<List<Window>>) () -> > Window.getWindows(), > 688: null, > 689: ACCESS_WINDOW_LIST_PERMISSION); @kevinrushforth once we finish removing AccessController, do we need to remove `FXPermissions` class? modules/javafx.graphics/src/main/java/com/sun/marlin/MarlinProperties.java line 271: > 269: > 270: // system property utilities > 271: @SuppressWarnings("removal") really, this should be in Utils. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824664938 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824770375 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824780352 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824784460 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824787462 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824788989
