Commands have to check the appropriate browsing context (and not the current window) if it's still available
Categories
(Testing :: Marionette, defect, P1)
Tracking
(Fission Milestone:M6c, firefox82 fixed)
Tracking | Status | |
---|---|---|
firefox82 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [marionette-fission-mvp][complex], [wptsync upstream])
Attachments
(3 files)
We added the check for the "top-level browsing context" to each of the commands already a while ago, but we actually missed that we also have to check for each "browsing context".
Currently it fails when a frame is selected, and removed. All commands in listener.js raise failures like the following:
> Failed to gather test failure debug: TypeError: can't access dead object
> stacktrace:
> getPageSource@chrome://marionette/content/listener.js:1130:3
> dispatch/</req<@chrome://marionette/content/listener.js:482:14
> dispatch/<@chrome://marionette/content/listener.js:477:15
> MessageListener.receiveMessage*startListeners@chrome://marionette/content/listener.js:540:3
> registerSelf@chrome://marionette/content/listener.js:458:5
> @chrome://marionette/content/listener.js:1681:1
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 3•9 months ago
|
||
When we make use of the browsing context id (bug 1519335) we could use BrowsingContext.get(id)
to check if the given browsing context still exists.
Assignee | ||
Comment 4•7 months ago
|
||
As given by the WebDriver specification the appropriate browsing context has to be checked and not the window before a command can be executed. Right now Marionette does the latter and as such fails for content context, when eg. frames are getting removed.
Given that we can retrieve the browsing context via this.getBrowsingContext({ top: [true|false]})
, this can all be done in the driver class.
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Comment 5•7 months ago
|
||
While working on this bug I noticed that with https://hg.mozilla.org/mozilla-central/rev/0883c2b689df (bug 1654628) there is a lot of missing code for correctly handling the appropriate browsing contexts. As such we should try to land a follow-up fix on the 81 branch. A series of patches will be posted soon.
Assignee | ||
Comment 6•6 months ago
|
||
By working on this bug I noticed a problem with the spec and that it is currently not possible to switch away from a frame that has been removed by some Javascript code or element interaction command. I filed a webdriver spec issue for that:
Assignee | ||
Comment 7•6 months ago
|
||
Except some minor issues the try build look good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7322d8c2486b339fc185a20b5555b4cdc487618e&selectedTaskRun=LBRqjoxYQKGDx_rmY_vrog.0
Sadly I'm blocked on fixing the GeckoView failures because I'm not able to build a local artifact build. I will have to wait for bug 1661406 being fixed. Hopefully this will happen soon.
Assignee | ||
Comment 8•6 months ago
|
||
Please note that users of geckodriver also noticed a regression with Firefox 81 when opening a new tab:
https://github.com/mozilla/geckodriver/issues/1769
This corresponds to my findings in comment 5, and will be fixed as well.
Assignee | ||
Comment 9•6 months ago
|
||
[Tracking Requested - why for this release]:
The changes as mentioned in comment 5 and as landed for Firefox 81 caused a major regression for users of geckodriver. Several commands operate on a different window than the expected one and as such cause invalid data to be returned. The patch 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.
Assignee | ||
Updated•6 months ago
|
Assignee | ||
Comment 10•6 months ago
|
||
We should be able to land the first set of patches earlier to actually fix the regression first. As such I will file a separate bug for tracking the release blocker.
Assignee | ||
Comment 11•6 months ago
|
||
The patches for the regression will end-up on bug 1661495.
Updated•6 months ago
|
Assignee | ||
Comment 12•6 months ago
|
||
Latest try build including the changes from bug 1661495 looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9dbf29a9635ca0fe8b940e3576d1751a871aa54
James, what shall we do with https://github.com/w3c/webdriver/issues/1539? If we don't fix that and change our implementation to what the spec tells us, users won't be able to switch to the parent/top frame if the current browsing context has been removed. Workaround would be to switch to another window if available first, which is not actually what I would like to see. Would you have the time to at least give a comment on that issue, or even better update the wording?
Assignee | ||
Comment 13•6 months ago
|
||
Assignee | ||
Comment 14•6 months ago
|
||
Depends on D88951
Assignee | ||
Comment 15•6 months ago
|
||
Depends on D88952
Comment 16•6 months ago
|
||
I've commented on the issue. My feeling is that it's unclear how one should handle this; once a frame is removed it doesn't have a browsing context, so the concept of current top-level browsing context doesn't really make sense. I think that to fix this we'd need an explicit cache of the previously selected top-level browsing context, which is probably fine but it's a big change from the spec which currently computes everything at runtime.
Assignee | ||
Updated•6 months ago
|
Comment 17•6 months ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bcf6908307f7 [marionette] Each command has to check the appropriate BrowsingContext for existence. r=marionette-reviewers,maja_zf,jdescottes https://hg.mozilla.org/integration/autoland/rev/b9cfef271995 [marionette] Use browsing context to return current (chrome) window handle. r=marionette-reviewers,maja_zf https://hg.mozilla.org/integration/autoland/rev/12fadd0009e4 [wdspec] Improve tests for browsing context checks. r=webdriver-reviewers,jgraham
Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.
Updated•6 months ago
|
Comment 19•6 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bcf6908307f7
https://hg.mozilla.org/mozilla-central/rev/b9cfef271995
https://hg.mozilla.org/mozilla-central/rev/12fadd0009e4
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25594 for changes under testing/web-platform/tests
Updated•6 months ago
|
Upstream PR merged by jgraham
Description
•