Closed Bug 1736323 Opened 3 years ago Closed 3 years ago

Marionette has to wait for the very first tab during startup being finished loading

Categories

(Remote Protocol :: Marionette, defect, P2)

Firefox 95
defect

Tracking

(firefox-esr78 unaffected, firefox-esr91 fixed, firefox93 unaffected, firefox94 unaffected, firefox95+ fixed, firefox96 fixed)

RESOLVED FIXED
96 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- fixed
firefox93 --- unaffected
firefox94 --- unaffected
firefox95 + fixed
firefox96 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

As we have already seen last week before landing bug 1735500 there were issues with some Marionette and Wdspec tests. These had been fixed, and the patch got re-landed. But by that time I already had the impression that more work in Marionette will be necessary.

Now after the weekend various tests are starting to fail that get run right after Firefox has been started. Due to the changes on bug 1735500 we have a process switch very early during the load of the page for the currently selected tab.

Right now Marionette does not wait for the initial tab to be loaded, and as such some tests immediately interact with the document (get elements, execute script) and fail because the document gets unloaded / replaced.

Given that Marionette / WebDriver expects a valid browsing context being initially selected, the code in New Session has to make sure to not only wait for the initial browser window to be fully loaded but also for the page in the currently selected tab.

Given that the patch from bug 1735500 is important, we will have to fix the underlying issue in Marionette as soon as possible because it opens a race condition that can affect a lot of users.

Blocks: 1736169
Blocks: 1736191
Blocks: 1736146
Blocks: 1736147
Type: enhancement → defect

Set release status flags based on info from the regressing bug 1735500

I can reproduce the failure permanently now when running the following command:

mach test testing/firefox-ui/tests/functional/security/test_ssl_status_after_restart.py --headless

And it started to happen with the following changeset landed:
https://hg.mozilla.org/mozilla-central/rev/16d787b72947

The situation here now is that we no longer have the initial tab as web content process (E10SUtils.DEFAULT_REMOTE_TYPE) but as privileged about process. That means that each and every navigation in tests is most likely causing a process switch now.

Blocks: 1736411
Blocks: 1737282
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

If the New Session command doesn't wait for the initial tab
to have finished loading, any other command send right away
could fail because the document could be replaced.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d95be87e96f7
[marionette] "WebDriver:NewSession" has to wait for the very first tab to finish loading. r=webdriver-reviewers,jdescottes
Regressions: 1739008
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

We basically also want to have this fix in beta for the Firefox 95 release, but I'll post-pone an uplift request until the newly detected regression (bug 1739008) has been identified and fixed.

No longer blocks: 1736411

The patch landed in nightly and beta is affected.
:whimboo, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
If yes, don't forget to request an uplift for the patches in the regression caused by this fix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(hskupin)
Flags: needinfo?(hskupin)

If the New Session command doesn't wait for the initial tab
to have finished loading, any other command send right away
could fail because the document could be replaced.

Attachment #9265782 - Attachment is obsolete: true

The try build that I pushed with the changes works just fine for Wd1 jobs:
https://treeherder.mozilla.org/jobs?repo=try&revision=7b426210e16c20b634d62f3fecc4b8659f5a9947

I'll push one more try build with the needed eslint fixes and if that passes as well, we can land the patch on esr91:
https://treeherder.mozilla.org/jobs?repo=try&revision=f2b22ab662d70ca4544b0165209199cf26fdeb89

Julian, mind taking a quick look if it all looks fine from your side?

Flags: needinfo?(jdescottes)

One small nit: there is an extra "+" from the diff at that line: https://hg.mozilla.org/try/file/156a06dd8af6077f5225d0c1f1e68cf19fc659e6/remote/marionette/navigate.js#l204

Otherwise this looks good to me, thanks!

Flags: needinfo?(jdescottes)

Good find. Pushed a new try which should be our final candidate then:
https://treeherder.mozilla.org/jobs?repo=try&revision=315304454cefa12e8a74744165a71bfe4d390710

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #12)

Good find. Pushed a new try which should be our final candidate then:
https://treeherder.mozilla.org/jobs?repo=try&revision=315304454cefa12e8a74744165a71bfe4d390710

We need to uplift the patch on this bug to esr91 to fix the intermittent failures as visible on bug 1737282. Ryan can you please approve and land the commit that is part of this try build? Thanks.

Flags: needinfo?(ryanvm)

Comment on attachment 9248852 [details]
Bug 1736323 - [marionette] "WebDriver:NewSession" has to wait for the very first tab to finish loading.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: With not having this patch on esr91 we will keep around a frequent intermittent failure that we were ask to fix for this branch.
  • User impact if declined: Probably not given that it's a rare situation.
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Fixes a race condition.
Attachment #9248852 - Flags: approval-mozilla-esr91?

Comment on attachment 9248852 [details]
Bug 1736323 - [marionette] "WebDriver:NewSession" has to wait for the very first tab to finish loading.

Approved for ESR91, thanks for the rebase.

Flags: needinfo?(ryanvm)
Attachment #9248852 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

I am against this change.
Not all scenarios require interaction with the current content.
In particular, my network is very poor and pages often take a long time to fully load. The long wait almost makes it impossible for me to continue using Marionette.

https://bugzilla.mozilla.org/show_bug.cgi?id=1792780

Kil, thanks for your feedback. Can you please file a new bug to get this discussed? That one is closed for quite some time. Maybe you can also add a testcase and steps in how you start Firefox to be affected by that. Thanks.

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: