Closed Bug 1493108 Opened 3 years ago Closed 1 year ago

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

Categories

(Testing :: Marionette, defect, P1)

Version 3
defect

Tracking

(Fission Milestone:M6c, firefox82 fixed)

RESOLVED FIXED
82 Branch
Fission Milestone M6c
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
Duplicate of this bug: 1349861
Duplicate of this bug: 1255946

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.

Depends on: 1519335

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.

Type: enhancement → defect
Summary: Commands in listener.js have to check if "current browsing context" is open (fails for removed frames) → Commands have to check the appropriate browsing context (and not the current window) if it's still available
Whiteboard: [marionette-fission-mvp]
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1

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.

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

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.

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.

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

Severity: normal → S1

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.

Severity: S1 → S3
Depends on: 1661495

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

Fission Milestone: --- → M6c

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?

Flags: needinfo?(james)

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.

Flags: needinfo?(james)
Whiteboard: [marionette-fission-mvp] → [marionette-fission-mvp][complex]
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.
Whiteboard: [marionette-fission-mvp][complex] → [marionette-fission-mvp][complex], [wptsync upstream error]
Regressions: 1665210
Regressions: 1665216
Regressions: 1665377
Regressions: 1582736
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25594 for changes under testing/web-platform/tests
Whiteboard: [marionette-fission-mvp][complex], [wptsync upstream error] → [marionette-fission-mvp][complex], [wptsync upstream]
Upstream PR merged by jgraham
Regressions: 1665437
No longer regressions: 1665216
You need to log in before you can comment on or make changes to this bug.