Edge: Freeze in Browser#evaluate() if evaluated script calls into SWT

[sratz]

Describe the bug
With Edge, when using browser.evaluate(script) and script somehow causing a callback into SWT, the evaluate() freezes / hangs in 100% CPU.

To Reproduce

@Test
public void test_EvaluateCallingIntoSwt() 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("""
			<button id="button" onClick="window.open('https://eclipse.org');">open eclipse.org</button>
			""");
	waitForPassCondition(initialLoad::get);

	browser.evaluate("""
				document.getElementById("button").click();
			""");

	waitForPassCondition(openWindowListenerCalled::get);
}

Expected behavior
Test succeeds with IE and Edge.

Actual behavior
Test hangs with Edge, succeeds with IE.
Stack trace shows:

Thread [main] (Suspended)	
	org.eclipse.swt.internal.win32.OS.PeekMessage(org.eclipse.swt.internal.win32.MSG, long, int, int, int) line: not available [native method]	
	org.eclipse.swt.browser.Edge.processNextOSMessage() line: 371	
	org.eclipse.swt.browser.Edge.callAndWait(java.lang.String[], java.util.function.ToIntFunction<org.eclipse.swt.internal.ole.win32.IUnknown>) line: 284	
	org.eclipse.swt.browser.Edge.evaluate(java.lang.String) line: 746	
	org.eclipse.swt.browser.Edge(org.eclipse.swt.browser.WebBrowser).evaluate(java.lang.String, boolean) line: 406	
	org.eclipse.swt.browser.Browser.evaluate(java.lang.String, boolean) line: 665	
	org.eclipse.swt.browser.Browser.evaluate(java.lang.String) line: 614	
	org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.test_EvaluateCallingIntoSwt() line: 2700	

Environment:

1. Select the platform(s) on which the behavior is seen:

• • All OS
• • Windows / Edge
• • Linux
• • macOS

2. Additional OS info (e.g. OS version, Linux Desktop, etc)

Additional context
This is a side-effect of #660, where inside Edge.callAndWait()
if (!display.readAndDispatch()) display.sleep();
was replaced with
processNextOSMessage();.

@HeikoKlare:
PR #660 suggest that this was to avoid a out-of-handles during initialization, i.e. inside createEnvironment(), but it also affects the evaluate() call negatively.
Do you think it would be reasonable to keep using display.readAndDispatch() in the latter?

[HeikoKlare]

Thank you for this report, @sratz! We are currently investigating potential reasons for Edge blocking the UI thread (according to eclipse-platform/eclipse.platform.ui#2734) starting with this PR:

• WIP: Edge Browser Scheduled Job timeout #1752

I also pointed to the calls to processNextOSMessage() there (#1752 (review)) as they might be blocking the UI thread. Now it's nice to have an actual reproducer of a potential issue from you!

In my opinion, we could/should replace all the calls to processNextOSMessage() and just do the previous if (!display.readAndDispatch()) display.sleep() again, as in every case (in particular the initialization) this is now run inside a loop anyway (as the initialization logic has changed and been made completely asynchronous since back then). So there is no need for peeking a message anymore.

[HeikoKlare]

I've created a draft PR with the idea for the improvement:

• [Win32] Spin event loop in Edge instead of only processing OS messages #1773

[laeubi]

@sratz @HeikoKlare I think the reproducer violates the basic contract of evaluate that is:

If document-defined functions or properties are accessed by the script then this method should not be invoked until the document has finished loading (ProgressListener.completed() gives notification of this).

As you invoke the evaluate synchronously with construction but access document.getElementById this is actually invalid and can at best lead to undefined value.

So that it works here is more maybe the bug, the browser should most likley fire the event of document load inside an asnyc exec and your code should look like this:

browser.addProgressListener(ProgressListener.completedAdapter(e ->{

	browser.evaluate("""
				document.getElementById("button").click();
			""");

}));

I can't really think of any (non deadlock prone) way to support async javascript loading and synchronous instantiation of a browsers document, and this is also the reason why this will never happen in a real web application and completed event is mandatory in JS to be used before accessing the dom.

[HeikoKlare]

Some more insights on the underlying cause of this: when opening a new window, the OS callback does not directly process this creation but, again, creates an async task to do it:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java

Lines 1249 to 1294 in 8f5e8fd

	int handleNewWindowRequested(long pView, long pArgs) {
	ICoreWebView2NewWindowRequestedEventArgs args = new ICoreWebView2NewWindowRequestedEventArgs(pArgs);
	args.AddRef();
	long[] ppv = new long[1];
	args.GetDeferral(ppv);
	ICoreWebView2Deferral deferral = new ICoreWebView2Deferral(ppv[0]);
	inNewWindow = true;
	browser.getDisplay().asyncExec(() -> {
	try {
	if (browser.isDisposed()) return;
	WindowEvent openEvent = new WindowEvent(browser);
	openEvent.display = browser.getDisplay();
	openEvent.widget = browser;
	openEvent.required = true;
	for (OpenWindowListener openListener : openWindowListeners) {
	openListener.open(openEvent);
	if (browser.isDisposed()) return;
	}
	if (openEvent.browser != null && !openEvent.browser.isDisposed()) {
	WebBrowser other = openEvent.browser.webBrowser;
	args.put_Handled(true);
	if (other instanceof Edge otherEdge) {
	args.put_NewWindow(otherEdge.webViewProvider.getWebView(true).getAddress());
	// Send show event to the other browser.
	WindowEvent showEvent = new WindowEvent (other.browser);
	showEvent.display = browser.getDisplay();
	showEvent.widget = other.browser;
	updateWindowFeatures(args, showEvent);
	for (VisibilityWindowListener showListener : other.visibilityWindowListeners) {
	showListener.show(showEvent);
	if (other.browser.isDisposed()) return;
	}
	}
	} else if (openEvent.required) {
	args.put_Handled(true);
	}
	} finally {
	deferral.Complete();
	deferral.Release();
	args.Release();
	inNewWindow = false;
	}
	});
	return COM.S_OK;
	}

Since our callAndWait() only processes OS events but does not process tasks scheduled via Display#asyncExec(), the actual window creation will never terminate. While being stuck in callAndWait() as in the original issue report, you see this event pending in the display's synchronizer:

This is the reason why spinning the event loop "solved" the problem, as it will execute that asynchronous task.

Simply performing the execution of that callback synchronously without creating an async task does not work either, as then other use cases (specified as tests) will get stuck.

[laeubi]

As mentioned here:

• WIP: Edge Browser Scheduled Job timeout #1752 (comment)

I think it is not appropriate to "join" a future unconditionally on the event thread (in any case) without special considerations/handling.