Fix Edge Browser Deadlock issue for instantiation during a WebView Callback #1378

amartya4256 · 2024-08-02T11:54:45Z

This contribution fixes Edge browser for win32 to handle the deadlock issue during the instantiation of a new edge browser object amidst a webview callback.

The problem with webview is that it can't execute tasks simultaneously and all the requests are serialized. Hence, a new request to the webview inside its callback leads to a deadlock since, both the tasks wait for each other to execute. The solution is to schedule the Webview calls during the callback asynchronously and let the control in the callback move on.

contributes to #669

Scope not covered

This implementation doesn't fix the evaluate calls during the webview callback and would be addressed in a separate issue.

Pre-requisites

Handle local resource references in Edge#setText() #1395

Testing

Change the method org.eclipse.swt.browser.BrowserFactory.createWebBrowser to:

WebBrowser createWebBrowser (int style) {
	return new Edge ();
}

github-actions · 2024-08-02T12:03:14Z

Test Results

486 files ±0 486 suites ±0 6m 55s ⏱️ - 1m 22s
4 159 tests +1 4 151 ✅ +1 8 💤 ±0 0 ❌ ±0
16 390 runs +4 16 298 ✅ +4 92 💤 ±0 0 ❌ ±0

Results for commit 1f22a98. ± Comparison against base commit 3c8ed8e.

♻️ This comment has been updated with latest results.

Phillipus · 2024-08-02T15:56:34Z

The temp file is not deleted on exit. Temp files on Windows can accumulate over time and Windows never deletes them, so perhaps file.deleteOnExit() could be added?

Phillipus · 2024-08-02T17:35:12Z

This PR creates a temporary file and replaces the original implementation of Edge#setText() in all cases, even where Edge#setText() might not need it in the case of very simple HTML (no references to local resources). Would an additional setText method work rather than replacing the existing one?

And Edge#getText() doesn't return the correct contents. Run the following snippet:

import org.eclipse.swt.SWT;
import org.eclipse.swt.browser.Browser;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;

public class SWTBrowserTest {
    
    public static void main(String[] args) {
        final Display display = new Display();

        final Shell shell = new Shell(display);
        shell.setText("SWT Browser");
        shell.setLayout(new FillLayout());
        shell.setSize(1024, 768);
        
        String html = makeHTMLEntry();
        System.out.println(html);
        
        Browser browser = new Browser(shell, SWT.EDGE);
        browser.setText(html);
        
        System.out.println(browser.getText());

        shell.open();
        
        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) {
                display.sleep();
            }
        }

        display.dispose();
    }
    
    private static String makeHTMLEntry() {
        StringBuffer html = new StringBuffer();
        html.append("<html><head>");
        html.append("</head>");
        
        html.append("<body>");
        html.append("<p>Hello World!</p>");
        html.append("</body>");
        
        html.append("</html>");
        return html.toString();
    }
}

Output is as follows (first line is original HTML, second line is returned from getText):

<html><head></head><body><p>Hello World!</p></body></html>
<html><head></head><body></body></html>

Phillipus · 2024-08-05T12:39:19Z

If BASE_URL is only used by the Edge class should that portion of code be moved there instead of being in the Browser class? And does it need to be public, perhaps it's an internal thing?

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

amartya4256 · 2024-08-06T11:38:32Z

If BASE_URL is only used by the Edge class should that portion of code be moved there instead of being in the Browser class? And does it need to be public, perhaps it's an internal thing?

The idea to put it in Browser was because this is needed outside swt as well and having it in Edge would lead to problems for other platforms. Moreover, this approach in Edge is also a high level implementation which means that this could be used for other browsers as well, since this problem can occur with any modern browser. I am not sure if a similar issue exists with browsers in other platforms but if so, in future we can do something similar to them as well.

But for now, we cant use Edge.BASE_URL outside SWT, since it is windows specific. We need the BASE_URL in 2 places: jdt.ui and eclipse.platform, these PRs would be follow up after this PR, as mentioned in the description. However I can make the static block to only create a file if Edge is the default, if necessary.

amartya4256 · 2024-08-06T11:41:43Z

This PR creates a temporary file and replaces the original implementation of Edge#setText() in all cases, even where Edge#setText() might not need it in the case of very simple HTML (no references to local resources). Would an additional setText method work rather than replacing the existing one?

And Edge#getText() doesn't return the correct contents. Run the following snippet:

