1118845 - SharedWorker should not directly use document load group

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Bug 1107516 added load group support to workers without a document, but it uses the wrong load group for SharedWorkers.  Currently it uses the load group from the document which first created the SharedWorker.  This leads to the following problem:

1) doc1 creates SharedWorker
2) SharedWorker has load group from doc1
3) doc2 attaches to SharedWorker
4) doc1 navigates away or is closed
5) doc1's load group is canceled

This leaves the SharedWorker still active with an invalid load group.

Talking with Jonas, we should instead always create a new load group for SharedWorkers.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add optional base context to LoadContext() constructor. (v0)

This patch adds back the optional base context to the LoadContext() constructor that was originally r+'d in bug 1107516 comment 24.  It was removed from that bug when later review comments made me believe it was not needed.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add optional base load group to NS_NewLoadGroup(). (v0)

This patch adds back the optional base load group argument to NS_NewLoadGroup() that was originally r+'d back in bug 1107516 attacment 8532667.  I unfortunately removed this argument later in the bug when it appeared it was no longer needed.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Force the creation of a new load group for SharedWorkers. (v0)

This forces the creation of a new load group for SharedWorkers and ServiceWorkers.  ServiceWorkers were previously getting a new group, though, because they don't have a document.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0454b9c8891e

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Note, bug 1107516 had not merged to any other branches yet, so backporting is not necessary here.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1fc46d4d56
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2bda66cf60fd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/41d0ad0271a6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d838801a16d

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5230126&repo=mozilla-inbound

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I need more time to investigate this.  I'll have to uplift any fix after the merge next week.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Also, note this in the log:

  WARNING: child tried to open http IPDL channel w/o security info

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I believe the problem here is that we are losing the nsITabChild callback on the LoadGroup.  I am only creating a similar nsILoadContext, but am not propagating any of the other callback info.

I think I need to build a worker-specific callback proxy object.  It will forward just the callbacks we care about to the original document's callbacks.  So we will get nsILoadContext and nsILoadGroup callbacks, but not other things like nsISelectionDisplay, etc.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make SharedWorker override parent LoadGroup with custom proxy callbacks. (v0)

Initial testing has confirmed the failures were due to missing the nsITabChild in the load group's callbacks.

Unfortunately, we can't just reference the first document's TabChild for the life of the SharedWorker.  The TabChild needs to be able to clean up when the browsing context goes away.  It also becomes partially destroyed after the IPC ActorDestroy().  So referencing the first document's TabChild would have the same problem as we have today with just using that document's load group.

The solution ended up being a custom nsIInterfaceRequestor for workers that holds a list of weak references to nsITabChild objects.  Weak references are used to allow the TabChild to clean up when it needs to.  When a document attaches to an existing SharedWorker, its TabChild is added to the list.  (I spoke with Ollie on IRC and he thought this sounded reasonable for SharedWorker.)

Right now ServiceWorkers will not reliably get an nsITabChild object, unfortunately.  This is a harder problem, so I wrote it up in bug 1125961.

This patch also handles canceling the load group when the worker goes away.  This is only done for "overriden" load groups using the custom nsIInterfaceRequestor callbacks.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove now-defunct NS_NewLoadGroup(result, nsIPrincipal) from nsNetUtil. (v0)

Patrick, I'm sorry, but the code you reviewed for me in bug 1107516 is no longer necessary.  I need to do a worker-specific callbacks object, so the NS_NewLoadGroup() doesn't really make sense.

I figure it would be best to remove unused code, but I'm ok leaving it in if you want.  This patch does the removal.

Sorry again for the churn!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66c7afcc48f7

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 interdiff 001 fix non-debug builds

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9019706b55be

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8555467 [details] [diff] [review]
P3 Remove now-defunct NS_NewLoadGroup(result, nsIPrincipal) from nsNetUtil. (v0)

Review of attachment 8555467 [details] [diff] [review]:
-----------------------------------------------------------------

I think this sets a useful precedent as a primitive - lets leave it in. Thanks for following up!

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8555462 [details] [diff] [review]
P2 Make SharedWorker override parent LoadGroup with custom proxy callbacks. (v0)

Review of attachment 8555462 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments addressed.

::: dom/workers/WorkerPrivate.cpp
@@ +2008,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(aPrincipal);
> +
> +    // Look for an existing LoadContext.  This is optional and ok if

it's ok

@@ +2081,5 @@
> +private:
> +  ~InterfaceRequestor() { }
> +
> +  already_AddRefed<nsITabChild>
> +  GetTabChild()

This should be named something like GetAnyLiveTabChild.

@@ +2097,5 @@
> +      }
> +
> +      // Otherwise remove the stale weak reference and check the next one
> +
> +      mTabChildList.RemoveElementAt(mTabChildList.Length() - 1);

nit, remove the \n after the comment

@@ +2110,5 @@
> +  // actors alive for long after their ActorDestroy() methods are called.
> +  nsTArray<nsWeakPtr> mTabChildList;
> +
> +private:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Move this to the top of the class (and ditch the private).

This also should not be threadsafe.  It is probably unnecessary, and the implementation is not threadsafe (nsSupportsWeakReference is not threadsafe, so destroying an array of them off the main thread won't be).  If this does need to be threadsafe the assertions will catch it and we can figure out what to do later.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make SharedWorker override parent LoadGroup with custom proxy callbacks. (v1)

Address review feedback.  I'm going to do another full try (once the trees are open) before landing.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8109c804efb

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Fix non-debug build again:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0180488cd45b

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d00c2de1db0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3df9f4282a65

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d00c2de1db0
https://hg.mozilla.org/mozilla-central/rev/3df9f4282a65