Closed Bug 1441932 Opened 2 years ago Closed Last year

Decide on how exactly to copy Service Worker Controller from parent to child

Categories

(Core :: DOM: Service Workers, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: mayhemer, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Ben Kelly onIRC:

1) make sure controller is not copied from parent-to-child when the pref is off
2) we could fix the parent to clear the controller properly so we don't have the problem where it overwrites the child state incorrectly

the option (1) is currently in the tree.  landing bug 1438935 will NOT change that code either.

this bug is filed to consider if we need anything better/safer here.
Summary: Decode on how exactly to copy Service Worker Controller from parent to child → Decide on how exactly to copy Service Worker Controller from parent to child
Priority: -- → P2
I think the solution here is to always copy the controller from parent back to child.  The controller should be cleared in the parent when its cleared in the child.  To do that we should make sure any changes to the LoadInfo in the child redirect callbacks is propagated to the parent in SendRedirect2Verify():

https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/netwerk/protocol/http/HttpChannelChild.cpp#2346
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Blocks: 1462460
No longer blocks: ServiceWorkers-e10s
Comment on attachment 8982010 [details] [diff] [review]
P1 Add the ServiceWorker controller to ParentLoadInfoForwarderArgs. r=mayhemer

Honza, this patch adds the controller into the ParentLoadInfoForwarderArgs you previously created.  This is the best solution because we do need the controller sent in all the cases where you are sending the forwarder now (start, redirect, etc).

Later patches in this bug will fix the issue of the parent side holding a stale controller which caused problems before.
Attachment #8982010 - Flags: review?(honzab.moz)
Comment on attachment 8982011 [details] [diff] [review]
P2 Remove explicit ServiceWorker controller from http OnStartRequest message. r=mayhemer

This removes the old code I had for explicitly passing the controller in OnStartRequest.  I got rid of some extra assertions that I don't think are necessary any more.
Attachment #8982011 - Flags: review?(honzab.moz)
Comment on attachment 8982012 [details] [diff] [review]
P3 Forward reserved client, initial client, and controller on Redirect2Verify message back to parent. r=mayhemer

This patch adds a new ChildLoadInfoForwarderArgs.  Its very similar to the Parent* type, but sends LoadInfo deltas from child-to-parent.  In this case we send them on each redirect verification step.

To start the ChildLoadInfoForwarderArgs contains the values that ClientChannelHelper might change in its redirect verification handler; reserved client, initial client, and controller.

This solves the problem of the parent-side getting confused about the current controller during redirection by letting the child-side ClientChannelHelper update the information in the parent correctly.
Attachment #8982012 - Flags: review?(honzab.moz)
Try build with parent intercept pref off to ensure this doesn't break anything in our shipping config:

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

It greatly improves tests with the pref flipped as well, but there are other fixes that need to be done before that config is truly functional.
Attachment #8982010 - Flags: review?(honzab.moz) → review+
Attachment #8982011 - Flags: review?(honzab.moz) → review+
Comment on attachment 8982012 [details] [diff] [review]
P3 Forward reserved client, initial client, and controller on Redirect2Verify message back to parent. r=mayhemer

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +945,5 @@
>        if (appCacheChannel) {
>          appCacheChannel->SetChooseApplicationCache(aChooseAppcache);
>        }
> +
> +      nsCOMPtr<nsILoadInfo> newLoadInfo;

nit: could this be called newChannelLoadInfo ?
Attachment #8982012 - Flags: review?(honzab.moz) → review+
Attachment #8982257 - Flags: review+
Attachment #8982258 - Flags: review+
Comment on attachment 8982294 [details] [diff] [review]
P4 Treat same-value assignments to the LoadInfo reserved and initial ClientInfo values as no-op changes. r=mayhemer

So a side effect of the P3 patch is we end up setting the same reserved/initial ClientInfo on the parent side in some cases.  This runs afoul of the Maybe<> assertion that calling emplace() only sets a value on an empty Maybe<>.

This patch makes it a no-op to set a reserved or initial ClientInfo on a LoadInfo if the value is the same as the one already set.  It lets us avoid the Maybe<> emplace assertion, but still ensures we don't overwrite a different ClientInfo value unexpectedly.
Attachment #8982294 - Flags: review?(honzab.moz)
Attachment #8982294 - Flags: review?(honzab.moz) → review+
Rebase on std::move().  Also fix a bug where I was deleting the propagating of the redirect count by accident.
Attachment #8982011 - Attachment is obsolete: true
Attachment #8983094 - Flags: review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc1e1ae65dd
P1 Add the ServiceWorker controller to ParentLoadInfoForwarderArgs. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/503612a33767
P2 Remove explicit ServiceWorker controller from http OnStartRequest message. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/de52f4e67dac
P3 Forward reserved client, initial client, and controller on Redirect2Verify message back to parent. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a9e16be960
P4 Treat same-value assignments to the LoadInfo reserved and initial ClientInfo values as no-op changes. r=mayhemer
Depends on: 1470114
You need to log in before you can comment on or make changes to this bug.