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
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Updated•2 years ago
|
Description
•