API changes/addition for asynchronous behaviour of new browsers

[amartya4256]

Hi,

I've been working on resolving the deadlock issue with the Edge browser. Let me explain the issue in detail.

In the embedded Edge browser, we use the WebView provided by Windows to execute tasks. By design, the WebView cannot execute tasks simultaneously; it serializes them instead. Now, imagine a scenario where the embedded browser is executing a task using the WebView and, during this process, we receive a callback from the WebView that requires executing another task. The new task cannot complete because the previous task is still running, and the previous task is waiting for the new one to finish, creating a deadlock.

For example, if you use the Edge browser and open the welcome page, then click on "Eclipse Platform" in the "What's New" tab, it opens a new shell, creates a new browser, and navigates to the relevant content. In this case, the creation of the new shell and browser occurs within a WebView callback, and since the browser creation requires another WebView call, a deadlock occurs.

In my PR: #1378, I implemented an asynchronous approach where I schedule new tasks asynchronously, allowing them to finish after the current task completes. This approach delays the WebView call to set up the browser environment until after the callback is completed. However, there are additional calls to set the URL or set text on the browser that occur after the browser is created, which need to ensure that they wait until the browser is fully initialized. These calls are also executed asynchronously if the browser's environment is not yet completely initialized.

The challenge arises with API compatibility. By design, APIs like execute, setURL, and setText of the Browser class return a boolean value (particularly for setText and setURL, which check if the request status is 200 OK). With asynchronous calls, this is not possible, so we need a way to use CompletableFuture for such requests.

In the PR, the implementation mimics synchronous behavior, but the return value for these async calls is explicitly set to true, which does not accurately reflect the intended behavior of these APIs. There are two proposals to address this problem:

• I reviewed the clients that call setText and setURL, and across Eclipse, none of them appear to consume the return value of these methods. Therefore, we could change the return type of setURL and setText to void.

• We could provide a new API that returns a Future and migrate the calls across Eclipse, deprecating setURL and setText. This approach would also allow us to adapt to other modern browsers facing similar issues with deadlocks and asynchronous calls.

Which approach do you think is better?

[HeikoKlare]

Some minor comments / additions for clarification:

• I reviewed the clients that call setText and setURL, and across Eclipse, none of them appear to consume the return value of these methods. Therefore, we could change the return type of setURL and setText to void.

Changing the return value will not be possible as it would break the API. Instead, the return values could still be boolean but in case the actual processing is done asynchronously, the methods might always return true. Since the return value does usually not seem be used, this will not be a problem. Effectively, for browsers like Edge, the method behavior would change from setText/setUrl to requestSetText/requestSetUrl or the like, indicating that the actual operation might be performed asynchronously.

• We could provide a new API that returns a Future and migrate the calls across Eclipse, deprecating setURL and setText. This approach would also allow us to adapt to other modern browsers facing similar issues with deadlocks and asynchronous calls.

For existing browsers with a synchronous API (like IE), the new and existing methods would behave the same. I.e., a future returned by a new API would directly yield a value as the execution has already finished synchronously.

The two approaches could even be combined:

• We could provide new proper API and deprecate the old one, such that clients know that they have to migrate.
• The existing browser implementations can stay as they are with the newly added methods behaving equally if the return value (future vs. boolean) is unused.
• For new browsers (like Edge) the existing (deprecated) API may not be implemented properly but simply behave asynchronous as well. As long as consumers do not rely on the return value of operations like setUrl/setText, this will not be a problem.

[laeubi]

I would just add new methods updateUrl / updateText that return a CompletableFuture<Result>, then Result can carry even additional information e.g. exceptions, error code, whatever and it is clear for the caller that the actual work happen (possibly) asynchronous.

Existing method can then call return update().join().isSuccess() ...

[sratz]

Another option could be some common, public API adapter interfaces, that the underlying internal WebBrowser implementations could implement, e.g.

class Edge extends WebBrowser implements IAsyncWebBrowser {
  // ...
}

public interface IAsyncWebBroser {
  CompletableFuture<Result>updateUrl(String url);
  // ...
}

which could be used like this:

Browser browser = new Browser(parent, SWT.EDGE);
IAsyncWebBroser asyncBrowser = browser.getAdapter(IAsyncWebBroser.class);
if (asyncBrowser != null) {
  asyncBrowser.updateUrl(url);
}

This concept could be used in a fine-granular interface fashion for all sorts of use-cases where we want to expose platform- / implementation-specific functionality behind platform-agnostic API.

[laeubi]

While I like that pattern I think in this particular case it is too cumbersome. e.g. if a browser does not support async it can simply return a completed future, otherwise one needs to have these checks all over the place and as mentions already at the moment no one really cares about the result anyways.

[HeikoKlare]

Thanks for your input. In general, I also like that pattern, but it puts an addition burden on clients, which should actually be unnecessary if the API was designed properly.

In today's dev community call, @akurtakov pointed to the WebKit implementation in SWT already dealing with the asynchronous API of WebKit. Looking at the implementation shows that it has the exact same challenge than Edge. Methods like setText and setUrl were solved by basically always returning true and delegating delegate asynchronous requests to WebKit, e.g.:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT WebKit/gtk/org/eclipse/swt/browser/WebKit.java

Lines 1918 to 1932 in 248ab17

	public boolean setText (String html, boolean trusted) {
	/* convert the String containing HTML to an array of bytes with UTF-8 data */
	byte[] html_bytes = (html + '\0').getBytes (StandardCharsets.UTF_8); //$NON-NLS-1$
	w2_bug527738LastRequestCounter.incrementAndGet();
	byte[] uriBytes;
	if (!trusted) {
	uriBytes = Converter.wcsToMbcs (ABOUT_BLANK, true);
	} else {
	uriBytes = Converter.wcsToMbcs (URI_FILEROOT, true);
	}
	WebKitGTK.webkit_web_view_load_html (webView, html_bytes, uriBytes);
	return true;
	}

For evaluate(), an async-to-sync handler (Webkit2AsyncToSync) was introduced. It should be possible to implement Edge in the same or a similar way, in particular in terms of asynchronous behavior. Since existing consumers seem to work fine with the WebKit implementation for years, there are no reasons for requiring something different for Edge.

That means, in my opinion, the basic pattern in #1378 is valid and can be extended.