2048233 - WebSerial browser window integration is potentially broken in multiple window case, and tightly coupled into browser-init

Status: RESOLVED FIXED

(Core :: DOM: Web Serial, defect, P2)

Description

[:Gijs (he/him)]

Unfortunately it would appear that https://phabricator.services.mozilla.com/D281259 did not get desktop core reviewers or other desktop folks involved for the changes in browser-init.js.

As it stands, every browser window will register an observer for "serial-device-state-changed" and the same observer will run N times for N windows, if/when the topic is notified, and every window will build up a weakmap pointing to browser objects across multiple windows. This is also adding globals to the main browser window, which we're desperately trying to avoid (we have a lint rule for browser.js globals and everything - we did not imagine people would start adding them to the other browser-n files which generally only have 1 global in them tobegin with).

We also reset the count if permission is removed (in only 1 map, in the window where the permission is removed - not all the other windows!), but nothing appears to ever read the map so it's unclear to me what the purpose of the map is.

If we do need it, this should be implemented using a singleton module, and probably use the ObserverForwarder to get instantiated so we do not pay JS load/run costs for it all the time when mostly, it will be unused code.

Greg, can you clarify the point of the map so we can make sure this is an actionable bug?

Comment 1

[Greg Stoll :gstoll]

Ack, sorry about this!

The point of this is to call updateBrowserSharing() when a serial port is in use (or when we were previously using serial ports but aren't any more), which in turn sets the "sharing" attribute on the tab, which we look at in CSS to show or hide the "port in use" icon (similar to if a camera is active, etc.)

The map gets read here so we can increment or decrement the current count for that browser.

Would this make sense to use an ObserverForwarder?

Comment 2

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:cmartin, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Greg Stoll :gstoll]

Attachment: Bug 2048233 - use ObserverForwarder for serial device state change events r=#firefox-desktop-core-reviewers!

This avoids having one of these per window.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/827e603217b8
https://hg.mozilla.org/integration/autoland/rev/c5d1fb43071e
use ObserverForwarder for serial device state change events r=firefox-desktop-core-reviewers ,mconley

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c5d1fb43071e