1425975 - use ClientHandle objects to track controlled clients in ServiceWorkerManager

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

A big part of removing nsIDocument from SWM is replacing the stored nsIDocument references for controlled clients.  Instead we will store ClientHandle references and use a new API to know when the ClientHandle is destroyed.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerManager mControlledClients to track controlled ClientHandle references. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerManager mControlledClients to track controlled ClientHandle references. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Not done here, but less see if the ClientHandle::OnDetach() promise causes anything to blow up:

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Refactor ServiceWorkerManager::GetDocumentRegistration() to GetClientRegistration(). r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ServiceWorkerManager::UpdateClientControllers use mControlledClients. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make ServiceWorkerManager::RemoveRegistration assert there is no controlled document. r=asuth

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Making good progress on this, but need to explore the best solution for a few problems:

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Rename some service worker methods to not reference documents. r=asuth

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Fix unregister-then-register-new-script.https.html to not race iframe.remove() and expect resurrection on failed scripts. r=asuth

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: hmm

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938113 [details] [diff] [review]
P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth

While I'm still finalizing the final patches in the queue, I'm pretty confident with the first few patches.  So I'm going to flag now.

This patch exposes a ClientHandle::OnDetach() method that returns a MozPromise that resolves when the handle shutdowns due to de-ref or IPC actor destruction.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8937904 [details] [diff] [review]
P2 Add ServiceWorkerManager mControlledClients to track controlled ClientHandle references. r=asuth

This patch adds a separate hashtable to store ClientHandle/ServiceWorkerRegistrationInfo pairs.  Its keyed by the ClientInfo identifier.  The goal is to ultimately replace all uses of mControlledDocuments with this new mControlledClients.  Initially there will be some overlap.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938032 [details] [diff] [review]
P3 Refactor ServiceWorkerManager::GetDocumentRegistration() to GetClientRegistration(). r=asuth

This patch refactors GetDocumentRegistration() to GetClientRegistration() replacing nsIDocument code with client-based code.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938033 [details] [diff] [review]
P4 Make ServiceWorkerManager::UpdateClientControllers use mControlledClients. r=asuth

Refactor UpdateClientControllers to use mControlledClients instead of mControlledDocuments.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938034 [details] [diff] [review]
P5 Make ServiceWorkerManager::RemoveRegistration assert there is no controlled document. r=asuth

Strengthen the assertion in RemoveScopeAndRegistration() that touches mControlledDocuments.  This is in preparation of later removing it.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938225 [details] [diff] [review]
P6 Rename some service worker methods to not reference documents. r=asuth

This patch just renames some methods and members that had the word "Document" in them.  Instead they will now say "Client".

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938227 [details] [diff] [review]
P8 Fix unregister-then-register-new-script.https.html to not race iframe.remove() and expect resurrection on failed scripts. r=asuth

This fixes a bad WPT test.  It was incorrectly trying to assert that registering a 404 or reject-on-install worker would not resurrect an uninstalling registration.  There is no such spec language, AFAICT.  See step 5.1 here:

https://w3c.github.io/ServiceWorker/#register-algorithm

It clears the flag before running the update algorithm with does the fetching and install event dispatch.

The reason this mostly passed before was because it called frame.remove() on a controlled frame at the same time it did the register().  So it raced the register against noticing that the frame was removed and letting the old SW go away.  With our new IPC-based code we begin losing this race.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

I'm going to wait to flag for P7 until I can split up my final work-in-progress patch.  I think some of it could be reasonably merged with it.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Refactor MaybeCheckNavigationUpdate() to take a ClientInfo instead of a document. r=asuth

With the work-in-progress patch and this the only remaining uses of mControlledDocuments is for logging.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938237 [details] [diff] [review]
P9 Refactor MaybeCheckNavigationUpdate() to take a ClientInfo instead of a document. r=asuth

This is a simple refactor from nsIDocument to ClientInfo using mControlledClients.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Fix the test_skip_waiting.html mochitest to properly wait for active worker state. r=asuth

Andrew, this patch fixes a "wait for active worker" bug in test_skip_waiting.html.  It was trying to use .ready in the window being opened for that, but was not waiting for an active worker before opening the window.  So it never got controlled in the first place.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8938113 [details] [diff] [review]
P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth

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

::: dom/clients/manager/ClientHandle.h
@@ +94,5 @@
>    RefPtr<GenericPromise>
>    PostMessage(ipc::StructuredCloneData& aData,
>                const ServiceWorkerDescriptor& aSource);
>  
> +  // Return a Promise the resolves when the ClientHandle object is detached

