waitForInitialNavigationCompleted doesn't recognize uninitialized documents
Categories
(Remote Protocol :: Marionette, defect, P2)
Tracking
(firefox-esr91 unaffected, firefox96 unaffected, firefox97 fixed, firefox98 fixed)
Tracking | Status | |
---|---|---|
firefox-esr91 | --- | unaffected |
firefox96 | --- | unaffected |
firefox97 | --- | fixed |
firefox98 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [bidi-m3-mvp])
Attachments
(2 files)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
With bug 1739369 we will have a waitForInitialNavigationCompleted()
helper in the navigate.js module. Right now it doesn't check for the currentWindowGlobal.isInitialDocument
property.
With the work on bug 1726465 to start Marionette way earlier during the startup of Firefox I can see failures because the page's readyState
is uninitialized
. It means that no page load has been started at this point and as such this helper method doesn't wait for the upcoming page load but returns immediately.
I can remember that adding this check caused issues on Android so we should have an eye on that.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
The issue here is some code on the main thread is not getting run and as such doesn't update the webProgress's loading state. When I poll the event loop or use Services.tm.dispatchToMainThread()
for checking the loading state, it works all fine. This should also improve the stability from the changes on bug 1739369.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
The upcoming patch should also fix all the known regressions from bug 1739369 and as such would be warranted on mozilla-beta.
Assignee | ||
Comment 3•3 years ago
|
||
Assignee | ||
Comment 4•3 years ago
|
||
If the navigation causes the browsing context to be replaced,
the listener cannot be unregistered. As such keep a reference
to the original webProgress instance that can still be used.
Depends on D135594
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
After talking to Olli the uninitialized
readyState of the document means that we have the initial about:blank
loaded (browsingContext.currentWindowGlobal.isInitialDocument=true
). If this state stays it means that the document will not be replaced with another about:blank
as usually done when using window.open()
with a URL argument. On desktop Firefox explicitly loads a new about:blank
and isInitialDocument
will be false
afterwards. But that's not the case on mobile. GeckoView keeps the initial about:blank.
Agi, could you please confirm that when opening a tab (event the first one during startup) there is no additional navigation to about:blank
happening for GeckoView applications? Thanks.
Comment 6•3 years ago
|
||
That's correct, we don't replace the initial about:blank
in a tab until we load an actual page.
Assignee | ||
Comment 7•3 years ago
|
||
Thanks Agi!
As spoken again with Olli we may wanna try the following path:
- Setup the WebProgress listener for watching potential navigations
- Setup a timer maybe around 50ms that will cancel the wait
- Check if
isInitialDocument
andisLoadingDocument
are both false return immediately
3.1. Maybe also include Android for now so that we do not force the 50ms delay each time a tab/window gets opened - Wait for the timer or WebProgress to fire first and remove the listener and timer
Agi, is there maybe a flag or pref that we could use for 3.1. to be more careful in case the behavior will change for Android at some point?
Comment 8•3 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [βοΈUTC+1] from comment #7)
Agi, is there maybe a flag or pref that we could use for 3.1. to be more careful in case the behavior will change for Android at some point?
I'm not sure what you mean here. Do you mean a pref that controls whether we replace the initial about:blank
or not? No, we don't have anything like that.
Assignee | ||
Comment 9•3 years ago
|
||
Yes exactly. So in this case we will drop off early for Android then. If the behavior changes at some point we will probably notice when tests are starting to fail. Thanks.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/499d5b03d4af [marionette] Keep reference to webProgress to unregister progress listener. r=webdriver-reviewers,jdescottes https://hg.mozilla.org/integration/autoland/rev/f64b4b7a69a1 [marionette] Improve checks for tab's initial page load. r=webdriver-reviewers,jdescottes
Comment 11•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/499d5b03d4af
https://hg.mozilla.org/mozilla-central/rev/f64b4b7a69a1
Assignee | ||
Comment 12•3 years ago
|
||
Now that this has been fixed I'm going to wait one more day to check for possible regressions. If nothing comes up I'll request uplift tomorrow.
Assignee | ||
Comment 13•3 years ago
|
||
Comment on attachment 9258458 [details]
Bug 1747359 - [marionette] Improve checks for tab's initial page load.
Beta/Release Uplift Approval Request
- User impact if declined: Users of WebDriver clients could experience intermittent test failures when using the
WebDriver:NewWindow
command with typetab
. - Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Just adds additional checks and an extra timer to improve the initial page loaded detection.
- String changes made/needed: none
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 14•3 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5ceef8ed532d
https://hg.mozilla.org/releases/mozilla-beta/rev/f86f46dafe38
Comment 15•3 years ago
|
||
Comment on attachment 9258458 [details]
Bug 1747359 - [marionette] Improve checks for tab's initial page load.
Approved for 97.0b6.
Updated•3 years ago
|
Updated•2 years ago
|
Description
•