Closed Bug 1137683 Opened 5 years ago Closed 5 years ago

Use document loadgroup for channel that initiates the ServiceWorker update job

Categories

(Core :: DOM: Workers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(2 files, 2 obsolete files)

Right now we create a channel out of thin air with no loadgroup in UpdateServiceWorkerJob::ContinueUpdate. We should pass in the document loadgroup instead.
Assignee: nobody → josh
Blocks: 1125961
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.
Status: NEW → ASSIGNED
Blocks: 1140658
When this bug is fixed, this changeset should be reverted: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c20c565d00
Duplicate of this bug: 1119729
Blocks: 1056702
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.
Attachment #8590498 - Flags: review?(bkelly)
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.
Attachment #8590498 - Flags: review?(bkelly) → review+
(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.
(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?
I suppose the patch in bug 1125961 will avoid the problem here as well.
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.
What's the status of this bug?
Flags: needinfo?(josh)
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.
Flags: needinfo?(josh)
Please re-enable these tests <https://hg.mozilla.org/integration/mozilla-inbound/rev/1189f6ffc65e#l2.19> when this bug gets fixed.  Thanks!
Attachment #8590498 - Attachment is obsolete: true
Thanks, Dragana!  Ehsan mentioned trying to land this before so he can likely try again on Monday.
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/ae459c286310
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.