phrasing nit: s/the resolves/that resolves/

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Fix test_workerupdatefound.html not to frame loading against SW activation and updatefound events. r=asuth

Here is another fun mochitest fix.  This test_workerupdatefound.html test was doing something very weird:

1. It registered a service worker, but did not wait for activation.
2. The service worker blocked its activate event on an updatefound event and required a controlled frame to present.
3. Opened a frame that it expected in (2).

In order for this to work it required some precise timing to line up.  The initial updatefound event needed to fire during the activation event handler, which is not guaranteed.  The activation event also needed to fire before the frame loaded, which was not guaranteed.

The fix here is to move the updatefound handler out of the activate event handler.  Also, we make sure the worker is activate before creating the frame.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

The json viewer failures are interesting.  Apparently the json viewer ends up with a null principal and we shouldn't allow storage.  But I think we want to be able to view json coming out of a service worker.

I guess I could allow the initial fetch event, but then revoke the controller after the fact.  I'll investigate that this afternoon.

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

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: hmm

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

Comment 29

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8937904 [details] [diff] [review]
P2 Add ServiceWorkerManager mControlledClients to track controlled ClientHandle references. r=asuth

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +331,5 @@
> +
> +    RefPtr<ServiceWorkerManager> self(this);
> +    clientHandle->OnDetach()->Then(
> +      SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
> +      [self = Move(self), aClientInfo] {

restating for my own sanity: aClientInfo is captured by-copy, the fact that the argument aClientInfo is a reference has no bearing on this.

Comment 30

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8938034 [details] [diff] [review]
P5 Make ServiceWorkerManager::RemoveRegistration assert there is no controlled document. r=asuth

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2308,5 @@
>      entry.Data()->Cancel();
>      entry.Remove();
>    }
>  
> +  // Verify there are no controlled documents for the purged registration.

Can you add a comment explaining why we think there won't be any controlled documents for the purged registration?

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8938225 [details] [diff] [review]
P6 Rename some service worker methods to not reference documents. r=asuth

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

::: dom/workers/ServiceWorkerRegistrationInfo.cpp
@@ +98,1 @@
>      NS_WARNING("ServiceWorkerRegistrationInfo is still controlling documents. This can be a bug or a leak in ServiceWorker API or in any other API that takes the document alive.");

nit: "still controlling documents".  No need to address here, but could be confusing later on when the workers thing happens.  Ignore if you correct later in the stack.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Don't mark an initial about:blank client as controlled if its sandboxed. r=asuth

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Assert that storage is allowed when a ClientSource is both execution ready and controlled. r=asuth

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

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth

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

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938501 [details] [diff] [review]
P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth

Ok, I ended up folding a lot of my "hmm" patch into P7 since it made sense here.

This patch moves the lifecycle management of registrations over into the mControlledClients world.  This is mainly significant in that we need to know when all of the controlled clients are gone so we can move workers to redundant and activate the next worker.

As part of doing this I exposed a new nsIServiceWorkerManager.StartControlling() method that takes ClientInfo and ServiceWorkerDescriptor.  This is called by docshell when it controls an initial about:blank ClientSource and ensures we track these clients as ClientHandle objects.

We also get ClientHandle objects into mControlledClients via ServiceWorkerManager::DispatchFetchEvent().  This is the same point where we mark the LoadInfo as controlled.

In the future the docshell StartControlling() call will have to become IPC enabled.  The internal DispatchFetchEvent() call to StartControllingClient() will in theory happen on the parent process already.

I also fixed up the "assert no more controlled document/clients" assertions a bit.  I incorrectly have an #ifdef around them that prevented the iter.Remove() from firing in release.  Also, we only assert the mControlledClients case now since that is what the lifecycle is based on.  Its possible documents may outlive the ClientHandle::OnDetach() promise in some race conditions.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938502 [details] [diff] [review]
P12 Don't mark an initial about:blank client as controlled if its sandboxed. r=asuth

You may have noticed the P7 patch removed an assertion that we don't control documents that don't have storage access.  P14 will add that assertion back in ClientSource, but first we need to fix some cases that make the new ClientSource assertions fail.

The first is that we should not mark initial about:blank clients as controlled if they are sandboxed.  While the nsDocShell::MaybeCreateInitialClientSource() checks for sandbox in the "no principal" case, we still need to check it again later when we have a principal and a controller.

Also, this changes MaybeCreateInitialClientSource() to use the more generous check for any sandbox flags.  This matches ShouldPrepareForIntercept().

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938508 [details] [diff] [review]
P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth

Another case that made us fail the storage access assertions in ClientSource is when the final document principal was changed to the null principal when viewing json.  This happens because the streamconv for json uses nsIChannel.owner to override the principal.

The ClientChannelHelper works hard to detect cross-origin network requests and redirects, but it has no way to tell when nsIChannel.owner or similar overrides are set.

To address the problem this patch double-checks the final document principal against the ClientSource.  If they don't match then it resets the ClientSource.  

In these cases its also important not to mark the new ClientSource controlled even though the nsIChannel may have been intercepted.  From the service worker's perspective its like there was a client there briefly, but it quickly navigated to a new origin.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938504 [details] [diff] [review]
P14 Assert that storage is allowed when a ClientSource is both execution ready and controlled. r=asuth

Finally, add assertions in ClientSource that we never have a Client that is both execution ready and controlled.  The execution ready requirement is simply there to ensure we have enough state to determine our storage policy correctly.

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8938377 [details] [diff] [review]
P10 Fix the test_skip_waiting.html mochitest to properly wait for active worker state. r=asuth

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

Restating: waitForActivated() was actually waiting for the "skip_waiting_scope/index.html"'s ready promise which would be fulfilled when start()'s registration of worker.js reached "active", but would not claim existing clients, so a race was possible where the iframe would never be controlled if the iframe's navigation event was serviced prior to the SW reaching activation.  This would manifest as a failure of checkWhetherItSkippedWaiting because the iframe could not experience a controllerchange if not controlled.  So now we wait for activation.  We also leave around the READY-postMessaging code, because maybe some other code uses it.

Comment 42

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8938397 [details] [diff] [review]
P11 Fix test_workerupdatefound.html not to frame loading against SW activation and updatefound events. r=asuth

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

I assume the intent was to wrap the async updatefound handling in an ExtendableEvent for maximum correctness.

Comment 43

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8938508 [details] [diff] [review]
P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth

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

Thanks for the excellent comments!

::: dom/base/nsGlobalWindowInner.cpp
@@ +1839,5 @@
> +  //       marked the LoadInfo controlled and it won't know about them.  Its
> +  //       also possible we are creating the client late due to the final
> +  //       principal changing and these clients should definitely not be
> +  //       controlled by a service worker with a different principal.
> +  else if (loadInfo) {

style/readability expectation violation: move the preceding } down to this 'else if' line.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #43)
> ::: dom/base/nsGlobalWindowInner.cpp
> @@ +1839,5 @@
> > +  //       marked the LoadInfo controlled and it won't know about them.  Its
> > +  //       also possible we are creating the client late due to the final
> > +  //       principal changing and these clients should definitely not be
> > +  //       controlled by a service worker with a different principal.
> > +  else if (loadInfo) {
> 
> style/readability expectation violation: move the preceding } down to this
> 'else if' line.

Actually, I've gotten conflicting review feedback on this in the past.  I used to do what you recommend, but people complained the comment was "in the last block".  Putting the else on a separate line when i want to use a block comment for it seemed the best compromise to me.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #42)
> I assume the intent was to wrap the async updatefound handling in an
> ExtendableEvent for maximum correctness.

Actually, the ergonomics of `updatefound` in a service worker context are quite poor.  I filed a spec issue:

https://github.com/w3c/ServiceWorker/issues/1255

For now I intend to leave the test as is.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #30)
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +2308,5 @@
> >      entry.Data()->Cancel();
> >      entry.Remove();
> >    }
> >  
> > +  // Verify there are no controlled documents for the purged registration.
> 
> Can you add a comment explaining why we think there won't be any controlled
> documents for the purged registration?

