[go: nahoru, domu]

Skip to content
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

Open
oliverdunk opened this issue Oct 31, 2023 · 3 comments
Open

Support close() method in ServiceWorkerGlobalScope #1696

oliverdunk opened this issue Oct 31, 2023 · 3 comments

Comments

@oliverdunk
Copy link
Member

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:

  • 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 (source).
  • A developer is writing tests and wants to immediately terminate the service worker (source).
  • In general, a service worker no longer has any work to perform and wishes to free up resources.

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.

@mkruisselbrink
Copy link
Collaborator

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
Copy link

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 (w3c/webextensions#311 (comment)).

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 (w3c/webextensions#311 (comment)).

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().)

@oliverdunk
Copy link
Member Author

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:

  • Maybe requesting close() specifically implied a desire for re-using the worker implementation. I have no strong feelings there. I think this general high-level capability is interesting, but could see it making sense to have a different implementation and/or different method name.
  • While I agree that we would need to specify how this impacts event processing, I think that should be a solvable problem. To some extent, this is no difference to an event arriving while the browser is trying to terminate the service worker based on a timeout, right? We could figure out the right series of locks to make sure that once a close request arrives that must finish and any subsequent events will be handled by a new service worker spun up after this one closes. I acknowledge the potential footguns/it being hard for a developer to know if an event is currently being processed though.

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 #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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants