1661495 - Broken tracking of current browsing context after opening new windows

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

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

[Tracking Requested - why for this release]:
As discovered while working on bug 1493108 and later by users on Github we have a major regression in Marionette. Several commands operate on a different window than the expected one and as such cause invalid data to be returned. The patch from bug 1654628 as landed cannot be backed out given that a certain amount of other dependent patches already landed. So this needs to block the 81 release.

Here a try push for the required changes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07f34218f3ecfbb2a3316f558e8eef0d85c3a747

Comment 1

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

Alex and Titus, could you both please check if the following build from the try push fixes the problem for you?

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/NgogFoBUS9qDfTRYV5p9oQ/runs/0/artifacts/public/build/target.tar.bz2

Thanks!

Comment 2

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

Attachment: Bug 1661495 - [marionette] Add context argument to getBrowsingContext.

It should be possible to get the browsing context not only for the
currently selected context (chrome vs. content), but also for a
specific one.

Comment 3

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

Attachment: Bug 1661495 - [marionette] Account for a non-existent browsing context in getBrowsingContext.

If a window does no longer exist the associated browsing context
will no longer be valid and set to null.

Depends on D88439

Comment 4

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

Attachment: Bug 1661495 - [marionette] Improve tracking of browsing contexts.

With the changes on bug 1654628 we do not completely track the current
browsing contexts, but loose the reference when windows are closed.
With this patch the appropriate browsing contexts will be correctly
reset.

Also the commands for switching frames are operating on content
windows and as such are not allowed to update the currently
selected chrome browsing context.

Depends on D88440

Comment 5

[Alexei]

Looks good to me so far! Thank you!

Comment 6

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

Attachment: Bug 1661495 - [marionette] Allow usage of switchToTab for GeckoView.

GeckoView allows only a single tab. But that shouldn't prevent the
code to make use of switchToTab() as long as the target tab is
the currently selected one.

Also reftests currently override window.gBrowser which leads to a
wrong detection of Firefox instead of GeckoView. Checking for
Android first will workaround the problem for now.

Depends on D88440

Comment 7

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

Thanks for testing Alexei. Good to hear that it's also fixed for you.

The problem with GeckoView was that the reftest code always sets window.gBrowser and that leads to a wrong detection of the tabBrowser in Marionette.

The newly uploaded revision fixes that. Here a new try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b143c986e27fd67bc6db265d863c8bcd32de50e

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7ca12380fdd
[marionette] Add context argument to getBrowsingContext. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/d0d932e73944
[marionette] Account for a non-existent browsing context in getBrowsingContext. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/bf8d8cb0f57c
[marionette] Allow usage of switchToTab for GeckoView. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/7d76fc9a1d41
[marionette] Improve tracking of browsing contexts. r=marionette-reviewers,maja_zf

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b7ca12380fdd
https://hg.mozilla.org/mozilla-central/rev/d0d932e73944
https://hg.mozilla.org/mozilla-central/rev/bf8d8cb0f57c
https://hg.mozilla.org/mozilla-central/rev/7d76fc9a1d41

Comment 10

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

Actually this is not fixed yet. By running the New Window command it is still broken. I will get this patched too, and actually add wdspec tests for both cases, which I formerly missed.

Comment 11

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

There is actually a second regression but now in Firefox 80 with the landing of bug 1652932. Here we accidentally set the current content browsing context to the newly created browser while it should only be done by WebDriver:SetWindowHandle.

Comment 12

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

Attachment: Bug 1661495 - [marionette] Don't always set the current content browsing context when registering a new browser.

Since the patch on bug 1652932 landed in Firefox 80 we accidentally
update the current content browsing context. That leads to an unexpected
change of the current window handle, which is only allowed via
"WebDriver:SetWindowHandle".

Comment 13

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

Attachment: Bug 1661495 - [marionette] Update content browsing context for remoteness changes.

If a navigation in the current browser causes a remoteness change,
the current content browsing context needs to be updated.