import org.eclipse.swt.SWT;
import org.eclipse.swt.browser.Browser;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;

public class SWTBrowserTest {
    
    public static void main(String[] args) {
        final Display display = new Display();

        final Shell shell = new Shell(display);
        shell.setText("SWT Browser");
        shell.setLayout(new FillLayout());
        shell.setSize(1024, 768);
        
        String html = makeHTMLEntry();
        System.out.println(html);
        
        Browser browser = new Browser(shell, SWT.EDGE);
        browser.setText(html);
        
        System.out.println(browser.getText());

        shell.open();
        
        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) {
                display.sleep();
            }
        }

        display.dispose();
    }
    
    private static String makeHTMLEntry() {
        StringBuffer html = new StringBuffer();
        html.append("<html><head>");
        html.append("</head>");
        
        html.append("<body>");
        html.append("<p>Hello World!</p>");
        html.append("</body>");
        
        html.append("</html>");
        return html.toString();
    }
}

Output is as follows (first line is original HTML, second line is returned from getText):

<html><head></head><body><p>Hello World!</p></body></html>
<html><head></head><body></body></html>

This issue was because of the asynchronous behaviour. I have fixed it now.

Phillipus · 2024-08-06T13:32:46Z

However I can make the static block to only create a file if Edge is the default, if necessary.

Perhaps create it lazily when setText is called?

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

.

fedejeanne

Looks far better now but I know this PR is going to be changed so please @Phillipus / @HeikoKlare review too.

I'm out of office the next 2 weeks.

amartya4256 · 2024-08-08T12:09:48Z

Split the PR into 2 and moved the set text fix to #1395

fedejeanne · 2024-08-08T12:12:22Z

Split the PR into 2 and moved the set text fix to #1395

You still have 2 commits in this PR. This one is probably the one that you want to move to #1395 : 9635dff

amartya4256 · 2024-08-08T13:25:06Z

Split the PR into 2 and moved the set text fix to #1395

You still have 2 commits in this PR. This one is probably the one that you want to move to #1395 : 9635dff

This PR is based on #1395. Once 1395 is merged, it will not be visible. The other commit is from #1395. As mentioned in the description, only the last commit is relevant for this PR.

My 2 findings need to be addressed in a separate PR

HeikoKlare

The code looks much better structured now. I still see some easy opportunities for improving the WebViewProvider from a rather data-wrapping class to a better encapsulated provider class via moving more of the initialization logic inside that class and reducing the methods to be called from the consumer.

I also have one remark regarding a potential regressions, but most of the comments concern design and documentation improvements.

HeikoKlare · 2024-09-18T12:51:53Z

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

