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

[HeikoKlare]

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 synchronously 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.

[github-actions]

Test Results

   492 files   -   2     492 suites   - 2   7m 15s ⏱️ - 3m 41s
 4 107 tests  - 226   4 100 ✅  - 220   7 💤  -  6  0 ❌ ±0 
16 160 runs   - 414  16 068 ✅  - 398  92 💤  - 16  0 ❌ ±0 

Results for commit 7a44e53. ± Comparison against base commit 1b2ea28.

This pull request removes 227 and adds 1 tests. Note that renamed tests count towards both.

org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_afterPageReload[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_with_String[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_with_boolean[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_with_integer[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_with_javaReturningInt[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_with_javaReturningString[browser flags: 524,288]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_BrowserFunction_callback_with_multipleValues[browser flags: 524,288]
…

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_evaluate_callingIntoSwt[browser flags: 0]

This pull request skips 1 test.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_Constructor_multipleInstantiationsInDifferentThreads[browser flags: 0]

♻️ This comment has been updated with latest results.

[sratz]

Looks good. Great to see that removing the old workarounds is possible now.

[HeikoKlare]

Thank you, @sratz! I wanted to ping you on this anyway, so great that you already took a look! As you have probably seen, I have also incorporated the regression test you provided in #1771 in a separate commit with you as an author. I hope that's fine for you.

One additional information on this change for which feedback would be good: it introduces a slight change in behavior. Edge now processes all kinds of events scheduled by OS or on the display during initialization and other operations requiring event queue processing. One change in behavior due to this can be seen in the test change. As simplified version is this:

Display.getCurrent().asyncExec(() -> {
	shell.dispose();
});
Browser b = new Browser(shell, SWT.None);
b.getUrl();

Before this change, that code initialized a browser and getUrl() succeeded, as the disposal was only processed when some code spinned the event queue later on. With this change, the disposal is already processed during browser initialization (waited for when calling getUrl()), such that getUrl() will fail as the browser was never fully initialized due to concurrent disposal. I tested the different use cases that failed to work two years ago (see #339) and they still work fine. And it actually appears to be a bit strange to me to execute operations on the browser while the UI elements in which it is embedded are still being intialized. In other words: I would expect that between browser initialization and calls to blocking functionality (like setting/getting URL, evaluating scripts etc), the event loop will be spinned in productive use anyway. So I do not consider this a problem in productive use. Still, any feedback on this would be welcome.

[sratz]

I have also incorporated the regression test you provided in #1771 in a separate commit with you as an author. I hope that's fine for you.

Sure, was going to suggest to include that test, anyways :)

it introduces a slight change in behavior

Yes, I have thought about the constructor behavior as well. I was wondering if we can at least make that one somehow synchronous. I think currently it always succeeds and all initialization errors go to nirvana.

I guess this is acceptable for now.

[laeubi]

@HeikoKlare I think this is actually a bit dangerous as it can have some side effects (as you already noticed) and code must not assume that a control gets disposed in any way while being in the event thread, e.g. your example w ill not only dispose the browser but all controls and might trigger even more code to run.

Just assume the example you given but not dispose anything but open a new blocking dialog, then construction of the browser control will suddenly open the dialog even though that from program flow it is not to be expected in any way.

So I wonder what are these

some asynchronously scheduled tasks need to be processed but are not.

that are needed at init time of the browser? Can the possibly run once in the firs paint event or similar? Or do they need to run in the UI thread anyways?

[HeikoKlare]

Yes, I have thought about the constructor behavior as well. I was wondering if we can at least make that one somehow synchronous. I think currently it always succeeds and all initialization errors go to nirvana.

Actually it's not only about the constructor, but also about evalute calls, which will now spin the event loop and process any kind of events at a place, where it might not be expected. Without spinning the event loop, we will not be able to process situation like in the example of #1771. To me, it looks as if evalute would actually have to be processed asynchronously, but since that method returns the result of the evaluation, we cannot easily change that without a breaking API change. It reminds me of the deadlock detection functionality we had before making Edge initialization and some other operations asynchronous.

I agree with @laeubi that processing the whole event loop (and not only OS events) might be dangerous here. I will take a further look at the WebKit implementation, as there are similiar implementations in the Webkit2AsyncToSync class, also using timeouts to avoid UI blocks (which we should adapt anyway). And there must also be some mechanism to deal with the situation in the test, as the test succeeds on GTK.

[HeikoKlare]

Converted this back to draft, as we agreed that the fix is questionable. Thus, #1771 remains an issue for now and potentially needs a different kind of fix.
At least, the test will now fail with a timeout after instead of non-terminating:

• [Win32] Avoid blocking operations on Edge due to OS message processing #1776