Skip to content

Edge:evaluate deadlock fix #1973

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

Merged
merged 1 commit into from
Apr 9, 2025
Merged

Edge:evaluate deadlock fix #1973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 30 additions & 5 deletions bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public WebViewEnvironment(ICoreWebView2Environment environment) {

static boolean inCallback;
boolean inNewWindow;
private boolean inEvaluate;
HashMap<Long, LocationEvent> navigations = new HashMap<>();
private boolean ignoreGotFocus;
private boolean ignoreFocusIn;
Expand Down Expand Up @@ -906,11 +907,21 @@ public Object evaluate(String script) throws SWTException {
// Feature in WebView2. ExecuteScript works regardless of IsScriptEnabled setting.
// Disallow programmatic execution manually.
if (!jsEnabled) return null;

if(inCallback) {
// Execute script, but do not wait for async call to complete as otherwise it
// can cause a deadlock if execute inside a WebView callback.
execute(script);
return null;
}
String script2 = "(function() {try { " + script + " } catch (e) { return '" + ERROR_ID + "' + e.message; } })();\0";
String[] pJson = new String[1];
int hr = callAndWait(pJson, completion -> webViewProvider.getWebView(true).ExecuteScript(script2.toCharArray(), completion));
if (hr != COM.S_OK) error(SWT.ERROR_FAILED_EVALUATE, hr);
inEvaluate = true;
try {
int hr = callAndWait(pJson, completion -> webViewProvider.getWebView(true).ExecuteScript(script2.toCharArray(), completion));
if (hr != COM.S_OK) error(SWT.ERROR_FAILED_EVALUATE, hr);
} finally {
inEvaluate = false;
}

Object data = JSON.parse(pJson[0]);
if (data instanceof String && ((String) data).startsWith(ERROR_ID)) {
Expand Down Expand Up @@ -1319,7 +1330,7 @@ int handleNewWindowRequested(long pView, long pArgs) {
args.GetDeferral(ppv);
ICoreWebView2Deferral deferral = new ICoreWebView2Deferral(ppv[0]);
inNewWindow = true;
browser.getDisplay().asyncExec(() -> {
Runnable openWindowHandler = () -> {
try {
if (browser.isDisposed()) return;
WindowEvent openEvent = new WindowEvent(browser);
Expand Down Expand Up @@ -1355,7 +1366,21 @@ int handleNewWindowRequested(long pView, long pArgs) {
args.Release();
inNewWindow = false;
}
});
};

// Creating a new browser instance within the same environment from inside the OpenWindowListener of another browser
// can lead to a deadlock. To prevent this, handlers should typically run asynchronously.
// However, if a new window is opened using `evaluate(window.open)`, running the handler asynchronously in that context
// may also result in a deadlock.
// Therefore, whether the listener runs synchronously or asynchronously should depend on the `inEvaluate` condition.
// That said, combining both situations—opening a window via `evaluate` and launching a new browser inside the OpenWindowListener—
// should be avoided altogether, as it significantly increases the risk of deadlocks.
if (inEvaluate) {
openWindowHandler.run();
} else {
browser.getDisplay().asyncExec(openWindowHandler);
}

return COM.S_OK;
}

Expand Down
6 changes: 0 additions & 6 deletions ...t.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,6 @@ public void changed(LocationEvent event) {

@Test
public void test_LocationListener_LocationListener_ordered_changing () {
assumeFalse("Currently broken for Edge", isEdge);
List<String> locations = Collections.synchronizedList(new ArrayList<>());
browser.addLocationListener(changingAdapter(event -> {
locations.add(event.location);
Expand Down Expand Up @@ -1578,8 +1577,6 @@ public void completed(ProgressEvent event) {
*/
@Test
public void test_LocationListener_evaluateInCallback() {
assumeFalse("behavior is not (yet) supported by Edge browser", isEdge);

AtomicBoolean changingFinished = new AtomicBoolean(false);
AtomicBoolean changedFinished = new AtomicBoolean(false);
browser.addLocationListener(new LocationListener() {
Expand Down Expand Up @@ -1628,8 +1625,6 @@ public void changed(LocationEvent event) {
/** Verify that evaluation works inside an OpenWindowListener */
@Test
public void test_OpenWindowListener_evaluateInCallback() {
assumeFalse("behavior is not (yet) supported by Edge browser", isEdge);

AtomicBoolean eventFired = new AtomicBoolean(false);
browser.addOpenWindowListener(event -> {
browser.evaluate("SWTopenListener = true");
Expand Down Expand Up @@ -2189,7 +2184,6 @@ public void test_evaluate_array_mixedTypes () {
*/
@Test
public void test_BrowserFunction_callback () {
assumeFalse("Currently broken for Edge", isEdge);
AtomicBoolean javaCallbackExecuted = new AtomicBoolean(false);

class JavascriptCallback extends BrowserFunction { // Note: Local class defined inside method.
Expand Down
Loading