w3c / ServiceWorker

Service Workers
https://w3c.github.io/ServiceWorker/
Other
3.63k stars 312 forks source link

Support `close()` method in ServiceWorkerGlobalScope #1696

Open oliverdunk opened 1 year ago

oliverdunk commented 1 year ago

close() is supported in the DedicatedWorkerGlobalScope (source), but not the ServiceWorkerGlobalScope.

In Manifest V3, the latest version of the Web Extensions platform, extensions can use service workers as a background context. In the Web Extensions Community Group, we have been discussing a feature request to support the immediate termination of a service worker.

We would prefer a spec level solution over an extension specific API so filing this issue.

There are a few key use cases we have identified:

I found https://github.com/w3c/ServiceWorker/issues/865 which has some of the history: it appears this initially threw an error, and was then moved to avoid exposing it entirely. But I couldn't find any specific motivations for not supporting it.

mkruisselbrink commented 1 year ago

I think one possible motivation is that close() would be a massive foot-gun. A service worker can be processing many different ExtendableEvents at the same time, and without manual bookkeeping there isn't really a way for a service worker script to know what events are in progress. If close() would immediately terminate the service worker all these events would get aborted or something, which probably is rarely the behavior intended. On the other hand if close() would wait for all extendable events to finish first it might not fulfill the stated goals/use cases either, as it could take arbitrarily long for that to be true. I suppose the same is somewhat true for dedicated/shared worker close(), although there the only thing that can trigger something to happen in the worker is an explicit postMessage by website code, while in service workers there are many external/user agent initiated things that can also trigger events etc.

asutherland commented 1 year ago

A service worker can be processing many different ExtendableEvents at the same time, and without manual bookkeeping there isn't really a way for a service worker script to know what events are in progress.

I'd go further than this. Because of the involvement of multiple threads and potentially multiple processes, the ServiceWorker agent thread inherently is not in a position to be able to reason about whether it's appropriate to self-terminate even with bookkeeping. Everything leading up to the task queued by step 6 of fire a functional event is outside of its awareness.

This would be further exacerbated by how close() is currently specified. Close a worker is specified to discard outstanding tasks and sets the closing flag which discards any future tasks. The worst part is that calling close() does not immediately abort execution of JS and potentially allows the worker to continue performing observable actions. (However, we have made improvements here, like BroadcastChannel's eligible for messaging check treats a worker in the zombie post-close-still-running-JS state as not eligible for messaging which is checked as the first step of postMessage() so the worker can no longer be observable over BroadcastChannel.)

That said, even if close() did immediately throw an uncatchable error after it set the closing flag, I still don't think it would be appropriate to expose on ServiceWorkers.

Use Cases

A developer is writing tests and wants to immediately terminate the service worker (https://github.com/w3c/webextensions/issues/311#issuecomment-1764675350).

This is an important use-case, but it seems like it is better addressed by adding a WebDriver extension command which could be used by sites for testing, and then also exposed to testdriver.js for use in any Web Platform Tests.

For example, I see that testdriver.js documents FedCM-specific capabilities which seems to be documented as a WebDriver extension in the FedCM CG draft report.

In general, a service worker no longer has any work to perform and wishes to free up resources.

It would be good to understand how the existing functional event mechanism and the ability of content to help out garbage collection are believed to be insufficient here. Especially as, to reiterate the top point, in general a ServiceWorker does not have the ability to reason about future events that might occur or what the cost to restarting the ServiceWorker would be.

A developer is handling sensitive data in the service worker (e.g a password manager). When the user asks the password manager to lock, immediately terminating the service worker would provide some additional certainty that any data in memory can be freed (https://github.com/w3c/webextensions/issues/311#issuecomment-1326648334).

I'm unclear if there's an assumption here that freed memory will not be available to an attacker with the ability to arbitrarily read the contents of the process' address space, but it seems like https://github.com/w3c/ServiceWorker/issues/1529 could provide more confidence about this anyways. (Although there's still a bunch of browser latitude in how to implement terminate(), it's strictly stronger than close().)

oliverdunk commented 1 year ago

Thanks both for sharing so much context. I honestly hadn't thought about the implications for event processing, which is clearly a major part of looking at what would be involved here.

A couple of general thoughts:

More specific thoughts:

This is an important use-case, but it seems like it is better addressed by adding a WebDriver extension command which could be used by sites for testing, and then also exposed to testdriver.js for use in any Web Platform Tests.

I generally agree and did mention this to them. In their case, they use WebDriver purely as a way to start the browser, and all of the test logic runs inside of the service worker (more similar to this sort of thing).

It would be good to understand how the existing functional event mechanism and the ability of content to help out garbage collection are believed to be insufficient here.

I think they are generally sufficient. Being able to terminate slightly early is nice but given the potential footguns I agree this is probably one of the weaker arguments.

I'm unclear if there's an assumption here that freed memory will not be available to an attacker with the ability to arbitrarily read the contents of the process' address space, but it seems like https://github.com/w3c/ServiceWorker/issues/1529 could provide more confidence about this anyways.

I don't think it's possible to make a statement quite that strong. I do still see value though. In particular, if your service worker is receiving a steady stream of events, it might never die. This means that if you have a bug and are accidentally keeping a reference to some sensitive data you wanted to clear, it may stay in a state where it is very easy to access forever. Being able to call close() immediately prevents the most basic attacks (looking at the memory tab in DevTools, for example) and gives you reasonable confidence that at some point the memory will be cleared. It's by no means something to rely on, but is a good last measure of defence (as a note, I wrote the initial WECG comment when I was at 1Password, but I no longer work there and this is purely from my own POV).