ensureWindow for reftests has to wait until page of added browser element has been loaded
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox82 fixed)
| Tracking | Status | |
|---|---|---|
| firefox82 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [marionette-fission-mvp])
Attachments
(2 files)
Right now the setupWindow method in reftest.js adds a new browser element but doesn't wait for the containing page to be loaded, and returns immediately:
That means there is a chance of overlapping navigation requests causing trouble with the page load events for the initial page load in loadTestUrl().
|
Assignee | |
Comment 1•4 years ago
|
||
Actually another problem is that we do not correctly initialize this.lastURL. By default it is null, whereby we always have a default page. For desktop it is about:blank, and for GeckoView based browsers we currently have about:newtab and as such to trigger a navigation to about:blank until bug 1533058 is fixed. Nevertheless initializing this.lastURL to the browser's current URL would make sense.
|
|
Assignee | |
Comment 2•4 years ago
|
||
Note that initializing lastURL based on the current URL doesn't work because this.driver.curBrowser.contentBrowser returns null for it but only on mobile. So in case of mobile I will just hard-code about:blank given that we have to load it anyway right before.
Here an initial try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37d26c811bae2522aeb29613782a0fb3ef4a0639
|
|
Assignee | |
Comment 3•4 years ago
|
||
Actually the necessary changes have to happen in ensureWindow().
|
|
Assignee | |
Comment 4•4 years ago
|
||
When the first test URL is "about:blank" the reftest window
needs to refresh its location and not trigger a new navigation.
Only by doing that proper page load events will occur.
By correctly initializing the lastURL property, "loadTestURL"
will be able to determine the right command to execute, and not
always trigger a navigation.
|
|
Assignee | |
Comment 5•4 years ago
|
||
Without waiting for the initial navigation to "about:blank" to
complete, there is a risk for a race condition given that the
navigation to the test URL happens immediately.
Depends on D86966
|
|
Assignee | |
Comment 6•4 years ago
|
||
Anny, can you please check if those patches make it work for you? Thanks
|
|
Assignee | |
Comment 7•4 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #2)
Here an initial try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37d26c811bae2522aeb29613782a0fb3ef4a0639
Win7 debug reftests have again some flaky test results. I will have to check if those are similar to the font ones we experienced on bug 1651297.
|
||
Comment 8•4 years ago
|
||
These patches work for me (tests on linux64 & android pass).
|
|
Assignee | |
Comment 9•4 years ago
|
||
With the dependency fixed now, I've updated the patches and pushed a new try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26b2a8676b46025fbcee7068b27f73e9014f3a9
|
|
Assignee | |
Comment 10•4 years ago
•
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #9)
With the dependency fixed now, I've updated the patches and pushed a new try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26b2a8676b46025fbcee7068b27f73e9014f3a9
We seem to have more broken font tests on Windows 7 (similar to bug 1650866).
I just triggered another try build for those failing tests with the Marionette log level set to trace:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=142dc412cc1ca70efb77e62c50ad5db941f3b3cc
|
|
Assignee | |
Comment 11•4 years ago
|
||
For the last try build the number of failures are actually low. So I don't think marking intermittently failing tests as known to be failing makes sense on this bug.
|
||
Comment 12•4 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2125e2962dcb [marionette] Properly initialize lastURL for reftests. r=marionette-reviewers,jgraham,maja_zf https://hg.mozilla.org/integration/autoland/rev/c8a6c4e4fb9f [marionette] Wait for initial navigation to "about:blank" to complete on Android. r=marionette-reviewers,jgraham,maja_zf
|
||
Comment 13•4 years ago
|
||
Backed out for perma failures.
Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314003610&repo=autoland&lineNumber=46095
Backout: https://hg.mozilla.org/integration/autoland/rev/dd4ecb5d6a3d42fc757675d9b6c1471d82e0fbda
|
|
Assignee | |
Comment 14•4 years ago
|
||
The fault here is in the first patch. We cannot access this.driver.currentURL before the switch to the newly opened reftest window has happened. Moving this line behind the following line will fix it:
await this.driver.setWindowHandle(found, true);
|
|
||
Comment 15•4 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fdf1d97d31b5 [marionette] Properly initialize lastURL for reftests. r=marionette-reviewers,jgraham,maja_zf https://hg.mozilla.org/integration/autoland/rev/171372ddeac4 [marionette] Wait for initial navigation to "about:blank" to complete on Android. r=marionette-reviewers,jgraham,maja_zf
|
||
Comment 16•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/fdf1d97d31b5
https://hg.mozilla.org/mozilla-central/rev/171372ddeac4
|
|
Assignee | |
Updated•4 years ago
|
|
||
Updated•1 year ago
|
Description
•