2014794 - lifetime of SerialPort in workers doesn't get extended when events are added

Status: RESOLVED DUPLICATE of bug 2010930

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

Description

[Greg Stoll :gstoll]

Doing something like navigator.serial.getPorts().then(ports => {port[0].ondisconnect = () => console.log("disconnected")}); doesn't work in a worker (the event will never fire) because the lifetime of ports[0] doesn't get extended.

We should probably do what WebSocket does - see these methods for an example. (for more context see this comment)

Note that this doesn't seem to be a problem in a normal window. (outside of a worker)

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Note that workers are also eligible for garbage collection if the worker runtime doesn't realize that there's some potentially external source of events that might cause the worker to do something and a strong reference is not directly maintained to the Worker binding from new Worker(...). The "WorkerPrivate" MOZ_LOG can help make it clear if this is happening and potentially also illuminate why that might be happening.

That said, in https://phabricator.services.mozilla.com/D281256 I do see the PSerialManager is managed by PBackground, so as long as that actor exists and is not offset by an IPCWorkerRef, the existence of such an actor should prevent the worker from being GC'ed. (Specifically, we check if there are PBackground actors that exist as an ingredient in determining whether the worker can be GC'ed, although we do subtract off the count of all live IPCWorkerRefs with non-zero mozilla::dom::IPCWorkerRef::SetActorCount calls (and where the default when you create one is to set it to 1 for that IPCWorkerRef). There are docs on workerrefs but in a case like this where it looks like you are only communicating with other threads via IPC and you have a PBackground actor whose existence should inhibit GC, as long as you don't need to delay the worker thread shutdown, you should be fine with direct navigator hookups and/or GlobalTeardownObserver disconnect notifications (which also work on main-thread window globals and for other global types).

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

[Jens Stutte [:jstutte]]

Attachment: WIP: Bug 2014794 - Part 1: Prevent GC from collecting SerialPort with event listeners. r=#dom-device-reviewers

Follow the WebSocket pattern to prevent garbage collection from collecting
SerialPort objects that have connect/disconnect event listeners registered.
Without this, setting ondisconnect in a worker has no effect because the
port gets collected once JS drops its reference.

Comment 4

[Jens Stutte [:jstutte]]

This bug looked simple enough to give it a try but Claude and I encountered the following problem:

The keep-alive fix in part 1 (prevent GC from collecting SerialPort objects that have event listeners) applies equally to window and worker contexts. However, while investigating a test for this, I found that hotplug notifications (connect/disconnect events) are never delivered to workers at all. This is a separate, pre-existing issue.

The root cause: PSerialManager is managed by PWindowGlobal, and workers don't have a WindowGlobal. The current workaround is that workers borrow the window's SerialManagerChild actor via Serial::AdoptManagerChild(). This lets workers call getPorts() and open(), but SerialManagerChild::RecvPortConnected() and RecvPortDisconnected() (which both assert NS_IsMainThread()) only notify the window's Serial instance — the worker's Serial never hears about device changes.

This means that in a worker, port.ondisconnect and port.onconnect will never fire, and navigator.serial.ondisconnect / navigator.serial.onconnect will never fire either.

Other DOM APIs that need to deliver events to workers (BroadcastChannel, Cache, CookieStore, etc.) use PBackground, which is available on worker threads via BackgroundChild::GetOrCreateForCurrentThread(), allowing each worker to create its own IPC actor and receive notifications independently.

Possible fixes:

1. Move PSerialManager back to PBackground (it was there originally per the revision history) or make it dual-managed, so workers can
   create their own actor.
2. Keep the shared-actor approach but add an observer mechanism on SerialManagerChild to forward notifications to worker Serial instances. This works but is an unusual pattern — I prototyped it and it requires a mutex-protected observer list, permission-check forwarding, and careful lifecycle management.

Option 1 seems cleaner and more consistent with the rest of the codebase.

Leaving the WIP patch for the simple piece here, but I am probably not the right person to solve the rest.

Comment 5

[Jens Stutte [:jstutte]]

Was this already discussed as part of your patches' reviews ?

Comment 6

[Greg Stoll :gstoll]

Oof, hmm. The latest changes (that I just pushed to Phabricator) get rid of Serial::AdoptManagerChild() because the IPC actors now expect to only be on the main thread.

It seems like new usages of PBackground are discouraged so I guess we might have to explore option 2.

Thanks for looking at this!

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Please do not explore option 2 or anything that would involve bouncing runnables between the main thread and the worker thread instead of binding an IPC actor to the worker thread, it is a great way to create security bugs, crashers, and leaks, especially as it relates to worker shutdown. A great option is creating your own top-level protocol that can be constructed over PWindowGlobal and PBackground.

I think concerns around PBackground in general might relate to the following; I would be very interested to hear of any other motivating concerns you are aware of:

• PWindowGlobal is explicitly tied to a specific global on the main-thread which provides authentication (what principal is this?) and lifetime bound benefits, whereas PBackground on the main thread is not tied to a specific global and lives as long as the main thread, so PWindowGlobal is generally a better choice than PBackground on the main thread. PBackground for worker threads in fact will strictly be associated with one global and its lifetime, but we do not currently make it easy to rely on that, but it is much easier to fix this than it is to deal with the pain that the cross-thread runnables situation creates.
  • We could also create a PWorkerGlobal if that helps provide clarity and simplifies advice to people about what to do for new protocols. I'll ping :nika.
• The IPDL Background thread in the parent potentially experiences more contention than it really needs to. But if you use a top-level protocol, you can avoid this contention by binding your actors in the parent process to a TaskQueue on a thread-pool or on your own dedicated thread or whatever you want. (Or maybe even route the endpoint to a dedicated process for security reasons, etc.)

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Greg Stoll :gstoll from comment #6)

It seems like new usages of PBackground are discouraged so I guess we might have to explore option 2.

FWIW "Discouraged" is perhaps too strong of a word. I'd like us to eventually move away from using PBackground for things which don't benefit from PBackground, but something like WebSerial could very well benefit from just being on PBackground to lean into the existing infrastructure which exists for other worker APIs.

Right now the tools to fully move away from PBackground for new APIs simply don't exist, and I don't know if WebSerial is the place to start building these new tools.

Comment 9

[Greg Stoll :gstoll]

FWIW I have changed the SerialManager IPC actor to PBackground in the latest revision of the change and went ahead and fixed this problem and added a test. So I expect we can close this when bug 2010930 lands.

Comment 10

[Jens Stutte [:jstutte]]

(In reply to Greg Stoll :gstoll from comment #9)

FWIW I have changed the SerialManager IPC actor to PBackground in the latest revision of the change and went ahead and fixed this problem and added a test. So I expect we can close this when bug 2010930 lands.

I assume this means I can abandon the patch here?

Comment 11

[Greg Stoll :gstoll]

(In reply to Jens Stutte [:jstutte] from comment #10)

(In reply to Greg Stoll :gstoll from comment #9)

FWIW I have changed the SerialManager IPC actor to PBackground in the latest revision of the change and went ahead and fixed this problem and added a test. So I expect we can close this when bug 2010930 lands.

I assume this means I can abandon the patch here?

Yes, I think that makes sense for now. Thanks for the help!

Comment 12

[Greg Stoll :gstoll]

As we have a test for this now in bug 2010930, I'm going to close this as a duplicate.