Closed Bug 1680479 Opened 3 years ago Closed 3 years ago

Make window handles of top-level browsing contexts unique (no change for browsing context swaps)

Categories

(Remote Protocol :: Marionette, enhancement, P2)

Default
enhancement
Points:
8

Tracking

(Fission Milestone:M7a, firefox90 fixed)

RESOLVED FIXED
90 Branch
Fission Milestone M7a
Tracking Status
firefox90 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [bidi-m1-mvp], [wptsync upstream])

Attachments

(1 file)

With the fix on bug 1674329 we update the handle of the currently selected window. But if other windows are open in the background, and a navigation gets triggered in one of those, we will miss that the top-level browsing context might get changed.

Due to those top-level browsing context swaps the window handle will change, and as such tests won't be able to identify the formerly window anymore.

The only constant value is the browsingContext.browserId, which references the actual tab. If we cannot use the browserId directly, maybe it would be good to have a new mapping for browserId => unique id, which can be updated whenever a top-level browsing context changes.

Blocks: 1682062

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

The only constant value is the browsingContext.browserId, which references the actual tab. If we cannot use the browserId directly, maybe it would be good to have a new mapping for browserId => unique id, which can be updated whenever a top-level browsing context changes.

With what we just have seen on bug 1682062 browserId is also not a feasible replacement. While it is unique and that is what we want, it depends on a valid content browser being attached to the tab. But as long as a tab is unloaded there is no such content browser, and the browserId stays at 0 for all those tabs.

We will have to check how to best identify tabs after bug 1669172 has been landed. Maybe we have to map permanentKey to a uuid to really have stable and unique entries.

Depends on: 1669172
Blocks: 1691493
Priority: -- → P2
Whiteboard: [bidi-m1-mvp]
See Also: → 1692468
Points: --- → 8
Blocks: 1646289

While working on bug 1704697 to get BFCache working with Fission in Marionette I noticed a couple of failures that are related to a missing unique identifier for tabs. Loading web pages can cause a process switch and as such the top-level browsing context is swapped / replaced. After that switching windows will fail for cached window handles because the handle is no longer valid. With BFCache + Fission this scenario is now much more likely.

Here a Wdspec test result:
https://treeherder.mozilla.org/logviewer?job_id=338030756&repo=try&lineNumber=9289-9345

[task 2021-04-28T13:11:44.267Z] 13:11:44     INFO - PID 2959 | 1619615504265	Marionette	DEBUG	0 -> [0,19,"WebDriver:SwitchToWindow",{"handle":"41","name":"41"}]
[..]
[task 2021-04-28T13:11:44.348Z] 13:11:44     INFO - PID 2959 | 1619615504345	Marionette	DEBUG	0 -> [0,20,"WebDriver:Navigate",{"url":"http://web-platform.test:8000/webdriver/tests/support/inline.py?doc=%3C%21doctype+html%3E%0A%3Cmeta+charset%3DUTF-8%3E%0A%3Cp+id%3D%27b%27%3Eother&mime=text%2Fhtml&charset=UTF-8"}]
[task 2021-04-28T13:11:44.356Z] 13:11:44     INFO - PID 2959 | 1619615504354	Marionette	TRACE	Received event beforeunload for about:blank
[task 2021-04-28T13:11:44.395Z] 13:11:44     INFO - PID 2959 | 1619615504392	Marionette	TRACE	Remoteness change detected. Set new top-level browsing context to 45
[task 2021-04-28T13:11:44.408Z] 13:11:44     INFO - PID 2959 | 1619615504405	Marionette	TRACE	Received event pagehide for about:blank
[task 2021-04-28T13:11:44.418Z] 13:11:44     INFO - PID 2959 | 1619615504410	Marionette	TRACE	[45] MarionetteEvents actor created for window id 34
[task 2021-04-28T13:11:44.423Z] 13:11:44     INFO - PID 2959 | 1619615504421	Marionette	TRACE	Received event beforeunload for about:blank
[task 2021-04-28T13:11:44.487Z] 13:11:44     INFO - PID 2959 | 1619615504485	Marionette	TRACE	Received event pagehide for about:blank
[task 2021-04-28T13:11:44.504Z] 13:11:44     INFO - PID 2959 | 1619615504502	Marionette	TRACE	[45] MarionetteEvents actor created for window id 6442450946
[task 2021-04-28T13:11:44.553Z] 13:11:44     INFO - PID 2959 | 1619615504550	Marionette	TRACE	Received event DOMContentLoaded for http://web-platform.test:8000/webdriver/tests/support/inline.py?doc=%3C%21doctype+html%3E%0A%3Cmeta+charset%3DUTF-8%3E%0A%3Cp+id%3D%27b%27%3Eother&mime=text%2Fhtml&charset=UTF-8
[task 2021-04-28T13:11:44.556Z] 13:11:44     INFO - PID 2959 | 1619615504551	Marionette	TRACE	Received event pageshow for http://web-platform.test:8000/webdriver/tests/support/inline.py?doc=%3C%21doctype+html%3E%0A%3Cmeta+charset%3DUTF-8%3E%0A%3Cp+id%3D%27b%27%3Eother&mime=text%2Fhtml&charset=UTF-8
[task 2021-04-28T13:11:44.567Z] 13:11:44     INFO - PID 2959 | 1619615504564	Marionette	DEBUG	0 <- [1,20,null,{"value":null}]
[..]
[task 2021-04-28T13:11:44.655Z] 13:11:44     INFO - PID 2959 | 1619615504653	Marionette	DEBUG	0 -> [0,22,"WebDriver:SwitchToWindow",{"handle":"39","name":"39"}]
[task 2021-04-28T13:11:44.686Z] 13:11:44     INFO - PID 2959 | 1619615504684	Marionette	TRACE	Received DOM event TabSelect for [object XULElement]
[task 2021-04-28T13:11:44.709Z] 13:11:44     INFO - PID 2959 | 1619615504706	Marionette	DEBUG	0 <- [1,22,null,{"value":null}]
[..]
[task 2021-04-28T13:11:44.846Z] 13:11:44     INFO - PID 2959 | 1619615504840	Marionette	DEBUG	0 -> [0,25,"WebDriver:SwitchToWindow",{"handle":"41","name":"41"}]
[task 2021-04-28T13:11:44.848Z] 13:11:44     INFO - PID 2959 | 1619615504843	Marionette	DEBUG	0 <- [1,25,{"error":"no such window","message":"Unable to locate window: 41","stacktrace":"WebDriverError@chrome://marionette/cont ... t@chrome://marionette/content/server.js:238:9\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:504:20\n"},null]

