From 3f39fdbd777829ead93aefe90bb6db85b1f64cad Mon Sep 17 00:00:00 2001 From: Amartya Parijat Date: Tue, 8 Apr 2025 13:47:22 +0200 Subject: [PATCH] Edge:evaluate deadlock fix #1466 This commit cotributes to replicating the behaviour of Edge:evaluate as it is in WebKit - it must not wait for the execution of script to obtain the result, in case evaluate is called inside a WebView callback. This commit also makes sure that OpenWindowListeners are execute synchronously or asynchronously depending on if the handleNewWindowRequested is called from the evaluate script. Moreover, this enables all the tests which were failing because of Edge:evaluate limitations. contributes to https://github.com/eclipse-platform/eclipse.platform.swt/issues/1771 and https://github.com/eclipse-platform/eclipse.platform.swt/issues/1919 --- .../win32/org/eclipse/swt/browser/Edge.java | 35 ++++++++++++++++--- .../Test_org_eclipse_swt_browser_Browser.java | 6 ---- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java b/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java index 53e90959efc..d3eaf1ba4cd 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java +++ b/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java @@ -81,6 +81,7 @@ public WebViewEnvironment(ICoreWebView2Environment environment) { static boolean inCallback; boolean inNewWindow; + private boolean inEvaluate; HashMap navigations = new HashMap<>(); private boolean ignoreGotFocus; private boolean ignoreFocusIn; @@ -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)) { @@ -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); @@ -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; } diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java index b75d08bc6ee..3ef33a60502 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java +++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java @@ -745,7 +745,6 @@ public void changed(LocationEvent event) { @Test public void test_LocationListener_LocationListener_ordered_changing () { - assumeFalse("Currently broken for Edge", isEdge); List locations = Collections.synchronizedList(new ArrayList<>()); browser.addLocationListener(changingAdapter(event -> { locations.add(event.location); @@ -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() { @@ -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"); @@ -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.