1137683 - Use document loadgroup for channel that initiates the ServiceWorker update job

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Josh Matthews [:jdm]]

Right now we create a channel out of thin air with no loadgroup in UpdateServiceWorkerJob::ContinueUpdate. We should pass in the document loadgroup instead.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This is not quite right.  You don't want the ServiceWorker LoadGroup to be canceled if the document goes away.

You probably want to create a LoadGroup from the document LoadGroup similar to what SharedWorker does.

Comment 2

[(no longer active)]

When this bug is fixed, this changeset should be reverted: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c20c565d00

Comment 4

[Josh Matthews [:jdm]]

Attachment: Use a loadgroup derived from the document's when updating a ServiceWorker

This makes test_scopes.html pass when run with --e10s and network.disable.ipc.security disabled. We could explicitly change that pref in the test to ensure future test coverage, if you wanted.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8590498 [details] [diff] [review]
Use a loadgroup derived from the document's when updating a ServiceWorker

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

r=me with comments addressed.  In particular, please confirm that nsITabChild is adequate here and we don't need your new ServiceWorker replacement.  Thanks!

::: dom/workers/ServiceWorkerManager.cpp
@@ +540,5 @@
>      , mScope(aScope)
>      , mScriptSpec(aScriptSpec)
>      , mCallback(aCallback)
>      , mPrincipal(aPrincipal)
> +    , mLoadGroup(aLoadGroup)

I guess we don't do it for other pointers here, but it seems nice to MOZ_ASSERT(mLoadGroup) to fast fail.

@@ +1000,5 @@
>  
> +  nsCOMPtr<nsILoadGroup> docLoadGroup = doc->GetDocumentLoadGroup();
> +  nsRefPtr<WorkerLoadInfo::InterfaceRequestor> ir =
> +      new WorkerLoadInfo::InterfaceRequestor(documentPrincipal, docLoadGroup);
> +  ir->MaybeAddTabChild(docLoadGroup);

Will this actually work if the document navigates away or is GC'd?  It seems the nsITabChild will go away and the update network call will fail security checks.  Don't we need your new ServiceWorker specific type?

@@ +1007,5 @@
> +  // This allows checks for interfaces like nsILoadContext to yield the values used by the
> +  // the document, yet will not cancel the update job if the document's load group is cancelled.
> +  nsCOMPtr<nsILoadGroup> loadGroup = do_CreateInstance(NS_LOADGROUP_CONTRACTID);
> +  loadGroup->SetNotificationCallbacks(ir);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));

It doesn't seem rv is actually set here...  Were you trying to test the result of SetNotificationCallbacks?

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +71,2 @@
>    {
>      MOZ_ASSERT(aPrincipal);

MOZ_ASSERT(aLoadGroup)

@@ +81,5 @@
>      rv = NS_NewChannel(getter_AddRefs(mChannel),
>                         uri, aPrincipal,
>                         nsILoadInfo::SEC_NORMAL,
> +                       nsIContentPolicy::TYPE_SCRIPT,
> +                       aLoadGroup); // FIXME(nsm): TYPE_SERVICEWORKER

nit: Seems this comment is on the wrong line now.

Comment 6

[Josh Matthews [:jdm]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> @@ +1000,5 @@
> >  
> > +  nsCOMPtr<nsILoadGroup> docLoadGroup = doc->GetDocumentLoadGroup();
> > +  nsRefPtr<WorkerLoadInfo::InterfaceRequestor> ir =
> > +      new WorkerLoadInfo::InterfaceRequestor(documentPrincipal, docLoadGroup);
> > +  ir->MaybeAddTabChild(docLoadGroup);
> 
> Will this actually work if the document navigates away or is GC'd?  It seems
> the nsITabChild will go away and the update network call will fail security
> checks.  Don't we need your new ServiceWorker specific type?

I believe you're thinking of bug 1125961. This is only for the initial request to fetch the ServiceWorker script; is there a danger of navigating away before the channel is opened? That's the only time this is important, afaik.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #6)
> I believe you're thinking of bug 1125961. This is only for the initial
> request to fetch the ServiceWorker script; is there a danger of navigating
> away before the channel is opened? That's the only time this is important,
> afaik.

Well, you're explicitly creating a new LoadGroup to avoid the document load group from getting canceled.  Isn't that the same scenario?

Comment 8

[Josh Matthews [:jdm]]

I suppose the patch in bug 1125961 will avoid the problem here as well.

Comment 9

[Josh Matthews [:jdm]]

No, that's a lie, since it doesn't have access to any load context. I believe this patch will work in concert with bug 1125961, since the load context won't disappear if the page navigates, and we can deal with missing nsITabChild in that case.

Comment 10

[Andrew Overholt [:overholt]]

What's the status of this bug?

Comment 11

[Josh Matthews [:jdm]]

It's basically ready, but I would like to hold off on further action until the state of bug 1125961 is clear since there's some overlap in their behaviour.

Comment 12

[(no longer active)]

Please re-enable these tests <https://hg.mozilla.org/integration/mozilla-inbound/rev/1189f6ffc65e#l2.19> when this bug gets fixed.  Thanks!

Comment 13

[Josh Matthews [:jdm]]

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

Comment 14

[Josh Matthews [:jdm]]

Attachment: Use a loadgroup derived from the document's when updating a ServiceWorker

Rebase.

Comment 15

[Josh Matthews [:jdm]]

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

Comment 16

[Dragana Damjanovic [:dragana]]

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

Comment 17

[Dragana Damjanovic [:dragana]]

Attachment: swupdateloadgroup rebased

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: swupdateloadgroup rebased and fixed the initialization order problem

Comment 19

[Dragana Damjanovic [:dragana]]

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

Comment 20

[Andrew Overholt [:overholt]]

Thanks, Dragana!  Ehsan mentioned trying to land this before so he can likely try again on Monday.

Comment 21

[(no longer active)]

(In reply to Andrew Overholt [:overholt] from comment #20)
> Thanks, Dragana!  Ehsan mentioned trying to land this before so he can
> likely try again on Monday.

That was bug 1125961...  Not sure if this is a dependency for that or not, but I can try landing this one.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae459c286310

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae459c286310