1758125 - WorkerPrivate's LoadInfo member isn't initialized consistently

Status: NEW

(Core :: DOM: Workers, defect, P3)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

After staring and WorkerPrivate.mLoadInfo for a bit I'm convinced this design is a recipe for bugs. (Because, well, I started staring at it because of a bug.)

My bug, specifically, was that the mShouldResistFingerprinting member wasn't correctly populated when running the browser_navigator test.

mShouldRFP is populated in WorkerPrivate::GetLoadInfo which seems like it should be fine - but really GetLoadInfo is not called in several codepaths of worker creation. While I could fairly easily initialize mShouldRFP in those other codepaths, the design of requiring members to be initialized inidividually in several different places is very non-obvious.

Specifically I found that if WorkerPrivate::Constructor's loadInfo argument is non-null; we'll skip the GetLoadInfo call. It is non-null in ServiceWorkerPrivate::SpawnWorkerIfNeeded and RemoteWorkerChild::ExecWorkerOnMainThread (the latter was the case I was hitting.)

I think we should improve this design to make it more obvious, although I'm not sure exactly how. It can think of two possibilities, but before I started trying to hack on either one I wanted to try to get some feedback...

1. pass in a null loadinfo in all cases, and see what is needed in the ServiceWorkerPrivate::SpawnWorkerIfNeeded and RemoteWorkerChild::ExecWorkerOnMainThread cases to initialize the LoadInfo in WorkerPrivate::GetLoadInfo (e.g. do we need to send in more ctor arguments? Maybe make a tiny-loadinfo we pass in that gets merged into the new one?)
2. write some sort of 'merge' function for LoadInfo if it is passed in. I'm not sure how this would go (would the provided loadinfo and WorkerPrivate::GetLoadInfo have different ideas of what the values should be? How do you know booleans in if the provided loadinfo are true values or should be overridden with other ones?)

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Andrew, do you have any opinions on this, and thoughts on what I could experiment with here?

Comment 2

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

The existing situation is definitely sub-optimal. (That said, it's getting slightly improved with Eden's ongoing removal of more child-intercept code which should slightly reduce the static control flow permutations. For example, ServiceWorkerPrivate::SpawnWorkerIfNeeded that you linked is actually dead code, although at this moment searchfox seems to lack coverage data so that's not clear.)

One nice thing that we've been doing in the workers/ServiceWorkers space is moving things to operate more directly in terms of spec abstractions. That is, for example, FetchEvents use IPCInternalRequest and the respondWith response uses Response. It would be nice if worker creation could more directly correspond to the spec, ex: HTML's set up a worker environment settings object and ServiceWorkers's Run Service Worker step 7 and its related dependencies.

Right now we characterize the state of SharedWorkers and ServiceWorkers (both of which are launched as RemoteWorkers) via RemoteWorkerData. It's under-documented and otherwise evolutionarily hacky and doesn't directly correspond to spec stuff, but arguably if we could clean it up to more directly match the spec, that would be amazing. There would need to potentially be some kind of helper wrappers since there are various edge-cases around off-main-thread creation (re: bug 1443925) where maybe we'd want to retain nsIPrincipal references rather than going through a PrincipalInfo in order to re-get an nsIPrincipal, but this seems do-able.

So then the idea is that normal dedicated worker creation would effectively build up the same data-structure that sorta directly maps to RemoteWorkerData and ideally that could more directly resemble the spec representations for things. And for things that are related to tracking-protection or other mitigations that don't (yet) correspond to specs, those could be clustered in a separate section. Even better if they could, for example, be rolled up in like CookieJarSettings which is already on RemoteWorkerData. (nsILoadInfo is fairly soupy in nature, whereas data structures with some greater level of hierarchy would be nice.)

edit: note that some things are of course specific to RemoteWorkerData for process selection purposes/etc, so it'd be more like RemoteWorkerData would include WorkerEnvironmentSettings inside it or some other new data structure .