Edge: unexpected concurrent processing of OS messages

[HeikoKlare]

The Edge implementation for using WebView2 in SWT employs a reduced event queue processing in processOSMessagesUntil() to process callback for initialization and other tasks that subsequent operations on an Edge instance have to wait for. The method is supposed to only process messages coming from the OS and not to process any tasks asynchronously scheduled at the display. To this end, the method performs an OS.PeekMessage() before calling display.readAndDisplay() as that is supposed to only process a single OS message (on Windows):

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

Lines 467 to 471 in 4a201e2

	if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE)) {
	display.readAndDispatch();
	} else {
	display.sleep();
	}

This assumption seems to be broken. I found the following call sequence in an exception during local browser test execution, which shows that when calling display.readAndDisplay(), messages in the synchronizer are processed instead of an OS message (which should be there because of the previous condition check and which should be processed instead of the synchornizer messages when it is there):

	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4077)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3693)
	at org.eclipse.swt.browser.Edge.processOSMessagesUntil(Edge.java:468)

By concept, this should not happen. There are two potential reasons for this:

• The assumption that display.readAndDispatch() only processes an OS message in case one is present does not hold
• Some other thread processed the OS message concurrently

So we should investigate why the conceptual assumptions of the Edge implementation do not hold.

This is the full strack trace of the test for reference:

java.util.concurrent.CancellationException
	at java.base/java.util.concurrent.CompletableFuture.cancel(CompletableFuture.java:2478)
	at org.eclipse.swt.browser.Edge$WebViewProvider.abortInitialization(Edge.java:320)
	at org.eclipse.swt.browser.Edge.lambda$14(Edge.java:602)
	at org.eclipse.swt.browser.Edge.lambda$15(Edge.java:608)
	at org.eclipse.swt.browser.Edge.lambda$5(Edge.java:238)
	at org.eclipse.swt.internal.win32.OS.DestroyWindow(Native Method)
	at org.eclipse.swt.widgets.Control.destroyWidget(Control.java:740)
	at org.eclipse.swt.widgets.Shell.destroyWidget(Shell.java:736)
	at org.eclipse.swt.widgets.Widget.release(Widget.java:907)
	at org.eclipse.swt.widgets.Widget.dispose(Widget.java:479)
	at org.eclipse.swt.widgets.Decorations.dispose(Decorations.java:396)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.lambda$4(Test_org_eclipse_swt_browser_Browser.java:337)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4077)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3693)
	at org.eclipse.swt.browser.Edge.processOSMessagesUntil(Edge.java:468)
	at org.eclipse.swt.browser.Edge$WebViewProvider.getWebViewWrapper(Edge.java:370)
	at org.eclipse.swt.browser.Edge$WebViewProvider.getWebView(Edge.java:381)
	at org.eclipse.swt.browser.Edge.getUrl(Edge.java:893)
	at org.eclipse.swt.browser.Browser.getUrl(Browser.java:771)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.createBrowser(Test_org_eclipse_swt_browser_Browser.java:311)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.test_Constructor_asyncParentDisposal(Test_org_eclipse_swt_browser_Browser.java:339)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.junit.runners.Suite.runChild(Suite.java:128)
	at org.junit.runners.Suite.runChild(Suite.java:27)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:93)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:520)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:748)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:443)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:211)

[HeikoKlare]

The implementation of processOSMessagesUntil seems to make a wrong assumption: calling OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE) will not only check the OS queue for events being queued but also does some processing which we do not expect (see https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-peekmessagea). The expectation means that if we called OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE) multiple times, we would expect the state of the system after each call to be the same. But changing the code as follows shows that this is not the case, as with that code we will usually run into the exception:

		if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE)) {
			int store = msg.message;
			OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE);
			if (store != msg.message) {
				throw new IllegalStateException();
			}
			display.readAndDispatch();
		} else {
			display.sleep();
		}

Limiting the processing to posted messages, i.e., using OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE | PM_QS_POSTMESSAGE) may solve the issue.

[fedejeanne]

It's not really clear to me PeekMessage really works but one thing is true (as you said): the state of the queue may change by simply calling the method (see Remarks).

If you're interested in merely knowing if there is something in the queue, maybe GetQueueStatus can help? I see that it has some useful parameters to check for several types of messages, like QS_POSTMESSAGE, QS_ALLPOSTMESSAGE, QS_ALLINPUT or QS_ALLEVENTS (among others).

[HeikoKlare]

GetQueueStatus() is an interesting pointer, as it fits to the purpose of what PeekMessage() is currently used for.

Unfortunately, the documentation states that the result will only be hint and there is no guarantee that a subsequent GetMessage() will actually return a message (https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getqueuestatus#remarks):

The presence of a QS_ flag in the return value does not guarantee that a subsequent call to the GetMessage or PeekMessage function will return a message. GetMessage and PeekMessage perform some internal filtering that may cause the message to be processed internally. For this reason, the return value from GetQueueStatus should be considered only a hint as to whether GetMessage or PeekMessage should be called.

So we may run into the same issue again.