Skip to content

Commit

Permalink
[Win32] Avoid blocking operations on Edge due to OS message processing
Browse files Browse the repository at this point in the history 
The Edge browser implementation currently spins the event queue without
any possibility to escape in case no OS message or not the expected OS
message to proceed occurs and is processed.

In order to avoid that operations called on Edge deadlock, this change
introduces timeouts for OS event processing inside Edge:
- The existing processNextOSMessage() method is replaced with
processOSMessagesUntil(condition) that spins the event queue until the
condition is met or a timeout occurred
- CompletableFutures used for the queuing events on the WebView instance
are extended to wake the display in order to avoid that the OS message
processing is unnecessary sleeping until a timeout wakes it up
  • Loading branch information
@HeikoKlare @fedejeanne
HeikoKlare authored and fedejeanne committed Jan 30, 2025
1 parent 9aa7973 commit 7ad097a
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 61 deletions.
117 changes: 60 additions & 57 deletions bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.time.*;
import java.util.*;
import java.util.concurrent.*;
import java.util.concurrent.atomic.*;
import java.util.function.*;

import org.eclipse.swt.*;
Expand All @@ -42,6 +43,7 @@ class Edge extends WebBrowser {
static final String APPLOCAL_DIR_KEY = "org.eclipse.swt.internal.win32.appLocalDir";
static final String EDGE_USER_DATA_FOLDER = "org.eclipse.swt.internal.win32.Edge.userDataFolder";
static final String EDGE_USE_DARK_PREFERED_COLOR_SCHEME = "org.eclipse.swt.internal.win32.Edge.useDarkPreferedColorScheme"; //$NON-NLS-1$
static final String WEB_VIEW_OPERATION_TIMEOUT = "org.eclipse.swt.internal.win32.Edge.timeout"; //$NON-NLS-1$

// System.getProperty() keys
static final String BROWSER_DIR_PROP = "org.eclipse.swt.browser.EdgeDir";
Expand All @@ -58,6 +60,7 @@ class Edge extends WebBrowser {
private static final String ABOUT_BLANK = "about:blank";

private static final int MAXIMUM_CREATION_RETRIES = 5;
private static final Duration MAXIMUM_OPERATION_TIME = Duration.ofMillis(Integer.getInteger(WEB_VIEW_OPERATION_TIMEOUT, 5_000));

private record WebViewEnvironment(ICoreWebView2Environment environment, ArrayList<Edge> instances) {
public WebViewEnvironment(ICoreWebView2Environment environment) {
Expand Down Expand Up @@ -260,14 +263,12 @@ static int callAndWait(long[] ppv, ToIntFunction<IUnknown> callable) {
phr[0] = callable.applyAsInt(completion);
// "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();
}
processOSMessagesUntil(() -> phr[0] != COM.S_OK || ppv[0] != 0, Display.getCurrent());
completion.Release();
return phr[0];
}

static int callAndWait(String[] pstr, ToIntFunction<IUnknown> callable) {
int callAndWait(String[] pstr, ToIntFunction<IUnknown> callable) {
int[] phr = new int[1];
IUnknown completion = newCallback((result, pszJson) -> {
phr[0] = (int)result;
Expand All @@ -280,9 +281,7 @@ static int callAndWait(String[] pstr, ToIntFunction<IUnknown> callable) {
phr[0] = callable.applyAsInt(completion);
// "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();
}
processOSMessagesUntil(() -> phr[0] != COM.S_OK || pstr[0] != null, browser.getDisplay());
completion.Release();
return phr[0];
}
Expand All @@ -298,7 +297,7 @@ class WebViewWrapper {

class WebViewProvider {

private CompletableFuture<WebViewWrapper> webViewWrapperFuture = new CompletableFuture<>();
private CompletableFuture<WebViewWrapper> webViewWrapperFuture = wakeDisplayAfterFuture(new CompletableFuture<>());
private CompletableFuture<Void> lastWebViewTask = webViewWrapperFuture.thenRun(() -> {});;

ICoreWebView2 initializeWebView(ICoreWebView2Controller controller) {
Expand Down Expand Up @@ -365,95 +364,91 @@ private ICoreWebView2_13 initializeWebView_13(ICoreWebView2 webView) {
return null;
}

ICoreWebView2 getWebView(boolean waitForPendingWebviewTasksToFinish) {
private WebViewWrapper getWebViewWrapper(boolean waitForPendingWebviewTasksToFinish) {
if(waitForPendingWebviewTasksToFinish) {
waitForFutureToFinish(lastWebViewTask);
processOSMessagesUntil(lastWebViewTask::isDone, browser.getDisplay());
}
return webViewWrapperFuture.join().webView;
return webViewWrapperFuture.join();
}

private WebViewWrapper getWebViewWrapper() {
processOSMessagesUntil(webViewWrapperFuture::isDone, browser.getDisplay());
return webViewWrapperFuture.join();
}

ICoreWebView2 getWebView(boolean waitForPendingWebviewTasksToFinish) {
return getWebViewWrapper(waitForPendingWebviewTasksToFinish).webView;
}

ICoreWebView2_2 getWebView_2(boolean waitForPendingWebviewTasksToFinish) {
if(waitForPendingWebviewTasksToFinish) {
waitForFutureToFinish(lastWebViewTask);
}
return webViewWrapperFuture.join().webView_2;
return getWebViewWrapper(waitForPendingWebviewTasksToFinish).webView_2;
}

boolean isWebView_2Available() {
waitForFutureToFinish(webViewWrapperFuture);
return webViewWrapperFuture.join().webView_2 != null;
return getWebViewWrapper().webView_2 != null;
}

ICoreWebView2_10 getWebView_10(boolean waitForPendingWebviewTasksToFinish) {
if(waitForPendingWebviewTasksToFinish) {
waitForFutureToFinish(lastWebViewTask);
}
return webViewWrapperFuture.join().webView_10;
return getWebViewWrapper(waitForPendingWebviewTasksToFinish).webView_10;
}

boolean isWebView_10Available() {
waitForFutureToFinish(webViewWrapperFuture);
return webViewWrapperFuture.join().webView_10 != null;
return getWebViewWrapper().webView_10 != null;
}

ICoreWebView2_11 getWebView_11(boolean waitForPendingWebviewTasksToFinish) {
if(waitForPendingWebviewTasksToFinish) {
waitForFutureToFinish(lastWebViewTask);
}
return webViewWrapperFuture.join().webView_11;
return getWebViewWrapper(waitForPendingWebviewTasksToFinish).webView_11;
}

boolean isWebView_11Available() {
waitForFutureToFinish(webViewWrapperFuture);
return webViewWrapperFuture.join().webView_11 != null;
return getWebViewWrapper().webView_11 != null;
}

ICoreWebView2_12 getWebView_12(boolean waitForPendingWebviewTasksToFinish) {
if(waitForPendingWebviewTasksToFinish) {
waitForFutureToFinish(lastWebViewTask);
}
return webViewWrapperFuture.join().webView_12;
return getWebViewWrapper(waitForPendingWebviewTasksToFinish).webView_12;
}

boolean isWebView_12Available() {
waitForFutureToFinish(webViewWrapperFuture);
return webViewWrapperFuture.join().webView_12 != null;
return getWebViewWrapper().webView_12 != null;
}

ICoreWebView2_13 getWebView_13(boolean waitForPendingWebviewTasksToFinish) {
if(waitForPendingWebviewTasksToFinish) {
waitForFutureToFinish(lastWebViewTask);
}
return webViewWrapperFuture.join().webView_13;
return getWebViewWrapper(waitForPendingWebviewTasksToFinish).webView_13;
}

boolean isWebView_13Available() {
waitForFutureToFinish(webViewWrapperFuture);
return webViewWrapperFuture.join().webView_13 != null;
return getWebViewWrapper().webView_13 != null;
}

/*
* Schedule a given runnable in a queue to execute when the webView is free and
* has finished all the pending tasks queued before it.
*/
void scheduleWebViewTask(Runnable action) {
lastWebViewTask = lastWebViewTask.thenRun(() -> {
action.run();
lastWebViewTask = wakeDisplayAfterFuture(lastWebViewTask.thenRun(action::run));
}

private <T> CompletableFuture<T> wakeDisplayAfterFuture(CompletableFuture<T> future) {
return future.handle((nil1, nil2) -> {
Display display = browser.getDisplay();
if (!display.isDisposed()) {
try {
display.wake();
} catch (SWTException e) {
// ignore then, this can happen due to the async nature between our check for
// disposed and the actual call to wake the display can be disposed
}
}
return null;
});
}

private <T> void waitForFutureToFinish(CompletableFuture<T> future) {
while(!future.isDone()) {
processNextOSMessage();
}
}

}

/**
* Processes a single OS message using {@link Display#readAndDispatch()}. This
* Processes single OS messages using {@link Display#readAndDispatch()}. This
* is required for processing the OS events during browser initialization, since
* Edge browser initialization happens asynchronously.
* Edge browser initialization happens asynchronously. Messages are processed
* until the given condition is fulfilled or a timeout occurs.
* <p>
* {@link Display#readAndDispatch()} also processes events scheduled for
* asynchronous execution via {@link Display#asyncExec(Runnable)}. This may
Expand All @@ -462,13 +457,21 @@ private <T> void waitForFutureToFinish(CompletableFuture<T> future) {
* 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() {
Display display = Display.getCurrent();
private static void processOSMessagesUntil(Supplier<Boolean> condition, Display display) {
MSG msg = new MSG();
while (!OS.PeekMessage (msg, 0, 0, 0, OS.PM_NOREMOVE)) {
display.sleep();
AtomicBoolean timeoutOccurred = new AtomicBoolean();
// The timer call also wakes up the display to avoid being stuck in display.sleep()
display.timerExec((int) MAXIMUM_OPERATION_TIME.toMillis(), () -> timeoutOccurred.set(true));
while (!display.isDisposed() && !condition.get() && !timeoutOccurred.get()) {
if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE)) {
display.readAndDispatch();
} else {
display.sleep();
}
}
if (!condition.get()) {
SWT.error(SWT.ERROR_UNSPECIFIED, null, " Waiting for Edge operation to terminate timed out");
}
display.readAndDispatch();
}

static ICoreWebView2CookieManager getCookieManager() {
Expand Down
12 changes: 8 additions & 4 deletions ...t.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -101,12 +102,16 @@
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public class Test_org_eclipse_swt_browser_Browser extends Test_org_eclipse_swt_widgets_Composite {

// TODO Reduce to reasonable value
private static Duration MAXIMUM_BROWSER_CREATION_TIME = Duration.ofSeconds(90);

static {
try {
printSystemEnv();
} catch (Exception e) {
e.printStackTrace();
}
System.setProperty("org.eclipse.swt.internal.win32.Edge.timeout", Long.toString(MAXIMUM_BROWSER_CREATION_TIME.toMillis()));
}

// CONFIG
Expand Down Expand Up @@ -296,14 +301,13 @@ private int reportOpenedDescriptors() {
}

private Browser createBrowser(Shell s, int flags) {
long maximumBrowserCreationMilliseconds = 90_000;
long createStartTime = System.currentTimeMillis();
Instant createStartTime = Instant.now();
Browser b = new Browser(s, flags);
// Wait for asynchronous initialization via getting URL
b.getUrl();
createdBroswers.add(b);
long createDuration = System.currentTimeMillis() - createStartTime;
assertTrue("creating browser took too long: " + createDuration + "ms", createDuration < maximumBrowserCreationMilliseconds);
Duration createDuration = Duration.between(createStartTime, Instant.now());
assertTrue("creating browser took too long: " + createDuration.toMillis() + "ms", createDuration.minus(MAXIMUM_BROWSER_CREATION_TIME).isNegative());
return b;
}

Expand Down

0 comments on commit 7ad097a

Please sign in to comment.