Julian, would you be interested to work on this bug or should I take it?

Flags: needinfo?(jdescottes)

Required preferences to enable BFCache and Fission are fission.autostart=true and fission.bfcacheInParent=true.

I will take the bug.

Quick note: I can reproduce the issue with

./mach wpt /webdriver/tests/close_window/close.py  --setpref fission.autostart=true --setpref fission.bfcacheInParent=true

But not with

./mach wpt /webdriver/tests/close_window/close.py  --enable-fission --setpref fission.bfcacheInParent=true

Somehow --enable-fission doesn't seem to have the same effect as --setpref fission.autostart=true ?

Flags: needinfo?(jdescottes)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

(In reply to Julian Descottes [:jdescottes] from comment #5)

Somehow --enable-fission doesn't seem to have the same effect as --setpref fission.autostart=true ?

Yes, that is bug 1680963 that James is working on to get fixed soon.

Btw. I briefly spoke with Olli and maybe we can indeed use the browserId for the unique window handle of tabs. Clients would have to make sure to really use chrome window handles separately, given that there can be identical ids between them.

Attachment #9219386 - Attachment description: Bug 1680479 - [marionette] Use stable browser ids as window handles → Bug 1680479 - [marionette] Use stable unique ids as window handles

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #6)

Btw. I briefly spoke with Olli and maybe we can indeed use the browserId for the unique window handle of tabs. Clients would have to make sure to really use chrome window handles separately, given that there can be identical ids between them.

As discussed on chat.mozilla.org, I use uuids for now, because of what was mentioned in comment 2:

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

With what we just have seen on bug 1682062 browserId is also not a feasible replacement. While it is unique and that is what we want, it depends on a valid content browser being attached to the tab. But as long as a tab is unloaded there is no such content browser, and the browserId stays at 0 for all those tabs.

Tracking this bug for Fission M7a because this bug blocks Fission bfcache (meta bug 1671510).

Fission Milestone: --- → M7a
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c15da7dafcab
[marionette] Use stable unique ids as window handles r=marionette-reviewers,webdriver-reviewers,whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29016 for changes under testing/web-platform/tests
Whiteboard: [bidi-m1-mvp] → [bidi-m1-mvp], [wptsync upstream]
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #11)

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

James, could you please merge? Thanks.

Flags: needinfo?(james)
Upstream PR merged by moz-wptsync-bot
Upstream PR merged by moz-wptsync-bot
Flags: needinfo?(james)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.