Closed Bug 1425975 Opened 2 years ago Closed 2 years ago

use ClientHandle objects to track controlled clients in ServiceWorkerManager

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(17 files, 18 obsolete files)

6.73 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.54 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.18 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.73 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.88 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.37 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.10 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.87 KB, patch
asuth
: review+
Details | Diff | Splinter Review
16.64 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.92 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.85 KB, patch
asuth
: review+
Details | Diff | Splinter Review
5.01 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
8.52 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.27 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.43 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.70 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
8.11 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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.
Priority: -- → P2
Depends on: 1426253
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
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
Attachment #8938227 - Attachment description: 8 Fix unregister-then-register-new-script.https.html to not race iframe.remove() and expect resurrection on failed scripts. r=asuth → P8 Fix unregister-then-register-new-script.https.html to not race iframe.remove() and expect resurrection on failed scripts. r=asuth
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.
Attachment #8938113 - Flags: review?(bugmail)
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.
Attachment #8937904 - Flags: review?(bugmail)
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.
Attachment #8938032 - Flags: review?(bugmail)
Comment on attachment 8938033 [details] [diff] [review]
P4 Make ServiceWorkerManager::UpdateClientControllers use mControlledClients. r=asuth

Refactor UpdateClientControllers to use mControlledClients instead of mControlledDocuments.
Attachment #8938033 - Flags: review?(bugmail)
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.
Attachment #8938034 - Flags: review?(bugmail)
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".
Attachment #8938225 - Flags: review?(bugmail)
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.
Attachment #8938227 - Flags: review?(bugmail)
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.
With the work-in-progress patch and this the only remaining uses of mControlledDocuments is for logging.
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.
Attachment #8938237 - Flags: review?(bugmail)
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.
Attachment #8938377 - Flags: review?(bugmail)
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/
Attachment #8938113 - Flags: review?(bugmail) → review+
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.
Attachment #8938397 - Flags: review?(bugmail)
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 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.
Attachment #8937904 - Flags: review?(bugmail) → review+
Attachment #8938032 - Flags: review?(bugmail) → review+
Attachment #8938033 - Flags: review?(bugmail) → review+
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?
Attachment #8938034 - Flags: review?(bugmail) → review+
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.
Attachment #8938225 - Flags: review?(bugmail) → review+
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.
Attachment #8938501 - Flags: review?(bugmail)
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().
Attachment #8938502 - Flags: review?(bugmail)
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.
Attachment #8938508 - Flags: review?(bugmail)
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.
Attachment #8938504 - Flags: review?(bugmail)
Attachment #8938501 - Flags: review?(bugmail) → review+
Attachment #8938227 - Flags: review?(bugmail) → review+
Attachment #8938237 - Flags: review?(bugmail) → review+
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.
Attachment #8938377 - Flags: review?(bugmail) → review+
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.
Attachment #8938397 - Flags: review?(bugmail) → review+
Attachment #8938502 - Flags: review?(bugmail) → review+
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.
Attachment #8938504 - Flags: review?(bugmail) → review+
(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.
(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.
(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.
(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 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.
Attachment #8938508 - Flags: review?(bugmail) → review+
I need to fix test_workerupdatefound.html on non-e10s it appears.
Blocks: 1426905
(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.
Pushed by bkelly@mozilla.com:
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
This was backed out, but I wanted to note it also caused these crashes: http://bit.ly/2l3PXur
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.
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.
See Also: → 1426979
(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.
This test triggers the crash.
Flags: needinfo?(bkelly)
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.
Target Milestone: mozilla59 → ---
Depends on: 1426977
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.
Attachment #8939984 - Flags: review?(kmaglione+bmo)
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.
Attachment #8939636 - Flags: review?(bugmail)
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.
Attachment #8939979 - Flags: review?(bugmail)
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) { ... }`
Attachment #8939984 - Flags: review?(kmaglione+bmo) → review+
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/
Attachment #8939979 - Flags: review?(bugmail) → review+
Attachment #8940255 - Flags: review+
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.
Attachment #8939636 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
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
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/b84fe2ad1ca2
Fix merge bustage. r=bustage-fix a=bustage-fix
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/9099a6ed993f
Follow-up: Move method to correct position. r=merge-fix a=merge-fix
Duplicate of this bug: 1237498
Duplicate of this bug: 1247055
Depends on: 1439879
No longer depends on: 1439879
Depends on: 1461181
You need to log in before you can comment on or make changes to this bug.