I think I added a comment somewhat describing this in P7.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #31)
> ::: dom/workers/ServiceWorkerRegistrationInfo.cpp
> @@ +98,1 @@
> >      NS_WARNING("ServiceWorkerRegistrationInfo is still controlling documents. This can be a bug or a leak in ServiceWorker API or in any other API that takes the document alive.");
> 
> nit: "still controlling documents".  No need to address here, but could be
> confusing later on when the workers thing happens.  Ignore if you correct
> later in the stack.

I'm actually just going to change this to a diagnostic assertion if I can.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8938508 [details] [diff] [review]
P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth

Andrew gave me verbal r+ for this over IRC.  He meant to flip the flag.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Rename some service worker methods to not reference documents. r=asuth

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

One last try since I added another assertion:

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

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

I need to fix test_workerupdatefound.html on non-e10s it appears.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #53)
> I need to fix test_workerupdatefound.html on non-e10s it appears.

I can't reproduce this locally and it does not trigger frequently in my try build.  I'm going to leave it as is for now and file a follow-up.  See bug 1426905.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ae11b5bce8
P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/acf4d61babab
P2 Add ServiceWorkerManager mControlledClients to track controlled ClientHandle references. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/553628a56e6a
P3 Refactor ServiceWorkerManager::GetDocumentRegistration() to GetClientRegistration(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/df13eba47970
P4 Make ServiceWorkerManager::UpdateClientControllers use mControlledClients. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/82d39ed8c831
P5 Make ServiceWorkerManager::RemoveRegistration assert there is no controlled document. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f091f5e182c4
P6 Rename some service worker methods to not reference documents. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f520ab76d6a
P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce186c6d8f5
P8 Fix unregister-then-register-new-script.https.html to not race iframe.remove() and expect resurrection on failed scripts. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b451ce55d4
P9 Refactor MaybeCheckNavigationUpdate() to take a ClientInfo instead of a document. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb89dbd922ba
P10 Fix the test_skip_waiting.html mochitest to properly wait for active worker state. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e50d9d1d069
P11 Fix test_workerupdatefound.html not to frame loading against SW activation and updatefound events. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1544ec814d
P12 Don't mark an initial about:blank client as controlled if its sandboxed. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e657fa97b71
P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f4a2d1df9a
P14 Assert that storage is allowed when a ClientSource is both execution ready and controlled. r=asuth

Comment 56

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b7ae11b5bce8
https://hg.mozilla.org/mozilla-central/rev/acf4d61babab
https://hg.mozilla.org/mozilla-central/rev/553628a56e6a
https://hg.mozilla.org/mozilla-central/rev/df13eba47970
https://hg.mozilla.org/mozilla-central/rev/82d39ed8c831
https://hg.mozilla.org/mozilla-central/rev/f091f5e182c4
https://hg.mozilla.org/mozilla-central/rev/6f520ab76d6a
https://hg.mozilla.org/mozilla-central/rev/4ce186c6d8f5
https://hg.mozilla.org/mozilla-central/rev/f2b451ce55d4
https://hg.mozilla.org/mozilla-central/rev/fb89dbd922ba
https://hg.mozilla.org/mozilla-central/rev/0e50d9d1d069
https://hg.mozilla.org/mozilla-central/rev/9e1544ec814d
https://hg.mozilla.org/mozilla-central/rev/1e657fa97b71
https://hg.mozilla.org/mozilla-central/rev/e6f4a2d1df9a

Comment 57

[Natalia Csoregi [:nataliaCs]]

Backed out for M(5) permafails mochitest/test_ext_webrequest_filter.html

Backout: https://hg.mozilla.org/mozilla-central/rev/2e0db1b48499fcf1598f23e04211360f9d39b7af

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=e6f4a2d1df9a4a50b1b659f87dca99d88a5ef63a (initial push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e6f4a2d1df9a4a50b1b659f87dca99d88a5ef63a merged to central)

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=153187458&repo=mozilla-inbound&lineNumber=29413

Comment 58

[Marcia Knous [:marcia]]

This was backed out, but I wanted to note it also caused these crashes: http://bit.ly/2l3PXur

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Sorry for the fire drill.  I think there was an intermittent for this test and it seemed unlikely to be effected by my changes so I didn't notice it perma-oranged.

Looking at the test it is clearly not using service workers correctly:

https://searchfox.org/mozilla-central/rev/e6fd22c37447579bd544254685c80fd0da221d93/toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html#92

It must wait for the worker to become active, etc.

I'll have to investigate the assertions some more.  Some notes:

1. The stack indicates the is *not* inner window re-use.
2. Its unclear if the initial about:blank ClientSource is being taken from the docshell or not.
3. Its most likely the controller is being set via the FetchEvent interception path
4. This suggests there is a mismatch between what nsDocShell::ShouldPrepareForIntercept() does and the storage assertion
5. Most like this is due to StorageAccessForWindow() returning StorageAccess::eSessionScoped which nsDocShell does not check for.

So most likely we need to fix nsDocShell::ShouldPrepareForIntercept() to use nsContentUtils::StorageAllowedForPrincipal() in addition to its third party iframe checks it does today.  We can't just use StorageAllowedForWindow() since the window does not exist in the tree yet.

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Of course, setting the prefs that would return eSessionScoped does not trigger the crash in that build for me.  I'll have to investigate more.

It also seems like many of the crashes are from profiles with ad blockers installed.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #60)
> Of course, setting the prefs that would return eSessionScoped does not
> trigger the crash in that build for me.  I'll have to investigate more.
> 
> It also seems like many of the crashes are from profiles with ad blockers
> installed.

I think I could not produce this in my previous tests because of bug 1426979.

I made this page which has a static iframe:

https://fetch-event-echo.glitch.me/index-with-frame.html

If I set cookies to expire "when I close Nightly" then it triggers the assertion.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Add session lifetime policy checks to test_third_party_iframe.html. r=asuth

This test triggers the crash.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Add session lifetime policy checks to test_third_party_iframe.html. r=asuth

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Check StorageAllowedForPrincipal() when determining if a window should be controlled. r=asuth

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Check StorageAllowedForPrincipal() when determining if a window should be controlled. r=asuth

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

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Check StorageAllowedForPrincipal() when determining if a window should be controlled. r=asuth

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

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Add a StorageAllowedForNewWindow() method that uses a URL and parent window for 3rd-party checks. r=asuth

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Make nsDocShell use StorageAllowedForNewWindow() when checking for a service worker controller. r=asuth

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

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

I've been trying to make nsDocShell use nsContentUtils::StorageAllowed*() methods.  Unfortunately thats not really possible at the moment since we can't ensure that nsPermissionManager has all its data for the first docshell in a process.  So for now I plan to punt on that improvement.  See bug 1428130.

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Make nsDocShell check for session cookie lifetime policy before allowing service worker intercept. r=asuth

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

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Make nsDocShell check for session cookie lifetime policy before allowing service worker intercept. r=asuth

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Make web extension tests wait for service worker to activate to avoid races. r=kmag

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

Comment 73

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8939984 [details] [diff] [review]
P17 Make web extension tests wait for service worker to activate to avoid races. r=kmag

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

Kris, I'm working on some service worker changes to support multi-e10s and ran into some web extension test failures when I changed the timing of things.  It looks like these tests are making a couple of common service worker mistakes:

1. They are not waiting for the registered service worker to become active before proceeding with the test.
2. They are not removing the service worker controlled window when they unregister causing the service worker to stick around longer than we want.

This patch fixes these two problems.

Note, I do have a minor eslint issue to fix to use double quote strings.  Otherwise, though, the try build is green with this patch.

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8939636 [details] [diff] [review]
P15 Add session lifetime policy checks to test_third_party_iframe.html. r=asuth

Andrew, this patch updates our test_third_party_iframe.html service worker test to check session cookie policy as well.  It triggers the crash which occurred on my first landing here.

I did briefly look at removing all the extra pushPrefEnv() here, but it did not seem immediately fixable without more investigation so I punted on that again.

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8939979 [details] [diff] [review]
P16 Make nsDocShell check for session cookie lifetime policy before allowing service worker intercept. r=asuth

This patch fixes the crash by making nsDocShell avoid intercepting navigation requests if session cookie policy is in place.

I spent a day or two trying to make this code us nsContentUtils::StorageAllowed*(), but ultimately had to keep our lame docshell side implementation.  It seems docshell needs to run this code before the cookie permission is in the child process.  Once we move interception to the parent process we should be able to switch over to StorageAllowed*() methods.  I filed bug 1428130 to follow-up there.

I did factor out the checks in docshell into a helper routine, though.  This lets us use the same logic in ShouldPrepareToIntercept() and MaybeCreateInitialClientSource().

Note, I did change behavior slightly.  Previously we would not intercept subresource requests if the cookie policy was changed to reject after the window was controlled.  I tried to do the same with this code, but that caused a test to fail which expected this kind of fetch() to work with session cookie policy.

After thinking about it I decided to just make the restrictions based at navigation interception and initial control time.  If the policy is changed after controlling a window then that page is still controlled.  That way the page doesn't start bypassing the service worker while seeing a controller service worker value.

Comment 76

[Kris Maglione [:kmag]]

Comment on attachment 8939984 [details] [diff] [review]
P17 Make web extension tests wait for service worker to activate to avoid races. r=kmag

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

Thanks. These tests have been pretty dodgy for a while.

::: toolkit/components/extensions/test/mochitest/head.js
@@ +67,5 @@
> +      if (sw.state !== state) {
> +        return;
> +      }
> +      sw.removeEventListener('statechange', onStateChange);
> +      resolve();

Nit: `if (sw.state === state) { ... }`

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Make web extension tests wait for service worker to activate to avoid races. r=kmag

Comment 78

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8939979 [details] [diff] [review]
P16 Make nsDocShell check for session cookie lifetime policy before allowing service worker intercept. r=asuth

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

Nice clean-up and comments documenting the present and future!

::: docshell/base/nsDocShell.h
@@ +367,5 @@
>  
> +  // Determine if a service worker is allowed to control a window in this
> +  // docshell with the given URL.  If there are any reasons it should not,
> +  // this will return false.  If true is returned then the window *may* be
> +  // controleld.  The caller must still consult either the parent controller

typo not: s/controleld/controlled/

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Make nsDocShell check for session cookie lifetime policy before allowing service worker intercept. r=asuth

Comment 80

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8939636 [details] [diff] [review]
P15 Add session lifetime policy checks to test_third_party_iframe.html. r=asuth

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

Our mochitest framework totally needs some kind of magic template literal parser that lets us draw the testing matrix using cool emojis instead of listing them all out by hand.  Like:

runTestMatrix("interception allowed", matrixParser`
                 LIFETIME_EXPIRE LIFETIME_SESSION
BEHAVIOR_ACCEPT        :)              :(
`, testShouldIntercept);

Obviously, pretend I know how to make emojis.

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21042078d807
P1 Add ClientHandle::OnDetach() which returns a MozPromise that resolves on actor destruction. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7cf1394d0f
P2 Add ServiceWorkerManager mControlledClients to track controlled ClientHandle references. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/818fd73be280
P3 Refactor ServiceWorkerManager::GetDocumentRegistration() to GetClientRegistration(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d432082aabbd
P4 Make ServiceWorkerManager::UpdateClientControllers use mControlledClients. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/560f15a80097
P5 Make ServiceWorkerManager::RemoveRegistration assert there is no controlled document. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/834f38d73b6f
P6 Rename some service worker methods to not reference documents. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/841e106b2810
P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/508a6ad11141
P8 Fix unregister-then-register-new-script.https.html to not race iframe.remove() and expect resurrection on failed scripts. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8449669c2adc
P9 Refactor MaybeCheckNavigationUpdate() to take a ClientInfo instead of a document. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4ffe879be7
P10 Fix the test_skip_waiting.html mochitest to properly wait for active worker state. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/86acf7e4d09d
P11 Fix test_workerupdatefound.html not to frame loading against SW activation and updatefound events. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8406786b419
P12 Don't mark an initial about:blank client as controlled if its sandboxed. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f1a8bfebf4
P13 Check for a different final document principal and reset the ClientSource when it happens. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b92985e941
P14 Assert that storage is allowed when a ClientSource is both execution ready and controlled. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/93e07d0261cb
P15 Add session lifetime policy checks to test_third_party_iframe.html. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1109ca1c1f
P16 Make nsDocShell check for session cookie lifetime policy before allowing service worker intercept. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/141238320685
P17 Make web extension tests wait for service worker to activate to avoid races. r=kmag

Comment 82

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/21042078d807
https://hg.mozilla.org/mozilla-central/rev/ef7cf1394d0f
https://hg.mozilla.org/mozilla-central/rev/818fd73be280
https://hg.mozilla.org/mozilla-central/rev/d432082aabbd
https://hg.mozilla.org/mozilla-central/rev/560f15a80097
https://hg.mozilla.org/mozilla-central/rev/834f38d73b6f
https://hg.mozilla.org/mozilla-central/rev/841e106b2810
https://hg.mozilla.org/mozilla-central/rev/508a6ad11141
https://hg.mozilla.org/mozilla-central/rev/8449669c2adc
https://hg.mozilla.org/mozilla-central/rev/8e4ffe879be7
https://hg.mozilla.org/mozilla-central/rev/86acf7e4d09d
https://hg.mozilla.org/mozilla-central/rev/d8406786b419
https://hg.mozilla.org/mozilla-central/rev/42f1a8bfebf4
https://hg.mozilla.org/mozilla-central/rev/e7b92985e941
https://hg.mozilla.org/mozilla-central/rev/93e07d0261cb
https://hg.mozilla.org/mozilla-central/rev/5b1109ca1c1f
https://hg.mozilla.org/mozilla-central/rev/141238320685

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b84fe2ad1ca2
Fix merge bustage. r=bustage-fix a=bustage-fix

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/9099a6ed993f
Follow-up: Move method to correct position. r=merge-fix a=merge-fix