Bugzilla

I mischaracterized the scenario somewhat in triage, but the pragmatic solution that Nika proposed is the same.

What's happening:
- With the state update performing the write:
   - The ServiceWorker is being told by its owner (ServiceWorkerManager) to change its state.
   - This is done via a control runnable for unclear reasons I can only speculate about but where there's some risk to changing the behavior[1] and so shouldn't happen in a sec-bug.  This means we update the state even if we're in a syncloop.
   - Because the classic scriptloader is synchronous and uses a syncloop, it can be a bit cavalier about what it's accessing on WorkerPrivate *unless control runnables are involved*.
- With the read:
   - The script loader cares about the SW state because we have policies that forbid loading scripts that aren't cached after installation.  It is latching the value, but on the main thread, which is too late because of the control runnable above.  This would be "fine" if not for the control runnable in this case.

The fix:
- Latch the value on the worker thread.  The [spot where we create the WorkerLoadContext in WorkerModuleLoader::CreateDynamicImport](https://searchfox.org/mozilla-central/rev/d895cf273d8cdceb4256f561c1ad1bf91135202a/dom/workers/loader/WorkerModuleLoader.cpp#126-128) and are already snapshotting the ClientInfo would be the most consistent and reasonable spot.  We could even snapshot descriptor.

Does this raise other security-ish bugs?:
- Auditing [searchfox's experimental diagram of all control runnable subclasses](https://asuth.searchfox.org/mozilla-central/query/default?q=inheritance-diagram%3AWorkerControlRunnable+graph-layout%3Aneato), this is the only control runnable that is messing with state that could impact the scriptloader.  That said, there are some that manipulate JS engine parameters, but all the script loader JS script evaluation explicitly happens on the worker thread, so that's fine.
- Auditing a text query on [all uses of `path:dom/workers/loader Private()->`](https://asuth.searchfox.org/mozilla-central/search?path=&q=path%3Adom%2Fworkers%2Floader%20Private()-%3E) doesn't make me super happy but there's nothing else like the problem we're addressing.

1: A bunch of our APIs snapshot the descriptor from the WorkerLoadInfo that's being updated by the control runnable.  The major side effect from being a control runnable besides getting to run when there are syncloops on the stack is that control runnables get to run before normal runnables, so that descriptor gets updated very quickly.  This means that calls will staple an up-to-date descriptor much sooner than they would if this was a normal runnable.  That's not particularly helpful though, as the content code couldn't have known about this change, and having the state out of sync is not useful there.  And APIs like the Clients API can't depend on this "early update" side effect in any way because the control runnable doesn't eliminate async lag, just reduces the impact of contention for the worker.

I mischaracterized the scenario somewhat in triage, but the pragmatic solution that Nika proposed is the same.

What's happening:
- With the state update performing the write:
   - The ServiceWorker is being told by its owner (ServiceWorkerManager) to change its state.
   - This is done via a control runnable for unclear reasons I can only speculate about but where there's some risk to changing the behavior[1] and so shouldn't happen in a sec-bug.  This means we update the state even if we're in a syncloop.
   - Because the classic scriptloader is synchronous and uses a syncloop, it can be a bit cavalier about what it's accessing on WorkerPrivate *unless control runnables are involved*.
- With the read:
   - The script loader cares about the SW state because we have policies that forbid loading scripts that aren't cached after installation.  It is latching the value, but on the main thread, which is too late because of the control runnable above.  This would be "fine" if not for the control runnable in this case.

The fix:
- Latch the value on the worker thread.  The [spot where we create the WorkerLoadContext in WorkerModuleLoader::CreateDynamicImport](https://searchfox.org/mozilla-central/rev/d895cf273d8cdceb4256f561c1ad1bf91135202a/dom/workers/loader/WorkerModuleLoader.cpp#126-128) and are already snapshotting the ClientInfo would be the most consistent and reasonable spot.  We could even snapshot the whole descriptor.

Does this raise other security-ish bugs?:
- Auditing [searchfox's experimental diagram of all control runnable subclasses](https://asuth.searchfox.org/mozilla-central/query/default?q=inheritance-diagram%3AWorkerControlRunnable+graph-layout%3Aneato), this is the only control runnable that is messing with state that could impact the scriptloader.  That said, there are some that manipulate JS engine parameters, but all the script loader JS script evaluation explicitly happens on the worker thread, so that's fine.
- Auditing a text query on [all uses of `path:dom/workers/loader Private()->`](https://asuth.searchfox.org/mozilla-central/search?path=&q=path%3Adom%2Fworkers%2Floader%20Private()-%3E) doesn't make me super happy but there's nothing else like the problem we're addressing.

1: A bunch of our APIs snapshot the descriptor from the WorkerLoadInfo that's being updated by the control runnable.  The major side effect from being a control runnable besides getting to run when there are syncloops on the stack is that control runnables get to run before normal runnables, so that descriptor gets updated very quickly.  This means that calls will staple an up-to-date descriptor much sooner than they would if this was a normal runnable.  That's not particularly helpful though, as the content code couldn't have known about this change, and having the state out of sync is not useful there.  And APIs like the Clients API can't depend on this "early update" side effect in any way because the control runnable doesn't eliminate async lag, just reduces the impact of contention for the worker.