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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Attachment: Bug 1736323 - [marionette] "WebDriver:NewSession" has to wait for the very first tab to finish loading.

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.

Comment 4

[Pulsebot]

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

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d95be87e96f7

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 8

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/d76cabaec0c1

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Attachment: Bug 1736323 - [marionette] (esr91) "WebDriver:NewSession" has to wait for the very first tab to finish loading.

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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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?

Comment 11

[Julian Descottes [: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!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/2a3841952a3b

Comment 17

[kil]

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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.