Depends on D88771

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46f9aa946264
[marionette] Don't always set the current content browsing context when registering a new browser. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/c5cdfb592493
[marionette] Update content browsing context for remoteness changes. r=marionette-reviewers,maja_zf

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25300 for changes under testing/web-platform/tests

Comment 16

[Narcis Beleuzu [:NarcisB]]

Backed out as req by whimboo

Backout link: https://hg.mozilla.org/integration/autoland/rev/33a5cae7c3fc31a1d5cc3fef940b2b6e024c5ae1

Comment 17

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

Attachment: Bug 1661495 - [marionette] Reset content browsing context if new chrome window is not a browser window.

Fixes a regression from bug 1661495, which missed to reset the
current content browsing context if the new chrome window isn't
a browser window.

Depends on D88827

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/46f9aa946264
https://hg.mozilla.org/mozilla-central/rev/c5cdfb592493

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19c8806a5dcb
[marionette] Don't always set the current content browsing context when registering a new browser. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/29bba6616e18
[marionette] Update content browsing context for remoteness changes. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/c8320f7940b2
[marionette] Reset content browsing context if new chrome window is not a browser window. r=marionette-reviewers,maja_zf

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/19c8806a5dcb
https://hg.mozilla.org/mozilla-central/rev/29bba6616e18
https://hg.mozilla.org/mozilla-central/rev/c8320f7940b2

Comment 21

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

Comment on attachment 9172429 [details]
Bug 1661495 - [marionette] Add context argument to getBrowsingContext.

Beta/Release Uplift Approval Request

• User impact if declined: The code refactoring for Fission caused a regression where Marionette looses the reference to the currently selected browsing context. That means when new windows or tabs are getting opened or closed, automated tests will fail. Affected here are also 3rd-party tools like Selenium, which are heavily used by web developers to test their websites.

There is no feasible workaround available, which means that running tests against Firefox 81 will be completely broken.

• 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: Given that the nightly build isn't available yet, the fix has been verified based on the CI build for the mozilla-central merge.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): As the previously not failing tests after the refactoring have been shown the test coverage was not complete. By adding more tests now we have better confidence, but there is still a remaining chance that not all available code paths have been covered yet, but those will be less often utilized.

The original reporter has also verified that the fix is working for him.

We would like to get this fixed on beta to get further early feedback from the Selenium community so that we can ship a high quality Marionette experience.

• String changes made/needed: no

Comment 22

[Ryan VanderMeulen [:RyanVM]]

At least the "Improve tracking of browsing contexts." patch has conflicts with Beta. Please rebase. Feel free to link to a Try push w/ the rebased patches like we've done in the past for big patch stacks.

Comment 23

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

Two of the patches have indeed merge conflicts, which are mostly for tests. Here a try push with the grafted patch stack on beta:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c32b0c3bb69360180808680e8af81160b944dce

Comment 24

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

Ryan, finally here is the try build with the correctly grafted commits on mozilla-beta:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2debb0d0520edb47fa9d7011b8edd5ee37d041d

Comment 25

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

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #15)

Created web-platform-tests PR
https://github.com/web-platform-tests/wpt/pull/25300 for changes under
testing/web-platform/tests

James, can you please check what's stopping us from getting this PR merged? Thanks.

Comment 26

[James Graham [:jgraham]]

https://github.com/web-platform-tests/wpt/pull/25300/checks?check_run_id=1053492897 is pretty clear; the user prompt tests are unstable in gecko, presumably working once and failing on reload. If this is expected we can admin merge the PR.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172429 [details]
Bug 1661495 - [marionette] Add context argument to getBrowsingContext.

Approved for 81.0b6.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/895661960a5a
https://hg.mozilla.org/releases/mozilla-beta/rev/75d30a3da945
https://hg.mozilla.org/releases/mozilla-beta/rev/8caea6dc7208
https://hg.mozilla.org/releases/mozilla-beta/rev/2f8d39bae98a
https://hg.mozilla.org/releases/mozilla-beta/rev/28c523209fa0
https://hg.mozilla.org/releases/mozilla-beta/rev/ae5c053bce6e
https://hg.mozilla.org/releases/mozilla-beta/rev/98fbf0defd5f

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham