1432184 - clean up worker LoadGroup handling

Status: NEW

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we have a weird LoadGroup mechanism in worker code.  We try to use the parent's LoadGroup in general.  For things like SharedWorker which may be attached to different clients over time, though, we do a weird overriding mechanism.  We also use this mechanism for ServiceWorkers.

We should perhaps consider simply making workers have their own distinct load group.  The closing of the worker for normal reasons would trigger this load group to be canceled, etc.

Currently we need the parent's load group for a few reasons, though:

1. We need the docshell loadgroup in order for docshell to act as the nsINetworkInterceptController.  After bug 1231211 we will be able to make the worker do this instead.
2. The docshell loadgroup allows devtools to see the top window on any nsIChannel.associatedWindow() so it can be displayed on network monitor.

There may be other issues as well.

It would really clean things up, though, if we could have a single load group setup path for workers without the weird corner cases for different types.

Comment 1

[Marion Daly [:mdaly]]

:bkelley, going to triage as P2 as this seems like a good area for us to tackle next in terms of cleaning up. Let me know what you think.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

In some ways it would be easier to wait on this.  If we had e10s in place on all platforms some of the reasons to keep the loadgroup from the docshell would go away naturally.

In any case, I don't think its an immediate priority.

Comment 3

[Jens Stutte [:jstutte]]

:perry, did "some of the reasons" go away? Is there something left to do?

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

The change proposed in the bug description hasn't happened, but I am definitely lacking context on what "some of the reasons" refers to and worker loadgroups in general...

Comment 5

[Jens Stutte [:jstutte]]

Andrew, do you have some more context here?

Comment 6

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

(In reply to Jens Stutte [:jstutte] from comment #5)

Andrew, do you have some more context here?

I think this is actionable at this point, with most of the related concerns having been addressed. To comment 0's specific items:

1. Bug 1231211 landed and thanks to parent intercept the load group is very explicitly not involved in interception in any way at this point.
2. Devtools is now getting its information via explicit propagation of the BrowsingContextID through PFetch as introduced by Eden in https://phabricator.services.mozilla.com/D174249 as part of bug 1819570.

In terms of the mechanisms at play:

• We copy the document's load group by default in WorkerPrivate::GetLoadInfo when our parent is a window.
• But if we end up without a load group because the parent wasn't a window or a specific override policy is provided, WorkerPrivate::GetLoadInfo calls OverrideLoadInfoLoadGroup which creates a new LoadGroup. This is what happens for RemoteWorkers which means Sharedworkers and ServiceWorkers.
  • Note that we do specify the OverrideLoadGroup policy for our call to WorkerPrivate::GetLoadInfo in SharedWorker::Constructor but we don't actually use the LoadGroup directly when building RemoteWorkerData.

We could probably try specifying the OverrideLoadGroup policy flag for all cases and see what breaks. There's also just a ton of potential to clean up the WorkerLoadInfo logic which has just accumulated a vast amount of cruft as it's generally been append-only with changes stacking up in a series of hacks.