Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

./wpt run webkitgtk_minibrowser is broken on Ubuntu 24.04 #49262

Open
mbrodesser-Igalia opened this issue Nov 19, 2024 · 10 comments
Open

./wpt run webkitgtk_minibrowser is broken on Ubuntu 24.04 #49262

mbrodesser-Igalia opened this issue Nov 19, 2024 · 10 comments
Labels
infra priority:backlog wptrunner The automated test runner, commonly called through ./wpt run

Comments

@mbrodesser-Igalia
Copy link
Contributor

mbrodesser-Igalia commented Nov 19, 2024

STR: ./wpt run --debug-test webkitgtk_minibrowser content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html

Log:

./wpt run --debug-test webkitgtk_minibrowser content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html 
Running 1 tests in web-platform-tests

  ▶ ERROR [expected OK] /content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html
  │   → TypeError: undefined is not an object (evaluating 'cookies[i].split('=')[1].trim')
  │ 
  │ @http://web-platform.test:8000/content-security-policy/support/checkReport.sub.js:33:49
  └ global code@http://web-platform.test:8000/content-security-policy/support/checkReport.sub.js:138:3

Ran 1 tests finished in 14.4 seconds.
  • 0 ran as expected. 0 tests skipped.
  • 1 tests had errors unexpectedly

Works with "chrome" instead of "webkitgtk_minibrowser". Works neither with Firefox, see #49261.

@mbrodesser-Igalia
Copy link
Contributor Author

This is not a multi-window issue, since e.g.

./wpt run --debug-test webkitgtk_minibrowser content-security-policy/navigation/to-javascript-url-script-src.html 
Running 1 tests in web-platform-tests

  [1/1] No tests running.

doesn't run the test.

@mbrodesser-Igalia
Copy link
Contributor Author

$ MiniBrowser --version
WebKitGTK 2.46.3

@mbrodesser-Igalia
Copy link
Contributor Author

@clopez: https://bugs.webkit.org/show_bug.cgi?id=281747 might be about the same issue

@clopez
Copy link
Contributor

clopez commented Nov 26, 2024

@clopez: https://bugs.webkit.org/show_bug.cgi?id=281747 might be about the same issue

I took a deeper look at the issue you are experimenting and is unrelated with that bug.
On top of that WebKitGTK 2.46.3 should not be affected by the bug above, because 2.46.0 was forked on webkit commit 282416@main but the regression from that bug was introduced much later on 285310@main

I also tested to run this test with wpewebkit_minibrowser runner that is not affected by such bug and it gives the same result.

$ ./wpt run --debug-test --headless wpewebkit_minibrowser content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html
Running 1 tests in web-platform-tests

  ▶ ERROR [expected OK] /content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html
  │   → TypeError: undefined is not an object (evaluating 'cookies[i].split('=')[1].trim')
  │ 
  │ @http://web-platform.test:8000/content-security-policy/support/checkReport.sub.js:33:49
  └ global code@http://web-platform.test:8000/content-security-policy/support/checkReport.sub.js:138:3

Ran 1 tests finished in 4.2 seconds.
  • 0 ran as expected. 0 tests skipped.
  • 1 tests had errors unexpectedly

So this test is not passing neither in webkitgtk neither in wpewebkit.

The results on the CI show webkitgtk nightly failing this test as well

However, other tests should pass... did you tried to run other tests with webkitgtk_minibrowser runner? Do other tests work for you?

So I think the title of this issue is not correct, the runner itself and the browser should work.
What happens here is that this very specific test is not working with webkit-linux (webkitgtk or wpewebkit) for some reason that I still don't known, a deeper look into the webkit code would be required here to understand why this test doesn't passes

@mbrodesser-Igalia
Copy link
Contributor Author

@clopez: https://bugs.webkit.org/show_bug.cgi?id=281747 might be about the same issue

I took a deeper look at the issue you are experimenting and is unrelated with that bug. On top of that WebKitGTK 2.46.3 should not be affected by the bug above, because 2.46.0 was forked on webkit commit 282416@main but the regression from that bug was introduced much later on 285310@main

I also tested to run this test with wpewebkit_minibrowser runner that is not affected by such bug and it gives the same result.

$ ./wpt run --debug-test --headless wpewebkit_minibrowser content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html
Running 1 tests in web-platform-tests

  ▶ ERROR [expected OK] /content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html
  │   → TypeError: undefined is not an object (evaluating 'cookies[i].split('=')[1].trim')
  │ 
  │ @http://web-platform.test:8000/content-security-policy/support/checkReport.sub.js:33:49
  └ global code@http://web-platform.test:8000/content-security-policy/support/checkReport.sub.js:138:3

