Closed Bug 1747359 Opened 2 years ago Closed 2 years ago

waitForInitialNavigationCompleted doesn't recognize uninitialized documents

Categories

(Remote Protocol :: Marionette, defect, P2)

Default
defect
Points:
2

Tracking

(firefox-esr91 unaffected, firefox96 unaffected, firefox97 fixed, firefox98 fixed)

RESOLVED FIXED
98 Branch
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)

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.

Summary: navigate.waitForInitialNavigationCompleted doesn't recognize unitialized documents → navigate.waitForInitialNavigationCompleted doesn't recognize uninitialized documents

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: nobody → hskupin
Status: NEW → ASSIGNED
Summary: navigate.waitForInitialNavigationCompleted doesn't recognize uninitialized documents → waitForInitialNavigationCompleted doesn't recognize uninitialized documents

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

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

Points: --- → 2
Whiteboard: [bidi-m3-mvp]

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.

Flags: needinfo?(agi)

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

Flags: needinfo?(agi)

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?

Flags: needinfo?(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.

Flags: needinfo?(agi)

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.

Blocks: 1750125
Attachment #9258458 - Attachment description: Bug 1747359 - [marionette] Dispatch check for initial webProgress loading state to main thread. → Bug 1747359 - [marionette] Improve checks for tab's initial page load.
Blocks: 1739540
Blocks: 1749626
Blocks: 1662808
Priority: P3 → P2
Blocks: 1750441
Blocks: 1750433
Blocks: 1533058
Blocks: 1750334
Blocks: 1750302
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch

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 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
Attachment #9258458 - Flags: approval-mozilla-beta?
Attachment #9258459 - Flags: approval-mozilla-beta?
Blocks: 1750993

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

Approved for 97.0b6.

Attachment #9258458 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9258459 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: