8342460: Remove calls to doPrivileged in javafx.web #1620

andy-goryachev-oracle · 2024-10-31T22:51:24Z

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

/reviewers 2

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8342460: Remove calls to doPrivileged in javafx.web (Bug - P3)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1620

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-31T22:51:34Z

👋 Welcome back angorya! 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 · 2024-10-31T22:52:15Z

@andy-goryachev-oracle 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:

8342460: Remove calls to doPrivileged in javafx.web

Reviewed-by: kcr, arapte

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

andy-goryachev-oracle · 2024-10-31T22:52:21Z

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

-                cause = cause.getCause();
-            throw cause;
-        }
+        return MethodHelper.invoke(method, instance, args);


I think the whole fwkInvokeWithContext method and associated constants can be safely removed.

Very unlikely, but I'll take a look when fixing JDK-8342993, since this method is on my list to be modified.

I don't see this private method being called...

https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp#L325-L330

All Java methods in javafx.web that are named fwk* are called by native WebKit code via JNI upcalls.

ouch. it would be nice to have the method commented or annotated somehow...
thank you for clarification!

Indeed it would. Instead we just have to "know" the convention. Not very satisfying, is it!

openjdk · 2024-10-31T22:57:02Z

@andy-goryachev-oracle
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).

mlbridge · 2024-10-31T23:25:56Z

Webrevs

kevinrushforth

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

kevinrushforth · 2024-11-01T13:02:22Z

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

-            } catch (@SuppressWarnings("removal") AccessControlException ex) {
-                errorCode = LoadListenerClient.PERMISSION_DENIED;


Excellent. This was on my list for 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).

kevinrushforth · 2024-11-01T13:26:25Z

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

-                cause = cause.getCause();
-            throw cause;
-        }
+        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.

Suggested change

return MethodHelper.invoke(method, instance, args);

try {

return MethodHelper.invoke(method, instance, args);

} catch (InvocationTargetException ex) {

Throwable cause = ex.getCause();

throw cause != null ? cause : ex;

}

changed as suggested.
is this test being run as part of GHA or the headful test suite?

kevinrushforth · 2024-11-01T13:36:37Z

modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java

@@ -830,9 +826,7 @@ private Button addButton(ToolBar toolbar, final String iconName, String tooltipT
        button.getStyleClass().add(styleClass);
        toolbar.getItems().add(button);

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


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

reverted here and below.

kevinrushforth · 2024-11-01T13:37:05Z

modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java

        ((StyleableProperty)toggleButton.graphicProperty()).applyStyle(null, new ImageView(icon));
-//        toggleButton.setGraphic(new ImageView(icon));


Same question as above.

kevinrushforth · 2024-11-01T13:40:31Z

@arapte Can you be the second reviewer?

arapte

LGTM, with one minor comment. Please take a look.

arapte · 2024-11-05T06:45:09Z

modules/javafx.web/src/main/java/javafx/scene/web/WebEngine.java

-                        messageCallback.call(message);
-                        return null;
-                    }, webEngine.page.getAccessControlContext());
+                    messageCallback.call(message);


@SuppressWarnings("removal") can be removed from this method.

removed, thanks!

…ed.web

web

ee1a9bf

andy-goryachev-oracle commented Oct 31, 2024

View reviewed changes

andy-goryachev-oracle marked this pull request as ready for review October 31, 2024 23:21

openjdk bot added the rfr Ready for review label Oct 31, 2024

kevinrushforth reviewed Nov 1, 2024

View reviewed changes

review comments

1c1e8ea

kevinrushforth approved these changes Nov 1, 2024

View reviewed changes

arapte suggested changes Nov 5, 2024

View reviewed changes

kevinrushforth mentioned this pull request Nov 5, 2024

8343630: Pass AccessControlContext to/from WebKit as opaque object #1626

Open

3 tasks

andy-goryachev-oracle added 2 commits November 5, 2024 12:07

Merge remote-tracking branch 'origin/master' into 8342460.do.privileg…

2c7eab8

…ed.web

review comments

dd44845

kevinrushforth approved these changes Nov 5, 2024

View reviewed changes

arapte approved these changes Nov 6, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8342460: Remove calls to doPrivileged in javafx.web #1620

8342460: Remove calls to doPrivileged in javafx.web #1620

andy-goryachev-oracle commented Oct 31, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024 •

edited

Loading

andy-goryachev-oracle Oct 31, 2024 •

edited

Loading

kevinrushforth Oct 31, 2024

andy-goryachev-oracle Oct 31, 2024

kevinrushforth Oct 31, 2024

andy-goryachev-oracle Oct 31, 2024

kevinrushforth Oct 31, 2024

openjdk bot commented Oct 31, 2024

mlbridge bot commented Oct 31, 2024 •

edited

Loading

kevinrushforth left a comment

kevinrushforth Nov 1, 2024

kevinrushforth Nov 1, 2024

andy-goryachev-oracle Nov 1, 2024

kevinrushforth Nov 1, 2024

andy-goryachev-oracle Nov 1, 2024

kevinrushforth Nov 1, 2024

kevinrushforth commented Nov 1, 2024

arapte left a comment

arapte Nov 5, 2024

andy-goryachev-oracle Nov 5, 2024

		} catch (@SuppressWarnings("removal") AccessControlException ex) {
		errorCode = LoadListenerClient.PERMISSION_DENIED;

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

		((StyleableProperty)toggleButton.graphicProperty()).applyStyle(null, new ImageView(icon));
		// toggleButton.setGraphic(new ImageView(icon));

8342460: Remove calls to doPrivileged in javafx.web #1620

Are you sure you want to change the base?

8342460: Remove calls to doPrivileged in javafx.web #1620

Conversation

andy-goryachev-oracle commented Oct 31, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024 • edited Loading

andy-goryachev-oracle Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 31, 2024

mlbridge bot commented Oct 31, 2024 • edited Loading

Webrevs

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth commented Nov 1, 2024

arapte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Oct 31, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 31, 2024 •

edited

Loading

andy-goryachev-oracle Oct 31, 2024 •

edited

Loading

mlbridge bot commented Oct 31, 2024 •

edited

Loading