1493108 - Commands have to check the appropriate browsing context (and not the current window) if it's still available

Status: RESOLVED FIXED

(Testing :: Marionette, defect, P1)

Description

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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

Comment 3

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 4

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 5

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 6

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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:

https://github.com/w3c/webdriver/issues/1539

Comment 7

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 8

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 9

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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.

Comment 11

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

The patches for the regression will end-up on bug 1661495.

Comment 12

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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?

Comment 13

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Attachment: Bug 1493108 - [marionette] Each command has to check the appropriate BrowsingContext for existence.

Comment 14

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Attachment: Bug 1493108 - [marionette] Use browsing context to return current (chrome) window handle.

Depends on D88951

Comment 15

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Attachment: Bug 1493108 - [wdspec] Improve tests for browsing context checks.

Depends on D88952

Comment 16

[James Graham [:jgraham]]

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.

Comment 17

[Pulsebot]

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

Comment 18

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

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 19

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/bcf6908307f7
https://hg.mozilla.org/mozilla-central/rev/b9cfef271995
https://hg.mozilla.org/mozilla-central/rev/12fadd0009e4

Comment 20

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

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

Comment 21

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

Upstream PR merged by jgraham