eclipse.platform.swt Fix Edge Browser Deadlock issue for instantiation during a WebView Callback

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.

To test the implementation, you need to set the default browser to Edge. To do it in the source code, change org.eclipse.swt.browser.BrowserFactory.createWebBrowser with the following source code: return new Edge();

Scope not covered:

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

Based on #1395 Hence, only the last commit is relevant.

contributes to #669

Aug 02 '24 11:08 amartya4256

Test Results

486 files ±0 486 suites ±0 6m 55s :stopwatch: - 1m 22s 4 159 tests +1 4 151 :white_check_mark: +1 8 :zzz: ±0 0 :x: ±0 16 390 runs +4 16 298 :white_check_mark: +4 92 :zzz: ±0 0 :x: ±0

Results for commit 1f22a98f. ± Comparison against base commit 3c8ed8e7.

:recycle: This comment has been updated with latest results.

Aug 02 '24 12:08 github-actions[bot]

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?

Aug 02 '24 15:08 Phillipus

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>

Aug 02 '24 17:08 Phillipus

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?

Aug 05 '24 12:08 Phillipus

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.

Aug 06 '24 11:08 amartya4256

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.

Aug 06 '24 11:08 amartya4256

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?

Aug 06 '24 13:08 Phillipus

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

Aug 08 '24 12:08 amartya4256

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 : https://github.com/eclipse-platform/eclipse.platform.swt/pull/1378/commits/9635dffbac1f32f8c8caf15c429d7d5df518b5cb

Aug 08 '24 12:08 fedejeanne

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.

Aug 08 '24 13:08 amartya4256

@amartya4256 I tested the current status of the PR and I found a bug: when using Edge, I sometimes don't see the page loading.

I tested with the "Welcome" page and I switched to other tabs (Tutorials, Samples, etc).

First, in light mode you see IE (working fine) and later, in dark mode, you see Edge (broken).

BTW in both cases I start the application in "dark" mode but IE didn't honor that and the websites had a white background. Edge has a dark background. I assume this is by design?

@fedejeanne I know this behavior. It can happen because of the caching effect. Eclipse caches and local URLs and since we are using a file url for displaying custom url, it just navigates to the file but doesnt follow the normal process flow which is using the setText method where after the page is loaded some js script is run to display the content but it rather uses setUrl and just navigates there and its just an empty page.

It happens so because eclipse caches everything except for about:blank and now since our file url acts as about:blank, we need to add a check to not cache this page (URL_FOR_CUSTOM_TEXT). And a PR for this is already available here: https://github.com/eclipse-platform/eclipse.platform/pull/1521/files

I would add all the PRs in the description that you should fetch which are needed for testing this PR.

However, i found the exact root of this issue and it has been resolved now.

Sep 04 '24 09:09 amartya4256

Code looks ok, just some minor comments about indentation and the test.

I tested and found 2 small issues:

There are no navigation keys in the JavaDoc when I press on a link "too early". Workaround: either leave it time to load the navigation keys before clicking on the link or hover the mouse on the bottom area (the last one seems to work only sometimes).

Since Edge honors the theme, now the "scope" dialog looks odd in the dark theme. You can address this one on a separate issue.

I looked into the first issue. Apparently, the feature to bring the buttons on hover is not implemented for Edge. I looked into if we can capture the hover event. Couldn't find anything. Another workaround could be to trigger this on click inside the webview. That seems to working. The reason why the buttons shows up by hovering in the bottom right or clicking is because the event is triggered by the styledText and not the browser. However, I'll look into if I can find something in WebView2 to capture hover events and trigger this there.

Also regarding the second issue, it seems to be the custom styling set for the element and thats why it doesnt adapt the color:

And yes its unrelated to this PR and must be addressed in a separate PR.

Sep 11 '24 15:09 amartya4256

Code looks ok, just some minor comments about indentation and the test. I tested and found 2 small issues:

There are no navigation keys in the JavaDoc when I press on a link "too early". Workaround: either leave it time to load the navigation keys before clicking on the link or hover the mouse on the bottom area (the last one seems to work only sometimes).

Since Edge honors the theme, now the "scope" dialog looks odd in the dark theme. You can address this one on a separate issue.

I looked into the first issue. Apparently, the feature to bring the buttons on hover is not implemented for Edge. I looked into if we can capture the hover event. Couldn't find anything. Another workaround could be to trigger this on click inside the webview. That seems to working. The reason why the buttons shows up by hovering in the bottom right or clicking is because the event is triggered by the styledText and not the browser. However, I'll look into if I can find something in WebView2 to capture hover events and trigger this there.

Is this even related to this PR? I see that behavior also with the current state of the Edge implementation, so no regression produced by this PR and nothing in the scope or this PR. Can't we create another issue and address that independently?

Sep 11 '24 15:09 HeikoKlare

Code looks ok, just some minor comments about indentation and the test. I tested and found 2 small issues:

There are no navigation keys in the JavaDoc when I press on a link "too early". Workaround: either leave it time to load the navigation keys before clicking on the link or hover the mouse on the bottom area (the last one seems to work only sometimes).

Since Edge honors the theme, now the "scope" dialog looks odd in the dark theme. You can address this one on a separate issue.

I looked into the first issue. Apparently, the feature to bring the buttons on hover is not implemented for Edge. I looked into if we can capture the hover event. Couldn't find anything. Another workaround could be to trigger this on click inside the webview. That seems to working. The reason why the buttons shows up by hovering in the bottom right or clicking is because the event is triggered by the styledText and not the browser. However, I'll look into if I can find something in WebView2 to capture hover events and trigger this there.

Is this even related to this PR? I see that behavior also with the current state of the Edge implementation, so no regression produced by this PR and nothing in the scope or this PR. Can't we create another issue and address that independently?

Yes, it must be addressed in a separate PR

Sep 11 '24 15:09 amartya4256

eclipse.platform.swt eclipse.platform.swt copied to clipboard

Fix Edge Browser Deadlock issue for instantiation during a WebView Callback

Test Results

eclipse.platform.swt
eclipse.platform.swt copied to clipboard