-
Notifications
You must be signed in to change notification settings - Fork 313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support close()
method in ServiceWorkerGlobalScope
#1696
Comments
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'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
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.
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.
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 #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().) |
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:
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).
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 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). |
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 #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.
The text was updated successfully, but these errors were encountered: