Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
+117 −318
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee1a9bf
web
andy-goryachev-oracle Oct 31, 2024
1c1e8ea
review comments
andy-goryachev-oracle Nov 1, 2024
2c7eab8
Merge remote-tracking branch 'origin/master' into 8342460.do.privileg…
andy-goryachev-oracle Nov 5, 2024
dd44845
review comments
andy-goryachev-oracle Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 5 additions & 23 deletions modules/javafx.web/src/main/java/com/sun/javafx/webkit/UIClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -41,9 +41,6 @@
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.nio.ByteBuffer;
import java.security.AccessControlContext;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -111,31 +108,20 @@ private WebEngine getWebEngine() {
return accessor.getEngine();
}

@SuppressWarnings("removal")
private AccessControlContext getAccessContext() {
return accessor.getPage().getAccessControlContext();
}

@Override public WebPage createPage(
boolean menu, boolean status, boolean toolbar, boolean resizable) {
final WebEngine w = getWebEngine();
if (w != null && w.getCreatePopupHandler() != null) {
final PopupFeatures pf =
new PopupFeatures(menu, status, toolbar, resizable);
@SuppressWarnings("removal")
WebEngine popup = AccessController.doPrivileged(
(PrivilegedAction<WebEngine>) () -> w.getCreatePopupHandler().call(pf), getAccessContext());
WebEngine popup = w.getCreatePopupHandler().call(pf);
return Accessor.getPageFor(popup);
}
return null;
}

@SuppressWarnings("removal")
private void dispatchWebEvent(final EventHandler handler, final WebEvent ev) {
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
handler.handle(ev);
return null;
}, getAccessContext());
handler.handle(ev);
}

private void notifyVisibilityChanged(boolean visible) {
Expand Down Expand Up @@ -197,23 +183,19 @@ private void notifyVisibilityChanged(boolean visible) {
}
}

@SuppressWarnings("removal")
@Override public boolean confirm(final String text) {
final WebEngine w = getWebEngine();
if (w != null && w.getConfirmHandler() != null) {
return AccessController.doPrivileged(
(PrivilegedAction<Boolean>) () -> w.getConfirmHandler().call(text), getAccessContext());
return w.getConfirmHandler().call(text);
}
return false;
}

@SuppressWarnings("removal")
@Override public String prompt(String text, String defaultValue) {
final WebEngine w = getWebEngine();
if (w != null && w.getPromptHandler() != null) {
final PromptData data = new PromptData(text, defaultValue);
return AccessController.doPrivileged(
(PrivilegedAction<String>) () -> w.getPromptHandler().call(data), getAccessContext());
return w.getPromptHandler().call(data);
}
return "";
}
Expand Down
10 changes: 3 additions & 7 deletions modules/javafx.web/src/main/java/com/sun/javafx/webkit/WebPageClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -27,8 +27,6 @@

import com.sun.javafx.scene.NodeHelper;
import java.lang.ref.WeakReference;
import java.security.AccessController;
import java.security.PrivilegedAction;

import com.sun.javafx.scene.traversal.Direction;
import com.sun.javafx.scene.traversal.TraversalMethod;
Expand All @@ -50,10 +48,8 @@
import com.sun.webkit.graphics.WCRectangle;

