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

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add the ServiceWorker controller to ParentLoadInfoForwarderArgs. r=mayhemer

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove explicit ServiceWorker controller from http OnStartRequest message. r=mayhemer

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Forward reserved client, initial client, and controller on Redirect2Verify message back to parent. r=mayhemer

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 9

[Honza Bambas (:mayhemer)]

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 ?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add the ServiceWorker controller to ParentLoadInfoForwarderArgs. r=mayhemer

Rebase.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Forward reserved client, initial client, and controller on Redirect2Verify message back to parent. r=mayhemer

Updated to rename the variable.  Also, I fixed a silly typo that caused try to blow up.

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Treat same-value assignments to the LoadInfo reserved and initial ClientInfo values as no-op changes. r=mayhemer

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove explicit ServiceWorker controller from http OnStartRequest message. r=mayhemer

Rebase on std::move().  Also fix a bug where I was deleting the propagating of the redirect count by accident.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/fcc1e1ae65dd
https://hg.mozilla.org/mozilla-central/rev/503612a33767
https://hg.mozilla.org/mozilla-central/rev/de52f4e67dac
https://hg.mozilla.org/mozilla-central/rev/55a9e16be960