From 632e54cb6c0c43a9fc6afcf16b6928bde4d72849 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 27 Jan 2025 21:31:12 +0100 Subject: [PATCH 1/3] [Win32] Correct WebView initialization retry logic in Edge WebView initialization may fail in some cases, for which the Microsoft documentation proposes to retry initialization. The Edge implementation already covers that case, but accidentally cancels the initialization of the Edge instance even though a retry of initializing the contained WebView is triggered. This change properly separates actual initialization abortions and retries and corrects the retry functionality. --- .../win32/org/eclipse/swt/browser/Edge.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 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 48fb51bea9d..07a5f19a148 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 @@ -588,20 +588,23 @@ private void createInstance(int previousAttempts) { private IUnknown createControllerInitializationCallback(int previousAttempts) { Runnable initializationRollback = () -> { - webViewProvider.abortInitialization(); if (environment2 != null) { environment2.Release(); environment2 = null; } containingEnvironment.instances().remove(this); }; + Runnable initializationAbortion = () -> { + webViewProvider.abortInitialization(); + initializationRollback.run(); + }; return newCallback((result, pv) -> { if (browser.isDisposed()) { - initializationRollback.run(); + initializationAbortion.run(); return COM.S_OK; } if (result == OS.HRESULT_FROM_WIN32(OS.ERROR_INVALID_STATE)) { - initializationRollback.run(); + initializationAbortion.run(); SWT.error(SWT.ERROR_INVALID_ARGUMENT, null, " Edge instance with same data folder but different environment options already exists"); } @@ -611,11 +614,11 @@ private IUnknown createControllerInitializationCallback(int previousAttempts) { setupBrowser((int) result, pv); break; case COM.E_WRONG_THREAD: - initializationRollback.run(); + initializationAbortion.run(); error(SWT.ERROR_THREAD_INVALID_ACCESS, (int) result); break; case COM.E_ABORT: - initializationRollback.run(); + initializationAbortion.run(); break; default: initializationRollback.run(); From 2772617ebe1f57288499887e89da328850fd552a Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 27 Jan 2025 21:31:12 +0100 Subject: [PATCH 2/3] [Win32] Spin event loop in Edge instead of processing OS messages #1771 Before recent improvements of the Edge implementation, it was possible that a browser instance was disposed during WebView initialization, which made the initialization fail exceptionally. This has been worked around by not processing all kinds of asynchronously scheduled events (like a disposal) while waiting for WebView initialization but only those being processes by the OS event queue. This was still necessary to process the OS callbacks for WebView initialization and other operations. In some cases, this may lead to an Edge browser instance blocking the UI thread, as some asynchronously scheduled tasks need to be processed but are not. In addition, enhancements of the Edge implementation made the initialization happen asynchronously anyway, such that the browser instantiation itself cannot throw an error anymore, but the asynchronously happening initialization is now capable of processing asynchronous browser disposals. This change simplifies the event processing inside Edge to not only process the next OS message but to just ordinarily spin the event loop. This reduces the risk of operations executed on an Edge browser to block execution. https://github.com/eclipse-platform/eclipse.platform.swt/issues/1771 --- .../win32/org/eclipse/swt/browser/Edge.java | 24 ++++--------------- .../Test_org_eclipse_swt_browser_Browser.java | 21 +++++++--------- 2 files changed, 13 insertions(+), 32 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 07a5f19a148..96f328f3f36 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 @@ -261,7 +261,7 @@ static int callAndWait(long[] ppv, ToIntFunction callable) { // "completion" callback may be called asynchronously, // so keep processing next OS message that may call it while (phr[0] == COM.S_OK && ppv[0] == 0) { - processNextOSMessage(); + spinEventLoop(); } completion.Release(); return phr[0]; @@ -281,7 +281,7 @@ static int callAndWait(String[] pstr, ToIntFunction callable) { // "completion" callback may be called asynchronously, // so keep processing next OS message that may call it while (phr[0] == COM.S_OK && pstr[0] == null) { - processNextOSMessage(); + spinEventLoop(); } completion.Release(); return phr[0]; @@ -444,31 +444,17 @@ void scheduleWebViewTask(Runnable action) { private void waitForFutureToFinish(CompletableFuture future) { while(!future.isDone()) { - processNextOSMessage(); + spinEventLoop(); } } } -/** - * Processes a single OS message using {@link Display#readAndDispatch()}. This - * is required for processing the OS events during browser initialization, since - * Edge browser initialization happens asynchronously. - *

- * {@link Display#readAndDispatch()} also processes events scheduled for - * asynchronous execution via {@link Display#asyncExec(Runnable)}. This may - * include events such as the disposal of the browser's parent composite, which - * leads to a failure in browser initialization if processed in between the OS - * events for initialization. Thus, this method does not implement an ordinary - * readAndDispatch loop, but waits for an OS event to be processed. - */ -private static void processNextOSMessage() { +private static void spinEventLoop() { Display display = Display.getCurrent(); - MSG msg = new MSG(); - while (!OS.PeekMessage (msg, 0, 0, 0, OS.PM_NOREMOVE)) { + if (!display.readAndDispatch()) { display.sleep(); } - display.readAndDispatch(); } static ICoreWebView2CookieManager getCookieManager() { 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 f76da4c2d77..29c27001d78 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 @@ -81,7 +81,6 @@ import org.eclipse.swt.widgets.Shell; import org.eclipse.swt.widgets.Text; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.FixMethodOrder; import org.junit.Ignore; import org.junit.Rule; @@ -158,16 +157,6 @@ public Test_org_eclipse_swt_browser_Browser(int swtBrowserSettings) { this.swtBrowserSettings = swtBrowserSettings; } -@BeforeClass -public static void setupEdgeEnvironment() { - // initialize Edge environment before any test runs to isolate environment setup - if (SwtTestUtil.isWindows) { - Shell shell = new Shell(); - new Browser(shell, SWT.EDGE); - shell.dispose(); - } -} - @Override @Before public void setUp() { @@ -302,11 +291,17 @@ private int reportOpenedDescriptors() { } private Browser createBrowser(Shell s, int flags) { + return createBrowser(s, flags, true); +} + +private Browser createBrowser(Shell s, int flags, boolean expectSuccess) { long maximumBrowserCreationMilliseconds = 90_000; long createStartTime = System.currentTimeMillis(); Browser b = new Browser(s, flags); // Wait for asynchronous initialization via getting URL - b.getUrl(); + if (expectSuccess) { + b.getUrl(); + } createdBroswers.add(b); long createDuration = System.currentTimeMillis() - createStartTime; assertTrue("creating browser took too long: " + createDuration + "ms", createDuration < maximumBrowserCreationMilliseconds); @@ -334,7 +329,7 @@ public void test_Constructor_asyncParentDisposal() { Display.getCurrent().asyncExec(() -> { shell.dispose(); }); - Browser browser = createBrowser(shell, swtBrowserSettings); + Browser browser = createBrowser(shell, swtBrowserSettings, false); assertFalse(browser.isDisposed()); } From 7a44e5390a0d8459fff8105a51c4fdefc95b7b8c Mon Sep 17 00:00:00 2001 From: Sebastian Ratz Date: Tue, 28 Jan 2025 21:32:51 +0100 Subject: [PATCH 3/3] Add browser regression test for calling evaluate requiring event loop Regression test for https://github.com/eclipse-platform/eclipse.platform.swt/issues/1771 --- .../Test_org_eclipse_swt_browser_Browser.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 29c27001d78..d657d67bab2 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 @@ -429,6 +429,27 @@ public void test_evalute_Cookies () { assertFalse(res.isEmpty()); } +@Test +public void test_evaluate_callingIntoSwt() throws Exception { + AtomicBoolean initialLoad = new AtomicBoolean(); + AtomicBoolean openWindowListenerCalled = new AtomicBoolean(); + browser.addProgressListener(ProgressListener.completedAdapter(e -> initialLoad.set(true))); + browser.addOpenWindowListener(event -> { + event.required = true; // block default + openWindowListenerCalled.set(true); + }); + browser.setText(""" + + """); + waitForPassCondition(initialLoad::get); + + browser.evaluate(""" + document.getElementById("button").click(); + """); + + waitForPassCondition(openWindowListenerCalled::get); +} + @Test public void test_ClearAllSessionCookies () { // clearSessions will only work for Webkit2 when >= 2.16