Ran 1 tests finished in 4.2 seconds.
  • 0 ran as expected. 0 tests skipped.
  • 1 tests had errors unexpectedly

So this test is not passing neither in webkitgtk neither in wpewebkit.

The results on the CI show webkitgtk nightly failing this test as well

However, other tests should pass... did you tried to run other tests with webkitgtk_minibrowser runner? Do other tests work for you?

Some other tests pass, others fail, locally (with WebKitGTK 2.46.3) and on CI. See e.g. https://wpt.fyi/results/content-security-policy/navigation?label=master&label=experimental&product=chrome&product=edge&product=firefox&product=safari&product=webkitgtk&aligned&q=content-security-policy%2Fnavigation%2F.

The passing tests are the ones which don't open other windows, the failing ones open other windows.
E.g. https://github.com/web-platform-tests/wpt/blob/d6ea893fdd/content-security-policy/navigation/to-javascript-url-script-src.html passes and https://github.com/web-platform-tests/wpt/blob/d6ea893fdd/content-security-policy/navigation/to-javascript-parent-initiated-parent-csp.html#L35 fails.

The failing test with the least noise seems to be https://github.com/web-platform-tests/wpt/blob/d6ea893fdd/content-security-policy/navigation/javascript-url-navigation-evaluated-to-string-inherits-csp.html, as it passes on Safari, whereas some of the other tests' subtests fail on Safari.

If I remember correctly, at least some of the tests which open other windows passed locally, until a few weeks ago. After fetching new patches from the WPT repository, they started failing. So it might be that changes in the WPT repo, roughly during the last four weeks, caused this issue.

So I think the title of this issue is not correct, the runner itself and the browser should work. What happens here is that this very specific test is not working with webkit-linux (webkitgtk or wpewebkit) for some reason that I still don't known, a deeper look into the webkit code would be required here to understand why this test doesn't passes

@mbrodesser-Igalia
Copy link
Contributor Author

If I remember correctly, at least some of the tests which open other windows passed locally, until a few weeks ago. After fetching new patches from the WPT repository, they started failing. So it might be that changes in the WPT repo, roughly during the last four weeks, caused this issue.

With WPT commit https://github.com/web-platform-tests/wpt/tree/merge_pr_48770/content-security-policy/navigation, from October 24th, https://github.com/web-platform-tests/wpt/blob/merge_pr_48770/content-security-policy/navigation/javascript-url-navigation-inherits-csp.html fails locally with WebKitGTK 2.46.3.

With WPT commit https://github.com/web-platform-tests/wpt/tree/merge_pr_48778/content-security-policy/navigation from September 19th,
https://github.com/web-platform-tests/wpt/blob/merge_pr_48778/content-security-policy/navigation/javascript-url-navigation-inherits-csp.html passes locally with WebKitGTK 2.46.3.

That specific test didn't change meanwhile. So some change during that timeframe in the WPT repo must have broken the test.

Be aware the test was renamed afterward that timeframe.

@clopez
Copy link
Contributor

clopez commented Nov 27, 2024

@mbrodesser-Igalia thanks a lot for the pointers!

This allowed me to bisect this and I see the issue started to happen with wpt commit bef892c

That commit changed some things on the webdriver executor that webkitgtk uses to run the wpt tests, like:

  • Open the new window for the test via webdriver command /window/new instead of using injected javascript to open the new window
  • Open the new window as tab by default instead as of a standalone window

And that seems to have broken this test on webkitgtk.

I have this wip patch that fixes the test back on webkitgtk

diff --git a/tools/wptrunner/wptrunner/executors/executorwebdriver.py b/tools/wptrunner/wptrunner/executors/executorwebdriver.py
index 243290841f..6d0cbaa699 100644
--- a/tools/wptrunner/wptrunner/executors/executorwebdriver.py
+++ b/tools/wptrunner/wptrunner/executors/executorwebdriver.py
@@ -77,7 +77,9 @@ class WebDriverBaseProtocolPart(BaseProtocolPart):
     def set_timeout(self, timeout):
         self.webdriver.timeouts.script = timeout
 
-    def create_window(self, type="tab", **kwargs):
+    def create_window(self, type=None, **kwargs):
+        if type is None:
+            type = 'window' if 'webkitgtk:browserOptions' in self.parent.capabilities else 'tab'
         return self.webdriver.new_window(type_hint=type)
 
     @property

Basically it makes the test window to be open as a new standalone window instead of a tab for webkitgtk.

Related to that I reported this issue on webkitgtk: https://bugs.webkit.org/show_bug.cgi?id=283392 but I don't think is the same issue here. Because if I manually focus the new tab when opened then it doesn't make the test pass, but with other tests that I tried (like acid ones) just focusing the tab manually is enough to make the test pass.

So it looks like some internal status of the browser than when it opens a new window is ok for this test but when it opens a new tab it isn't

And with the wpewebkit_webdriver runner (just landed it) it fails this test always, no not sure what is going on here.

It will be really helpful if you can provide me a standalone html test case for this that I can load directly from the browser to check what is happening there (so I can use the webinspector, etc) (without having to use the wpt runner)

@mbrodesser-Igalia
Copy link
Contributor Author

Basically it makes the test window to be open as a new standalone window instead of a tab for webkitgtk.

IIRC, that's what webkitgtk's Minibrowser did during WPT executions before it broke. Consider submitting that fix, as it would allow more tests to pass. Presumably, from a 'headless'/non visual UI perspective, new tabs and new windows behave the same for many features.

Fixing webkitgtk's Minibrowser to correctly open tabs could be dealt with separately.

WDYT?

@mbrodesser-Igalia
Copy link
Contributor Author

It will be really helpful if you can provide me a standalone html test case for this that I can load directly from the browser to check what is happening there (so I can use the webinspector, etc) (without having to use the wpt runner)

I lack currently capacity to do that.

clopez added a commit to clopez/wpt that referenced this issue Dec 4, 2024
…ow in a tab

WPT commit bef892c changed some things on the webdriver executor, like:

1. Open the new window for the test via webdriver command "/window/new"
   instead of using injected javascript to open the new window.
2. Open the new window as tab by default instead as of a standalone window

And in the case of WebKitGTK-based browsers there is currently a bug
that when the new test window is opened in a tab instead in a standalone
window it causes many timeouts and failures on the WPT tests.

This has been reported to WebKitGTK here https://webkit.org/b/283392

So this commits defaults back the new test window to be an standalone window
instead of a tab for the WebkitGTK-based browsers meanwhile the issue
is not fixed there.

Related issue: web-platform-tests#49262
clopez added a commit to clopez/wpt that referenced this issue Dec 5, 2024
…ow in a tab

WPT commit bef892c changed some things on the webdriver executor, like:

1. Open the new window for the test via webdriver command "/window/new"
   instead of using injected javascript to open the new window.
2. Open the new window as tab by default instead as of a separate window

And in the case of WebKitGTK-based browsers there is currently a bug
that when the new test window is opened in a tab instead in a standalone
window it causes many timeouts and failures on the WPT tests.

This has been reported to WebKitGTK here https://webkit.org/b/283392

So this commits defaults back the new test window to be an standalone window
instead of a tab for the WebkitGTK-based browsers meanwhile the issue
is not fixed there.

Related issue: web-platform-tests#49262
jgraham pushed a commit that referenced this issue Dec 9, 2024
…ow in a tab

WPT commit bef892c changed some things on the webdriver executor, like:

1. Open the new window for the test via webdriver command "/window/new"
   instead of using injected javascript to open the new window.
2. Open the new window as tab by default instead as of a separate window

And in the case of WebKitGTK-based browsers there is currently a bug
that when the new test window is opened in a tab instead in a standalone
window it causes many timeouts and failures on the WPT tests.

This has been reported to WebKitGTK here https://webkit.org/b/283392

So this commits defaults back the new test window to be an standalone window
instead of a tab for the WebkitGTK-based browsers meanwhile the issue
is not fixed there.

Related issue: #49262
@clopez
Copy link
Contributor

clopez commented Dec 11, 2024

Basically it makes the test window to be open as a new standalone window instead of a tab for webkitgtk.

IIRC, that's what webkitgtk's Minibrowser did during WPT executions before it broke. Consider submitting that fix, as it would allow more tests to pass. Presumably, from a 'headless'/non visual UI perspective, new tabs and new windows behave the same for many features.

I submitted the workaround and it landed in a4e5090

See the diff between this two webkitgtk runs (the last run before the change and the first run after it):
https://wpt.fyi/results/?diff&filter=ADC&run_id=6236409772113920&run_id=5085873362436096
There are quite lot more tests passing now.

@gsnedders gsnedders added infra wptrunner The automated test runner, commonly called through ./wpt run labels Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra priority:backlog wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

No branches or pull requests

4 participants