+	private CompletableFuture<ICoreWebView2_11> webView_11Future = new CompletableFuture<>();
+
+	private CompletableFuture<Void> lastWebViewTask = webViewFuture.thenCompose(__ -> {
+		return CompletableFuture.allOf(webView_2Future.handle((result, ex) -> null), webView_11Future.handle((result, ex) -> null));


I do not understand the purpose of this line. Maybe you can wrap the initialization of lastWebViewTask into a method that better explains (by its name or, if not possible, by documentation) what this is supposed to do?

I'll extract it to a method. This one bascially creates a future which will complete after webview is completed and after that webview_2 and webview_11 are either completed or cancelled (depending on the execution in setup task), just to make sure once all the webviews are ready before we start executing the other scheduled tasks

What should I name it? getWaitForWebViewInitializationFuture?

I do not understand the purpose of the calls to the method handle.

Anyway, why not simply complete the webViewFuture after the other web view interface have been initialized, i.e., in the initializeWebView method, replace this:

webViewFuture.complete(new ICoreWebView2(ppv[0])); initializeWebView_2(); initializeWebView_11();

with something like this:

ICoreWebView2 webView = new ICoreWebView2(ppv[0]); initializeWebView_2(webView); initializeWebView_11(webView); webViewFuture.complete(webView);

handle basically chains the 2 futures telling that we dont care if they are cancelled or completed but it the lastWebViewTask would only be completed after both the webView_2 and webView_11 are done after webView itself. But I like the approach. I'll create a Completablefuture and complete it following the initialization.

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

....swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java

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

HeikoKlare · 2024-09-20T10:51:48Z

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

@@ -312,12 +407,13 @@ static ICoreWebView2CookieManager getCookieManager() {
 		SWT.error(SWT.ERROR_NOT_IMPLEMENTED, null, " [WebView2: cookie access requires a Browser instance]");
 	}
 	Edge instance = environmentWrapper.instances().get(0);
-	if (instance.webView_2 == null) {
+	instance.webViewProvider.waitForPendingWebviewTasksToFinish();


Now all except this calls to waitForPendingWebiewTaskToFinish() are encapsulated within the WebViewProvider. I propose to keep that functionality completely hidden in the WebViewProvider. I guess calls to isWebView*Unavailable() should probably block until initialization has finished, as otherwise the method will return unreasonable resonable in case the web view has not been initialized yet anyway.

cant put waitForPendingWebiewTaskToFinish inside isWebView*Unavailable cause its used in callbacks and calling it inside that would cause a deadlock

But actually that's not even what you want to do here, is it? You want to wait for webView being initialized, not for all tasks being finished. That is something that should be done in the isWebView*Unavailable() methods anyway in order to avoid that they return unreasonable results when called while the web view has not been initialized yet.

In any case, hiding the need to wait for pending tasks in all but one case is a design flaw for which we need to have a solution.

I moved it inside the method and tested it. It seems fine and it isnt causing any deadlock. Seems good.

....swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java

HeikoKlare · 2024-09-20T10:56:58Z

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

+	private CompletableFuture<ICoreWebView2_11> webView_11Future = new CompletableFuture<>();
+
+	private CompletableFuture<Void> lastWebViewTask = webViewFuture.thenCompose(__ -> {
+		return CompletableFuture.allOf(webView_2Future.handle((result, ex) -> null), webView_11Future.handle((result, ex) -> null));


I do not understand the purpose of the calls to the method handle.

Anyway, why not simply complete the webViewFuture after the other web view interface have been initialized, i.e., in the initializeWebView method, replace this:

webViewFuture.complete(new ICoreWebView2(ppv[0])); initializeWebView_2(); initializeWebView_11();

with something like this:

ICoreWebView2 webView = new ICoreWebView2(ppv[0]); initializeWebView_2(webView); initializeWebView_11(webView); webViewFuture.complete(webView);

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

This contribution fixes Edge browser Deadlock issue on instantiating a new Edge Browser object during a webview callback using async execution. The Implementation follows the pattern similar to how WebKit has been implemented imitating the async behaviour in a sync fashion using Futures and callbacks. contributes to eclipse-platform#669

HeikoKlare

Looks like a rather clean solution now. Locally executing the Browser tests for Edge is successful. In some manual testing, I also did not find any issues so far.
Let's give this a try.

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

Fixup for #1378.

amartya4256 requested a review from niraj-modi as a code owner August 2, 2024 11:54

amartya4256 force-pushed the fix_edge branch from 86cfd33 to fdebf52 Compare August 2, 2024 12:39

amartya4256 force-pushed the fix_edge branch 2 times, most recently from 7b7ea3a to 95d0757 Compare August 5, 2024 12:18

Phillipus reviewed Aug 5, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java Outdated Show resolved Hide resolved

Phillipus reviewed Aug 5, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java Show resolved Hide resolved

amartya4256 force-pushed the fix_edge branch from 95d0757 to d5fe3dd Compare August 6, 2024 11:27

amartya4256 force-pushed the fix_edge branch 2 times, most recently from 50bd0bc to b3a9b1f Compare August 6, 2024 15:29

fedejeanne previously requested changes Aug 7, 2024

View reviewed changes

amartya4256 force-pushed the fix_edge branch 2 times, most recently from c0ddfb8 to 47c0484 Compare August 7, 2024 10:37

amartya4256 force-pushed the fix_edge branch 2 times, most recently from 5a2fdf4 to 8897897 Compare August 8, 2024 09:58

fedejeanne reviewed Aug 8, 2024

View reviewed changes

amartya4256 force-pushed the fix_edge branch from 8897897 to b03f52a Compare August 8, 2024 11:47

Phillipus mentioned this pull request Aug 8, 2024

Handle local resource references in Edge#setText() #1395

Merged

amartya4256 changed the title ~~Fix Edge Browser for Win32~~ Fix Edge Browser Deadlock issue for instantiation during a WebView Callback Aug 8, 2024

amartya4256 force-pushed the fix_edge branch from b03f52a to 3e1d92f Compare August 8, 2024 13:57

amartya4256 force-pushed the fix_edge branch 7 times, most recently from 78f900a to 73be7aa Compare September 16, 2024 14:52

HeikoKlare reviewed Sep 18, 2024

View reviewed changes

amartya4256 force-pushed the fix_edge branch 2 times, most recently from 0f4dd4e to 91431d7 Compare September 19, 2024 13:13

HeikoKlare reviewed Sep 20, 2024

View reviewed changes

amartya4256 force-pushed the fix_edge branch from 91431d7 to 2234dfa Compare September 20, 2024 15:45

HeikoKlare reviewed Sep 22, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java Outdated Show resolved Hide resolved

HeikoKlare reviewed Sep 22, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java Outdated Show resolved Hide resolved

amartya4256 force-pushed the fix_edge branch 2 times, most recently from 006a04d to 696e45c Compare September 23, 2024 12:46

HeikoKlare reviewed Sep 24, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java Show resolved Hide resolved

amartya4256 force-pushed the fix_edge branch 2 times, most recently from 43f0a80 to d9f776c Compare September 25, 2024 10:24

amartya4256 force-pushed the fix_edge branch from d9f776c to 1f22a98 Compare September 25, 2024 11:23

HeikoKlare approved these changes Sep 25, 2024

View reviewed changes

HeikoKlare merged commit 1d12c5f into eclipse-platform:master Sep 25, 2024
14 checks passed

sratz reviewed Oct 2, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java Show resolved Hide resolved

sratz added a commit that referenced this pull request Oct 2, 2024

Edge: Fix ICoreWebView2_11 availability check

9c4cb26

Fixup for #1378.

sratz mentioned this pull request Oct 2, 2024

Edge: Fix ICoreWebView2_11 availability check #1501

Merged

sratz added a commit that referenced this pull request Oct 2, 2024

Edge: Fix ICoreWebView2_11 availability check

e473f1a

Fixup for #1378.

amartya4256 mentioned this pull request Oct 7, 2024

Edge Browser freezes indefinitely when launching one Browser from another via a BrowserFunction #669

Closed

4 tasks

sratz added this to the 4.34 M1 milestone Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Edge Browser Deadlock issue for instantiation during a WebView Callback #1378

Fix Edge Browser Deadlock issue for instantiation during a WebView Callback #1378

amartya4256 commented Aug 2, 2024 •

edited by fedejeanne

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

Phillipus commented Aug 2, 2024 •

edited

Loading

Phillipus commented Aug 2, 2024 •

edited

Loading

Phillipus commented Aug 5, 2024

amartya4256 commented Aug 6, 2024 •

edited

Loading

amartya4256 commented Aug 6, 2024

Phillipus commented Aug 6, 2024

fedejeanne left a comment

amartya4256 commented Aug 8, 2024

fedejeanne commented Aug 8, 2024

amartya4256 commented Aug 8, 2024

HeikoKlare left a comment

HeikoKlare Sep 18, 2024

amartya4256 Sep 19, 2024

HeikoKlare Sep 20, 2024

amartya4256 Sep 20, 2024

HeikoKlare Sep 20, 2024

amartya4256 Sep 20, 2024

HeikoKlare Sep 22, 2024

amartya4256 Sep 23, 2024

HeikoKlare Sep 20, 2024

HeikoKlare left a comment

Fix Edge Browser Deadlock issue for instantiation during a WebView Callback #1378

Fix Edge Browser Deadlock issue for instantiation during a WebView Callback #1378

Conversation

amartya4256 commented Aug 2, 2024 • edited by fedejeanne Loading

Scope not covered

Pre-requisites

Testing

github-actions bot commented Aug 2, 2024 • edited Loading

Test Results

Phillipus commented Aug 2, 2024 • edited Loading

Phillipus commented Aug 2, 2024 • edited Loading

Phillipus commented Aug 5, 2024

amartya4256 commented Aug 6, 2024 • edited Loading

amartya4256 commented Aug 6, 2024

Phillipus commented Aug 6, 2024

fedejeanne left a comment

Choose a reason for hiding this comment

amartya4256 commented Aug 8, 2024

fedejeanne commented Aug 8, 2024

amartya4256 commented Aug 8, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

amartya4256 commented Aug 2, 2024 •

edited by fedejeanne

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

Phillipus commented Aug 2, 2024 •

edited

Loading

Phillipus commented Aug 2, 2024 •

edited

Loading

amartya4256 commented Aug 6, 2024 •

edited

Loading