RFR: 8342460: Remove calls to doPrivileged in javafx.web

Kevin Rushforth kcr at openjdk.org
Fri Nov 1 13:43:36 UTC 2024


On Thu, 31 Oct 2024 22:51:24 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> Removes `doPrivileged` calls in the javafx.web module, excluding the code in `{android,ios}`.

Looks good except for one problem that needs to be fixed. I also left a question inline about removed comment lines.

@arapte Can you be the second reviewer?

modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java line 119:

> 117:         }
> 118: 
> 119:         return MethodHelper.invoke(method, instance, args);

The removal of the try/catch is causing a test failure in `JavaScriptBridgeTest` because `InvocationTargetException` is now thrown rather than `InvocationTargetException::getCause`.

Suggestion:

        try {
            return MethodHelper.invoke(method, instance, args);
        } catch (InvocationTargetException ex) {
            Throwable cause = ex.getCause();
            throw cause != null ? cause : ex;
        }

modules/javafx.web/src/main/java/com/sun/webkit/network/HTTP2Loader.java line 580:

> 578:                 errorCode = LoadListenerClient.MALFORMED_URL;
> 579:             } catch (@SuppressWarnings("removal") AccessControlException ex) {
> 580:                 errorCode = LoadListenerClient.PERMISSION_DENIED;

Excellent. This was on my list for [JDK-8342998](https://bugs.openjdk.org/browse/JDK-8342998), so one less thing for me to do for that (actually, two less things, since you also took care of the one in `URLLoader.java`).

modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java line 835:

> 833:         @SuppressWarnings("removal")
> 834:         Image icon = AccessController.doPrivileged((PrivilegedAction<Image>) () -> new Image(HTMLEditorSkin.class.getResource(iconName).toString()));
> 835: //        button.setGraphic(new ImageView(icon));

I see you removed this comment. Do you think there is no value in leaving it?

modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java line 861:

> 859:         Image icon = AccessController.doPrivileged((PrivilegedAction<Image>) () -> new Image(HTMLEditorSkin.class.getResource(iconName).toString()));
> 860:         ((StyleableProperty)toggleButton.graphicProperty()).applyStyle(null, new ImageView(icon));
> 861: //        toggleButton.setGraphic(new ImageView(icon));

Same question as above.

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

PR Review: https://git.openjdk.org/jfx/pull/1620#pullrequestreview-2410029043
PR Comment: https://git.openjdk.org/jfx/pull/1620#issuecomment-2451891315
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825816669
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825794129
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825827044
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825827478


More information about the openjfx-dev mailing list