Use document loadgroup for channel that initiates the ServiceWorker update job

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
mozilla41
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

4 years ago
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

Updated

4 years ago
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

Updated

4 years ago
Blocks: 1140658

Comment 2

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

Updated

4 years ago
Duplicate of this bug: 1119729

Updated

4 years ago
Blocks: 1056702
Assignee

Comment 4

4 years ago
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+
Assignee

Comment 6

4 years ago
(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?
Assignee

Comment 8

4 years ago
I suppose the patch in bug 1125961 will avoid the problem here as well.
Assignee

Comment 9

4 years ago
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)
Assignee

Comment 11

4 years ago
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)

Comment 12

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

Updated

4 years ago
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)

Comment 21

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.