Broken tracking of current browsing context after opening new windows
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(Fission Milestone:M6c, firefox-esr68 unaffected, firefox-esr78 unaffected, firefox80 wontfix, firefox81+ fixed, firefox82+ fixed)
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox-esr78 | --- | unaffected |
firefox80 | --- | wontfix |
firefox81 | + | fixed |
firefox82 | + | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Regression, )
Details
(Keywords: regression, Whiteboard: [marionette-fission-mvp], [wptsync upstream])
Attachments
(7 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
[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
Assignee | ||
Comment 1•4 years ago
|
||
Alex and Titus, could you both please check if the following build from the try push fixes the problem for you?
Thanks!
Assignee | ||
Comment 2•4 years ago
|
||
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.
Assignee | ||
Comment 3•4 years ago
|
||
If a window does no longer exist the associated browsing context
will no longer be valid and set to null.
Depends on D88439
Assignee | ||
Comment 4•4 years ago
|
||
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
Looks good to me so far! Thank you!
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
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
Assignee | ||
Comment 7•4 years ago
|
||
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
Assignee | ||
Updated•4 years ago
|
Pushed by hskupin@mozilla.com: 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•4 years ago
|
||
bugherder |
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
Assignee | ||
Comment 10•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
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
.
Assignee | ||
Comment 12•4 years ago
|
||
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".
Updated•4 years ago
|
Assignee | ||
Comment 13•4 years ago
|
||
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•4 years ago
|
||
Pushed by hskupin@mozilla.com: 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25300 for changes under testing/web-platform/tests
Comment 16•4 years ago
|
||
Backed out as req by whimboo
Backout link: https://hg.mozilla.org/integration/autoland/rev/33a5cae7c3fc31a1d5cc3fef940b2b6e024c5ae1
Assignee | ||
Comment 17•4 years ago
|
||
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•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/46f9aa946264
https://hg.mozilla.org/mozilla-central/rev/c5cdfb592493
Comment 19•4 years ago
|
||
Pushed by hskupin@mozilla.com: 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
Updated•4 years ago
|
Comment 20•4 years ago
|
||
bugherder |
Assignee | ||
Comment 21•4 years ago
|
||
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
Assignee | ||
Updated•4 years ago
|
Comment 22•4 years ago
|
||
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.
Assignee | ||
Comment 23•4 years ago
|
||
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
Updated•4 years ago
|
Assignee | ||
Comment 24•4 years ago
|
||
Ryan, finally here is the try build with the correctly grafted commits on mozilla-beta:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2debb0d0520edb47fa9d7011b8edd5ee37d041d
Assignee | ||
Comment 25•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
|
||
Comment on attachment 9172429 [details]
Bug 1661495 - [marionette] Add context argument to getBrowsingContext.
Approved for 81.0b6.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 28•4 years ago
|
||
bugherder uplift |
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
Updated•4 years ago
|
Upstream PR merged by jgraham
Updated•1 year ago
|
Description
•