Skip to content

Conversation

sratz
Copy link
Member

@sratz sratz commented Apr 15, 2025

According to https://webkitgtk.org/reference/webkit2gtk/stable/signal.WebView.create.html

The new WebKitWebView must be related to web_view, see WebKitWebView:related-view for more details.

To ensure this, org.eclipse.swt.browser.WebKit.create(Composite, int)
does the following:

        Composite parentShell = parent.getParent();
        Browser parentBrowser = WebKit.parentBrowser;
        if (parentBrowser == null && parentShell != null) {
                Control[] children = parentShell.getChildren();
                for (int i = 0; i < children.length; i++) {
                        if (children[i] instanceof Browser) {
                                parentBrowser = (Browser) children[i];
                                break;
                        }
                }
        }

        if (parentBrowser == null) {
                webView = WebKitGTK.webkit_web_view_new();
        } else {
                webView = WebKitGTK.webkit_web_view_new_with_related_view(((WebKit)parentBrowser.webBrowser).webView);
        }
  1. A static variable set to true inside NewWindowListeners:
        /**
         * Stores the browser which is opening a new browser window,
         * during a WebKit {@code create} signal. This browser
         * must be passed to a newly created browser as "related".
         *
         * See bug 579257.
         */
        private static Browser parentBrowser;
  1. A heuristic for when two browser instances share the parent

In the failing tests, we currently have neither.

To fix that, use the more 'correct' solution 1) by moving the instantiation of new Browser inside the OpenWindowListener.

Resolves #2014.
Resolves #1564.

@sratz sratz requested a review from HeikoKlare April 15, 2025 10:41
Copy link
Contributor

github-actions bot commented Apr 15, 2025

Test Results

   545 files  + 6     545 suites  +6   28m 20s ⏱️ - 3m 28s
 4 373 tests +37   4 361 ✅ +35   12 💤 +3  0 ❌  - 1 
16 634 runs  +37  16 530 ✅ +38  104 💤 ±0  0 ❌  - 1 

Results for commit 0f43a3a. ± Comparison against base commit 70fab96.

♻️ This comment has been updated with latest results.

@akurtakov
Copy link
Member

This seems to break Windows browser tests and will definitely leave some cases that used to work to actually crash swt. On the other side I agree that these cases are probably not so nicely written code.

@sratz
Copy link
Member Author

sratz commented Apr 15, 2025

This seems to break Windows browser tests

Hm, yeah, for some reason there are two completed events. I'll update the tests to be not so strict.

will definitely leave some cases that used to work to actually crash swt

I think the tests were green, but the feature actually did not work, i.e. in the old WebKit there was a return path:
https://github.com/WebKit/WebKit/pull/36316/files#diff-380d659fc1da401ce48dd8a799eafc41c6b8f4ae36e44412035ba64b913d64f9L2674
and the new window never really showed up.

So I don't think any productive feature is actually broken.

But yeah, the fact that WebKit is more strict now and crashes if you use the OpenWindowListener incorrectly, is not very nice.

@akurtakov
Copy link
Member

I think the tests were green, but the feature actually did not work, i.e. in the old WebKit there was a return path: https://github.com/WebKit/WebKit/pull/36316/files#diff-380d659fc1da401ce48dd8a799eafc41c6b8f4ae36e44412035ba64b913d64f9L2674 and the new window never really showed up.

So I don't think any productive feature is actually broken.

I don't question that. What I am afraid of such old (and non-working) code now hard crashing the whole shell instead of "silently" not working (like it was). But I guess such plugins/projects will have to fix their own code as I still can't think how to prevent that in SWT.

sratz added 2 commits April 15, 2025 13:13
According to
https://webkitgtk.org/reference/webkit2gtk/stable/signal.WebView.create.html

'The new WebKitWebView must be related to web_view, see
WebKitWebView:related-view for more details.'

To ensure this, org.eclipse.swt.browser.WebKit.create(Composite, int)
does the following:

	Composite parentShell = parent.getParent();
	Browser parentBrowser = WebKit.parentBrowser;
	if (parentBrowser == null && parentShell != null) {
		Control[] children = parentShell.getChildren();
		for (int i = 0; i < children.length; i++) {
			if (children[i] instanceof Browser) {
				parentBrowser = (Browser) children[i];
				break;
			}
		}
	}

	if (parentBrowser == null) {
		webView = WebKitGTK.webkit_web_view_new();
	} else {
		webView = WebKitGTK.webkit_web_view_new_with_related_view(((WebKit)parentBrowser.webBrowser).webView);
	}

1) A static variable set to true inside NewWindowListeners:

	/**
	 * Stores the browser which is opening a new browser window,
	 * during a WebKit {@code create} signal. This browser
	 * must be passed to a newly created browser as "related".
	 *
	 * See bug 579257.
	 */
	private static Browser parentBrowser;

2) A heuristic for when two browser instances share the parent

In the failing tests, we currently have neither.

To fix that, use the more 'correct' solution 1) by moving the
instantiation of new Browser inside the OpenWindowListener.

Resolves eclipse-platform#2014.
@sratz sratz force-pushed the browsertest-newwindow-linux branch from 8a9c582 to 0f43a3a Compare April 15, 2025 12:13
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. I agree with your conclusions regarding the potential risks you have already discussed.

This will resolve #1564 as well, right?

@sratz
Copy link
Member Author

sratz commented Apr 15, 2025

This will resolve #1564 as well, right?

Yes. I added it to the PR message 👍

@sratz sratz merged commit cc629f8 into eclipse-platform:master Apr 15, 2025
17 checks passed
@sratz sratz deleted the browsertest-newwindow-linux branch April 15, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants