Marionette executor should use "New Window" command to open the test tab
Categories
(Testing :: web-platform-tests, enhancement, P2)
Tracking
(firefox132 fixed)
| Tracking | Status | |
|---|---|---|
| firefox132 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Regressed 2 open bugs)
Details
(Whiteboard: [fxdroid][geckoview:p3][webdriver:m12], [wptsync upstream])
Attachments
(5 files, 7 obsolete files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
Bug 1522790 - [wpt] Remove metadata for all the intermittently timing out tests for Android Fission.
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
Now that we have the support for the "New Window" command in Marionette, we might want to make use of it.
|
||
Comment 1•6 years ago
|
||
Wes, can you add this to your todo list and coordinate with whimboo if you have any questions.
|
Assignee | |
Comment 2•6 years ago
|
||
Note that bug 1523234 isn't needed here because wptrunner expects to have the new tab focused.
Would that be the various open() calls in https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py#636 ?
|
|
Assignee | |
Comment 4•6 years ago
|
||
All the window.open() calls, which are not only in the py files, but also in various JS modules like:
|
||
Comment 5•6 years ago
|
||
It's not all, since it can only be used with resources that depend on executormarionette.
I put together a quick patch at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2ee65df66a329d7d5acda114a30ddf96a623d7
There are a couple of problems:
- The new tab opened in this way appears to show the default nee tab page and not about:blank as required by the spec. This is probably slowing tests down a little.
- This depends on an unreleased marionette_driver. If the plan is to not make more releases then we'll need to either avoid using the client methods here or put a copy of the client in [testing/web-platform/tests]tools/third_party for wptrunner to use.
Re 1, I'd suggest setting the new tab and homepage prefs to be about:blank in one of the unittest profiles that wpt depends on.
|
|
Assignee | |
Comment 7•6 years ago
|
||
Re 2, I will get a new marionette driver package released via bug 1530794.
Here's a try push of James's patch plus a change for the homepage: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&searchStr=wpt&revision=d47b22c1be92a3a3728c9dd6c613d5ae86e2079d
|
|
Assignee | |
Comment 9•6 years ago
|
||
That last try push looks like a complete fail someone needs to be investigate. I'm happy to fix a possible underlying issue with Marionette.
|
|
Assignee | |
Comment 10•6 years ago
|
||
I just released marionette_driver 2.8.0 to PyPI, which includes support for the new window command. So this unblocks the implementation here.
Depends on D23972
Here's jgraham's patch and a patch to set the homepage to about:blank in the wpt testing profile, rebased to current central.
And a try push to see what that looks like: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=3940a503f1c911c6fb5c8c212e18d8d5fada5a7d
Do I need to do anything to pick up the new marionette_driver now that bug 1530794 is completed? Are there any parts of jgraham's patch that can be dropped now?
|
|
Assignee | |
Comment 14•6 years ago
|
||
You have to make sure to at least bump the dependency for marionette_driver.
Here's a try push with the dependency bumped: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=234671713&revision=11d24ebc5935bc60a540d1dfee13189ed6ec896f
Looks like everything's still broken, much in the same way it was for jgraham's original try push from comment 5. It's not every single test failing, though. There's one chunk that passed completely, and there are several TEST-OKs in each of the failing chunks. I'm not sure why things are failing. Are they occasionally getting the wrong window or something?
|
|
||
Comment 16•6 years ago
|
||
Eh I think it was just a silly error; for some reason I thought Create Window switched to the new window, but it turns out it doesn't. So https://treeherder.mozilla.org/#/jobs?repo=try&revision=608fb12a70a9b1ce8451fdacd51fabd114d15818 is a try push with that fixed.
Looks better. Still seems like there are several failures, and I'm seeing some of the JavascriptException: TypeError: window.__wptrunner_process_next_event is not a functions that we've been seeing in bug 1522790.
Some seem to be known intermittents, some UNEXPECTED-PASSes. Maybe this fixed some of our tests? Maybe we're just on a bad base revision?
|
|
Assignee | |
Comment 18•6 years ago
|
||
(In reply to James Graham [:jgraham] from comment #16)
Eh I think it was just a silly error; for some reason I thought Create Window switched to the new window, but it turns out it doesn't.
Not by default. If you want that behavior you have to specify focus=True. I can see that this was already used in Wes' try push, so what exactly did you change?
|
|
||
Comment 19•6 years ago
|
||
Not by default. If you want that behavior you have to specify focus=True. I can see that this was already used in Wes' try push, so what exactly did you change?
I mean "switched the webdriver active browsing context to"
Still seems like there are several failures, and I'm seeing some of the JavascriptException: TypeError: window.__wptrunner_process_next_event is not a functions that we've been seeing in bug 1522790.
I'm pretty sure there's a race condition that's causing the __process_next_event thing; I'm investigating.
|
|
||
Comment 20•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&author=james%40hoppipolla.co.uk&selectedJob=235205397 is a try push that ports the manual logic to make the runner wait for pending loads over to marionette (we were already using this for Chrome which doesn't support the right pageLoadStragtegy). This looks like it improved things, although there are still quite a few changed results which could be due to focus changes or something; this needs some investigation. That same patch could be a workaround in bug 1501562 so I suggest trying it there and see if it helps. But in any case it's clearly working around a bug in marionette in each case, so it's not the correct long-term solution.
|
||
Comment 21•6 years ago
•
|
||
So on Geckoview, if I don't also have a time.sleep(2) prior to the navigate(), wait_for_load() ends up in an infinite loop. location.href is 'about:blank', and document.readyState is 'complete'.
If I do have the time.sleep(2) up there, wait_for_load() is called once. location.href is the wpt test url, and document.readyState is 'complete'.
|
|
||
Comment 22•6 years ago
|
||
FTR the try push mentioned in my previous comment is https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b229af02e908f1ff528260098c54ec22a6344d5
|
||
Updated•6 years ago
|
|
||
Updated•6 years ago
|
|
||
Updated•4 years ago
|
|
|
||
Updated•4 years ago
|
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 23•2 years ago
|
||
James, we should be good to go now with the changes for this bug on both desktop and Android. Are you still interested in updating the existing patch?
|
|
||
Updated•2 years ago
|
|
|
||
Comment 24•2 years ago
|
||
I rebased the patch, and updated it.
The problem now seems to be that all the focus-related tests are failing; when we open the window with window.open we end up with the document in the new window getting focus, whereas with the new window approach, it seems like the focus ends up in the chrome area, and so tests that expect to be able to call element.focus() and have it change the focus don't work.
I guess the spec doesn't make it clear what should happen here, but it seems reasonable for marionette at least that if we pass focus=True we end up with focus in the content area.
|
|
Assignee | |
Comment 25•2 years ago
|
||
James, this should be unblocked now. Please try again after rebasing to latest central.
|
|
Assignee | |
Updated•11 months ago
|
|
|
Assignee | |
Comment 27•11 months ago
|
||
Here the latest try push:
https://treeherder.mozilla.org/jobs?repo=try&revision=eefbf4c472d9f816b70d3398693df76ce05b6f24
James, if you need anything to test from me feel free to ask and as best push the updated patch to Phabricator.
|
|
||
Comment 28•11 months ago
|
||
|
|
||
Comment 29•11 months ago
|
||
I've pushed my updated patch to phab. There are a still some CI failures. Also I think the reftest change is broken, although we don't use that codepath in practice.
|
|
Assignee | |
Comment 30•11 months ago
|
||
I've made some updates to the meta files and additional fixes. All in a separate commit. I've pushed only those test suites to try which were failing before:
https://treeherder.mozilla.org/jobs?repo=try&revision=b10273fe599a01d5c93cb642057ebe23a2238b51
|
||
Updated•10 months ago
|
|
|
Assignee | |
Comment 31•10 months ago
|
||
As agreed after the investigation of the sanitizer deadlock failures (see bug 1887870) we will temporarily mark those failing tests as intermittent OK, TIMEOUT. Here a new complete try build for web-platform-tests:
https://treeherder.mozilla.org/jobs?repo=try&revision=bd04435c99c3a95c27b074380ae7413bd2481657
|
|
||
Updated•10 months ago
|
|
|
Assignee | |
Comment 32•10 months ago
|
||
Depends on D204234
|
|
Assignee | |
Comment 33•10 months ago
|
||
Depends on D206622
|
|
Assignee | |
Comment 34•10 months ago
|
||
Depends on D206623
|
|
||
Updated•10 months ago
|
|
|
Assignee | |
Comment 35•10 months ago
|
||
As discussed with James I'm taking this bug over because I investigated all remaining issues, updated the meta data and also fixed some tests. Based on the amount of time it took I would suggest that we set 5 points.
|
||
Comment 36•10 months ago
|
||
|
|
Assignee | |
Comment 38•10 months ago
|
||
|
|
||
Comment 39•10 months ago
|
||
|
||
Comment 40•10 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/9e98e314a251
https://hg.mozilla.org/mozilla-central/rev/3acba738b962
https://hg.mozilla.org/mozilla-central/rev/251e04906ebf
https://hg.mozilla.org/mozilla-central/rev/f8d941b403d0
https://hg.mozilla.org/mozilla-central/rev/e25a9624d6d9
https://hg.mozilla.org/mozilla-central/rev/433392f1c300
|
|
||
Comment 41•10 months ago
|
||
Backed out for having multiple regressions
Backout link: https://hg.mozilla.org/integration/autoland/rev/27749cc19f3ebf7e6c33b48705539456e0594e77
|
|
||
Comment 46•10 months ago
|
||
|
|
Assignee | |
Comment 47•10 months ago
|
||
Quick update what's left to do after the requested backout:
- We should wait until the detected TSAN issues, which cause a lot of intermittent failures for canvas jobs, have been all fixed.
- Bug 1890038 - "Perma wpt canvas timeout": Might be related to the above canvas issues and we should re-test once these bugs got fixed
- Bug 1890062 - "TestRunner hit external timeout": Looks like most or all of them are related to bug 1887870 (deadlock detector)
|
|
Assignee | |
Comment 49•10 months ago
|
||
(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #47)
- We should wait until the detected TSAN issues, which cause a lot of intermittent failures for canvas jobs, have been all fixed.
- Bug 1890038 - "Perma wpt canvas timeout": Might be related to the above canvas issues and we should re-test once these bugs got fixed
Both known TSAN issues for graphics (text) have been fixed. That means that canvas tests should work now. I've pushed a try build with all tests for TSAN: https://treeherder.mozilla.org/jobs?repo=try&revision=4d39ecc4ad0fc982708e36e13580c392f25ea2b2
Lets see / hope that all the other canvas timeouts (bug 1890038) are fixed as well.
|
|
||
Updated•10 months ago
|
|
|
||
Updated•10 months ago
|
|
|
||
Updated•10 months ago
|
|
|
Assignee | |
Comment 50•10 months ago
|
||
(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #49)
Both known TSAN issues for graphics (text) have been fixed. That means that canvas tests should work now. I've pushed a try build with all tests for TSAN: https://treeherder.mozilla.org/jobs?repo=try&revision=4d39ecc4ad0fc982708e36e13580c392f25ea2b2
Lets see / hope that all the other canvas timeouts (bug 1890038) are fixed as well.
I can confirm that all these issues are fixed now. The try jobs are all green!
We also got the fix for the mutex deadlock landed, and I'll push another try with updated meta data (all previously changed metadata reverted) once it got merged to mozilla-central somtime today.
|
|
Assignee | |
Comment 51•10 months ago
|
||
Here a new try build with all the TSAN issues fixed now:
https://treeherder.mozilla.org/jobs?repo=try&revision=21cc29fb9363ea52095440f5d9ec80427d339f19
|
|
Assignee | |
Comment 52•10 months ago
|
||
Now Android wpt jobs are failing with TestRunner timeouts. Sadly I missed to enable remote trace logs for the last try build, so that I had to push again to try for just Android:
https://treeherder.mozilla.org/jobs?repo=try&revision=88cad767424e03d0200a40d188993870d7c23bda
|
|
Assignee | |
Comment 53•10 months ago
•
|
||
(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #52)
Now Android wpt jobs are failing with TestRunner timeouts. Sadly I missed to enable remote trace logs for the last try build, so that I had to push again to try for just Android:
https://treeherder.mozilla.org/jobs?repo=try&revision=88cad767424e03d0200a40d188993870d7c23bda
The problem here seems actually be a delay in finish loading the new window on Android. This can be seen via logging as done to adb logcat and manifests in a browser-delayed-startup-finished notification that gets send out very late while other commands are already running. Current wptrunner is using window.open() and then awaits the load event for the new window's document if not yet finished loading. Right after a navigation is started for the test page. And here the WebProgress listener sees a hick-up and suddenly stops to work, resulting in the page never reaching the readyState=complete and the testharness to timeout.
With my patch to use Marionette's NewWindow command the problem is there as well but could be easily fixed by delaying the return of GeckoViewTabUtil.createNewTab() until the window has been finished initializing / loading. As of now this method only waits for the geckoview-window-created notification which actually is fired too early, and the navigation as triggered right after causes a collision with the navigation to the test page as triggered by wptrunner. Changing the notification to use browser-delayed-startup-finished as well will workaround the problem to let tests work correctly. Tests that could be used to demonstrate the behavior are under /referrer-policy/generic/.
My plan is to patch the GeckoViewTabUtil.createNewTab() function as mentioned above which would allow us to remove around 14.000 intermittent timeout entries across all wpt tests! Then I will file a bug to get the underlying issue investigated.
|
|
Assignee | |
Comment 54•10 months ago
|
||
With the above method applied the tests for Fission on Android look great. There is not a single timeout of the test harness anymore! Here a try build: https://treeherder.mozilla.org/jobs?repo=try&revision=0ed041267314f5db7a84f528d4ff1f9140543e0d
All the test failures that were still happening are due to the fact that the test finally runs now. I've updated the related meta data and pushed a new try build to verify it works all fine:
https://treeherder.mozilla.org/jobs?repo=try&revision=630b44e6d511312e68e50e7f108426aca0b10aaf
|
|
||
Updated•10 months ago
|
|
|
Assignee | |
Comment 55•10 months ago
|
||
Depends on D206624
|
|
Assignee | |
Comment 56•10 months ago
|
||
Depends on D207318
|
|
Assignee | |
Updated•10 months ago
|
|
|
Assignee | |
Comment 57•10 months ago
|
||
As discussed with the team we are going to bump the number of points based on the amount of extra work that was necessary so far to get working patches done. Hopefully no more work is needed for this bug.
|
||
Comment 58•10 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/d180feadd8b4
https://hg.mozilla.org/mozilla-central/rev/b5b4a44dd06b
|
|
Assignee | |
Comment 59•10 months ago
|
||
(In reply to Iulian Moraru from comment #58)
https://hg.mozilla.org/mozilla-central/rev/d180feadd8b4
https://hg.mozilla.org/mozilla-central/rev/b5b4a44dd06b
So what are these landings and why is the bug marked as fixed? This was - as it looks like - a no-op push?
The latest try build for the remaining Android fixes is green so I'm going to land all the revisions of the stack.
|
|
Assignee | |
Comment 60•10 months ago
|
||
This cannot be landed yet due to the latest wpt-sync causes merge conflicts in quite a lot of files now. I'll have to rebase the last two revisions.
|
|
Assignee | |
Comment 61•10 months ago
|
||
So I've rebased to latest mozilla-central, which includes the recent metadata updates from the downstream wpt-sync. Hopefully it's still all working. To verify before landing I pushed another try build:
https://treeherder.mozilla.org/jobs?repo=try&revision=f5d54cd034abe7508e03e825bfb42f472bb3c093
|
|
Assignee | |
Updated•10 months ago
|
|
|
||
Comment 62•10 months ago
|
||
|
||
Updated•10 months ago
|
|
|
Assignee | |
Comment 65•10 months ago
|
||
|
|
||
Comment 66•10 months ago
|
||
|
||
Comment 68•10 months ago
|
||
Backed out as requested by Whimboo
Backout link: https://hg.mozilla.org/integration/autoland/rev/a03a9d4e55c5c7d27b4c7784e66ff2b0adbb9fbd
|
|
||
Comment 69•10 months ago
|
||
|
|
Assignee | |
Updated•10 months ago
|
|
|
Assignee | |
Updated•10 months ago
|
|
|
||
Comment 70•10 months ago
|
||
Comment on attachment 9394990 [details]
Bug 1522790 - [wpt] Fix intermittent failures in font-face-[003, 004].html.
Revision D206622 was moved to bug 1891476. Setting attachment 9394990 [details] to obsolete.
|
|
||
Updated•10 months ago
|
|
|
Assignee | |
Updated•9 months ago
|
|
|
Assignee | |
Comment 71•9 months ago
|
||
For being able to better analyze the slowness in navigation with the changes applied I would like to first change the navigation handling in Marionette from page load events to the webprogress listener. This means we are blocked on bug 1664165.
|
|
Assignee | |
Updated•8 months ago
|
|
||
Comment 72•8 months ago
|
||
How come comment #68 added massive if (os == "android") and fission: [OK, TIMEOUT] everywhere? Not sure that's about the backed out patches?
|
|
||
Comment 73•8 months ago
|
||
Oh maybe https://phabricator.services.mozilla.com/D207318 is the culprit 🙂
|
|
Assignee | |
Comment 74•8 months ago
|
||
(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #73)
Oh maybe https://phabricator.services.mozilla.com/D207318 is the culprit 🙂
Yes, sadly we had to backout the patch again. With the next landing it should hopefully be sticky and we will have way lesser timing out tests on Android.
|
|
||
Comment 75•8 months ago
|
||
I'm not sure those are actual timeouts or just bug 1798871, I've been removing them from my modules and so far there was no other Android timeouts happened.
|
|
Assignee | |
Comment 76•8 months ago
|
||
Note that there could be a race on GeckoView when opening a new window and when it is actually ready to work with. I've fixed a specific case (bug 1891008) for Marionette but the API will be in use with this patch series. I'll observe the other bug to stay up-to-date. Thanks!
|
|
Assignee | |
Comment 77•7 months ago
|
||
I'm not able to finish this work in M11. As such moving to M12.
|
|
||
Comment 78•5 months ago
|
||
Depends on D219657
|
|
||
Comment 79•5 months ago
|
||
Comment on attachment 9420158 [details]
Bug 1522790 - Update test grouping logic for web-platform-tests-canvas
Revision D219761 was moved to bug 1914209. Setting attachment 9420158 [details] to obsolete.
|
|
Assignee | |
Comment 80•5 months ago
•
|
||
After the fix on bug 1761634 we should now have more stable results. As such I pushed another complete try build for web-platform tests. Lets cross fingers that we are mostly green now.
https://treeherder.mozilla.org/jobs?repo=try&revision=b315c19d563e9502bf95beb06051aa601ebb88e0
|
|
Assignee | |
Updated•5 months ago
|
|
|
Assignee | |
Comment 81•5 months ago
|
||
We can remove bug 1664165 from the dependency list given that the WebProgress listener is not needed to get these patches landed.
|
||
Updated•5 months ago
|
|
||
Updated•5 months ago
|
|
|
Assignee | |
Comment 82•4 months ago
|
||
Thanks to Olivia there is now a Google document that is used to investigate all the unexpected results of tests for Android. Hopefully it won't take such a long time to finish.
|
|
Assignee | |
Comment 83•4 months ago
|
||
Actually I wrongly triggered the web-platform tests on Android with Fission enabled instead of with SHIP. So the results shouldn't be taken into account. To ensure that SHIP is no longer affected by timeouts of the testharness I've triggered those jobs now:
So far it looks good and if there are unexpected passes / fails those should be handled on a different bug because SHIP as well is not a default job.
|
|
||
Updated•4 months ago
|
|
|
||
Updated•4 months ago
|
|
|
||
Updated•4 months ago
|
|
|
Assignee | |
Comment 84•4 months ago
|
||
Bug 1891706 didn't occur anymore in the last try build so lets remove it from the dependency list. But I just stumbled over bug 1890113 which is as well on the dependency list and wasn't investigated yet. I triggered wpt jobs for that build type on the same try build from comment 83.
|
|
||
Comment 85•4 months ago
|
||
|
|
Assignee | |
Comment 86•4 months ago
|
||
(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #84)
Bug 1891706 didn't occur anymore in the last try build so lets remove it from the dependency list.
This failure is still present but intermittently and most likely needs a given set of tests run. On autoland we randomly choose tests so this might not have been seen on try. I'll take a look at it.
|
|
Assignee | |
Updated•4 months ago
|
|
|
||
Comment 87•4 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/8046864b1279
https://hg.mozilla.org/mozilla-central/rev/377a55ac4ec5
https://hg.mozilla.org/mozilla-central/rev/a79c4f87abb4
|
|
||
Comment 89•4 months ago
|
||
|
|
||
Comment 90•4 months ago
|
||
| bugherder | ||
Description
•