Commands have to check the appropriate browsing context (and not the current window) if it's still available
Categories
(Remote Protocol :: 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•4 years ago
|
|
|
Assignee | |
Comment 3•4 years 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•4 years 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•4 years ago
|
|
|
Assignee | |
Comment 5•4 years 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•4 years 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•4 years 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•4 years 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•4 years 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•4 years ago
|
|
|
Assignee | |
Comment 10•4 years 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•4 years ago
|
||
The patches for the regression will end-up on bug 1661495.
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 12•4 years 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•4 years ago
|
||
|
|
Assignee | |
Comment 14•4 years ago
|
||
Depends on D88951
|
|
Assignee | |
Comment 15•4 years ago
|
||
Depends on D88952
|
||
Comment 16•4 years 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•4 years ago
|
|
||
Comment 17•4 years 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.
|
||
Comment 19•4 years 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
Upstream PR merged by jgraham
|
||
Updated•1 year ago
|
Description
•