public final class WebPageClientImpl implements WebPageClient<WebView> {
@SuppressWarnings("removal")
private static final boolean backBufferSupported = Boolean.valueOf(
AccessController.doPrivileged((PrivilegedAction<String>) () -> System.getProperty(
"com.sun.webkit.pagebackbuffer", "true")));
private static final boolean backBufferSupported =
Boolean.valueOf(System.getProperty("com.sun.webkit.pagebackbuffer", "true"));
private static WebConsoleListener consoleListener = null;
private final Accessor accessor;

Expand Down
10 changes: 3 additions & 7 deletions modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/WCGraphicsPrismContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -53,8 +53,6 @@

import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -81,10 +79,8 @@ public enum Type {

private final static PlatformLogger log =
PlatformLogger.getLogger(WCGraphicsPrismContext.class.getName());
@SuppressWarnings("removal")
private final static boolean DEBUG_DRAW_CLIP_SHAPE = Boolean.valueOf(
AccessController.doPrivileged((PrivilegedAction<String>) () ->
System.getProperty("com.sun.webkit.debugDrawClipShape", "false")));
private final static boolean DEBUG_DRAW_CLIP_SHAPE =
Boolean.valueOf(System.getProperty("com.sun.webkit.debugDrawClipShape", "false"));

Graphics baseGraphics;
private BaseTransform baseTransform;
Expand Down
35 changes: 15 additions & 20 deletions modules/javafx.web/src/main/java/com/sun/webkit/Disposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -32,7 +32,6 @@

import java.lang.ref.ReferenceQueue;
import java.lang.ref.WeakReference;
import java.security.PrivilegedAction;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
Expand All @@ -59,24 +58,20 @@ public final class Disposer implements Runnable {
new HashSet<>();

static {
@SuppressWarnings("removal")
var dummy = java.security.AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
/*
* The thread must be a member of a thread group
* which will not get GCed before VM exit.
* Make its parent the top-level thread group.
*/
ThreadGroup tg = Thread.currentThread().getThreadGroup();
for (ThreadGroup tgn = tg;
tgn != null;
tg = tgn, tgn = tg.getParent());

Thread t = new Thread(tg, disposerInstance, "Disposer");
t.setDaemon(true);
t.setPriority(Thread.MAX_PRIORITY);
t.start();
return null;
});
/*
* The thread must be a member of a thread group
* which will not get GCed before VM exit.
* Make its parent the top-level thread group.
*/
ThreadGroup tg = Thread.currentThread().getThreadGroup();
for (ThreadGroup tgn = tg;
tgn != null;
tg = tgn, tgn = tg.getParent());

Thread t = new Thread(tg, disposerInstance, "Disposer");
t.setDaemon(true);
t.setPriority(Thread.MAX_PRIORITY);
t.start();
}

/**
Expand Down
9 changes: 2 additions & 7 deletions modules/javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -28,18 +28,13 @@
import com.sun.javafx.reflect.MethodUtil;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.AccessController;
import java.security.PrivilegedAction;
import com.sun.javafx.reflect.ReflectUtil;

/**
* Utility class to wrap method invocation.
*/
public class MethodHelper {
@SuppressWarnings("removal")
private static final boolean logAccessErrors
= AccessController.doPrivileged((PrivilegedAction<Boolean>) ()
-> Boolean.getBoolean("sun.reflect.debugModuleAccessChecks"));
private static final boolean logAccessErrors = Boolean.getBoolean("sun.reflect.debugModuleAccessChecks");

private static final Module trampolineModule = MethodUtil.getTrampolineModule();

Expand Down
10 changes: 2 additions & 8 deletions modules/javafx.web/src/main/java/com/sun/webkit/Timer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -25,9 +25,6 @@

package com.sun.webkit;

import java.security.AccessController;
import java.security.PrivilegedAction;

public class Timer {
private static Timer instance;
private static Mode mode;
Expand All @@ -42,12 +39,9 @@ public static enum Mode {
SEPARATE_THREAD
}

@SuppressWarnings("removal")
public synchronized static Mode getMode() {
if (mode == null) {
mode = Boolean.valueOf(AccessController.doPrivileged(
(PrivilegedAction<String>) () -> System.getProperty(
"com.sun.webkit.platformticks", "true"))) ? Mode.PLATFORM_TICKS : Mode.SEPARATE_THREAD;
mode = Boolean.valueOf(System.getProperty("com.sun.webkit.platformticks", "true")) ? Mode.PLATFORM_TICKS : Mode.SEPARATE_THREAD;
}
return mode;
}
Expand Down
18 changes: 2 additions & 16 deletions modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -28,9 +28,6 @@
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.AccessControlContext;
import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.util.List;
import java.util.Set;

Expand Down Expand Up @@ -119,17 +116,6 @@ private static Object fwkInvokeWithContext(final Method method,
});
}

try {
return AccessController.doPrivileged((PrivilegedExceptionAction<Object>)
() -> MethodHelper.invoke(method, instance, args), acc);
} catch (PrivilegedActionException ex) {
Throwable cause = ex.getCause();
if (cause == null)
cause = ex;
else if (cause instanceof InvocationTargetException
&& cause.getCause() != null)
cause = cause.getCause();
throw cause;
}
return MethodHelper.invoke(method, instance, args);
Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Loading