1747359 - waitForInitialNavigationCompleted doesn't recognize uninitialized documents

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The upcoming patch should also fix all the known regressions from bug 1739369 and as such would be warranted on mozilla-beta.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1747359 - [marionette] Improve checks for tab's initial page load.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1747359 - [marionette] Keep reference to webProgress to unregister progress listener.

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

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[[ex-Mozilla] Agi Sferro | :agi]

That's correct, we don't replace the initial about:blank in a tab until we load an actual page.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Agi!

As spoken again with Olli we may wanna try the following path:

1. Setup the WebProgress listener for watching potential navigations
2. Setup a timer maybe around 50ms that will cancel the wait
3. Check if isInitialDocument and isLoadingDocument 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
4. 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

[[ex-Mozilla] Agi Sferro | :agi]

(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.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/499d5b03d4af
https://hg.mozilla.org/mozilla-central/rev/f64b4b7a69a1

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 type tab.
• 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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ceef8ed532d
https://hg.mozilla.org/releases/mozilla-beta/rev/f86f46dafe38

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258458 [details]
Bug 1747359 - [marionette] Improve checks for tab's initial page load.

Approved for 97.0b6.