separate service worker controller from docshell

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 3 bugs)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [e10s-multi:M3])

Attachments

(20 attachments, 44 obsolete attachments)

10.62 KB, patch
valentin
: review+
Details | Diff | Splinter Review
10.67 KB, patch
valentin
: review+
Details | Diff | Splinter Review
1.08 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.30 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.56 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.80 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.91 KB, patch
asuth
: review+
Details | Diff | Splinter Review
15.10 KB, patch
asuth
: review+
Details | Diff | Splinter Review
9.86 KB, patch
asuth
: review+
Details | Diff | Splinter Review
13.67 KB, patch
asuth
: review+
Details | Diff | Splinter Review
5.89 KB, patch
asuth
: review+
Details | Diff | Splinter Review
7.21 KB, patch
asuth
: review+
Details | Diff | Splinter Review
822 bytes, patch
asuth
: review+
Details | Diff | Splinter Review
6.03 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.29 KB, patch
asuth
: review+
Details | Diff | Splinter Review
23.86 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
48.43 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.25 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
9.96 KB, patch
Details | Diff | Splinter Review
2.38 KB, patch
Details | Diff | Splinter Review
Currently the main implementation of nsINetworkInterceptController is nsDocShell.  In order to support remote IPC clients we really need to separate the controller from the docshell.

This bug is to create a layer between the channel/SWM and the docshell so we can easily introduce new controllers in the future.
Step 1, ie. the simplest possible extraction.
Assignee: nobody → josh
Status: NEW → ASSIGNED
Comment on attachment 8701053 [details] [diff] [review]
Part 1: Extract network interception controller out of nsDocShell

Maybe I'll wait until it passes tests...
Attachment #8701053 - Attachment is obsolete: true
Assignee: josh → amarchesini
Assignee: amarchesini → josh
Assignee: josh → amarchesini
Attachment #8701062 - Attachment is obsolete: true
Attachment #8770122 - Flags: review?(josh)
Comment on attachment 8770122 [details] [diff] [review]
Part 1: Extract network interception controller out of nsDocShell

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

Is this different at all from the previous patch I attached? I had previously decided that this work didn't gain us anything; what's changed?
Comment on attachment 8770122 [details] [diff] [review]
Part 1: Extract network interception controller out of nsDocShell

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

Clearing review until previous questions are answered.
Attachment #8770122 - Flags: review?(josh)
Whiteboard: [e10s-multi:M1]
Whiteboard: [e10s-multi:M1] → [e10s-multi:M3]
I plan to work this after the new Client infrastructure.  The Client primitive will allow the SW code to reference things other than nsIDocument nsDocShell, etc.
Assignee: amarchesini → bkelly
Depends on: 1293277
I'm actively working on this now.  My plan is:

1. Make nsDocShell store all data on the nsIChannel necessary for us to perform the interception in the parent process.
2. Create a separate network controller object that operates on this nsIChannel data.
3. Make nsDocShell attach the controller object to the nsIChannel or forward to it.

In the future we can then move the controller object to the parent process and stop using it in nsDocShell.  From ServiceWorkerManager's perspective, though, it will not be getting any information directly from docshell.  Instead it will be coming from nsIChannel data.
Blocks: 1231218
No longer blocks: ServiceWorkers-e10s
Attachment #8770122 - Attachment is obsolete: true
Also note, docshell will still have some service worker code.  It must do some checks to see if a controller should be propagated from a parent window to an about:blank or about:srcdoc child frame.
Some of the IPC validation I have in here is failing and I need to investigate why.  I think it might indicate a bug in the initial client stuff in docshell.
This patch passes principal verification.  I think I will probably move that bit out into a separate IPC verification patch as P4, though.
Attachment #8942355 - Attachment is obsolete: true
See Also: → 1430348
Attachment #8943011 - Attachment description: hg log → P7 Pass the nsIChannel to ShouldPrepareForIntercept(). r=asuth
I have a few things to fix here:

1. We are not intercepting worker subresource requests like we used to.  See test_fetch_event.html.
2. We are not intercepting some stylesheets like we used to.  See browser_styleeditor_bug_1405342_serviceowker_iframes.js.
3. We need to set the LOAD_BYPASS_SERVICE_WORKER flag in nsDocShell is storage is blocked.
(In reply to Ben Kelly [:bkelly] from comment #24)
> 3. We need to set the LOAD_BYPASS_SERVICE_WORKER flag in nsDocShell is
> storage is blocked.

Actually, I need to do the storage check without docshell involved.  This is important so we can stop sending cookie permissions across to the child all the time.  Looks like it should be straightforward.
Attachment #8943321 - Attachment is obsolete: true
I considered punting on this part, but lets see if we can do it the right way now.
We pass test_fetch_event.html again with these patches.  As bonus we now properly set FetchEvent.clientId for subresource requests from worker threads.

Need to investigate some WPT failures.
We also pass shared-worker-controlled.https.html now.
Other WPT issues currently:

Unexpected Logs
---------------
/service-workers/service-worker/sandboxed-iframe-fetch-event.https.html
  CRASH [Parent]
/service-workers/service-worker/service-worker-csp-connect.https.html
  FAIL Fetch test for connect-src
  FAIL Redirected fetch test for connect-src
/service-workers/service-worker/service-worker-csp-default.https.html
  FAIL importScripts test for default-src
  FAIL Fetch test for default-src
  FAIL Redirected fetch test for default-src
/service-workers/service-worker/service-worker-csp-script.https.html
  FAIL importScripts test for script-src
/service-workers/service-worker/unregister-controller.https.html
  TIMEOUT Unregister does not affect existing controller
  TIMEOUT [Parent]

The sandboxed issue is because of a mis-matched principal triggering an assertion I added.

Not sure what is up with the other failures yet.
The CSP failures are doing to CSP being passed on nsIPrincipal.  I'll have to change my NS_NewChannel() API to pass both the loading principal and ClientInfo.  Even though the ClientInfo is the same principal, it will be missing the latest mutable CSP from the worker's principal.

We'll have to wait until bug 965637 to remove the explicit loading principal when ClientInfo is passed.
Blocks: 965637
The unregister-controller.https.html failure is because we are honoring the uninstalling flag when intercepting when we should not.  If a page is controlled, we should intercept even if it is being uninstalled.
Posted patch unregister-fix.patch (obsolete) — Splinter Review
With this fix we are able to pass WPT tests as we do today.
Attachment #8943727 - Attachment is obsolete: true
Some devtools tests were failing because things were not getting intercepted any more.  This patch sets the loadingNode on the channel to the document so the controller gets set properly.  Lets see if this breaks anything else...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3423c70181c18879959d4f0894315fa35e72e2dd
See Also: → 1431814
Blocks: 1431814
See Also: 1431814
Blocks: 1431847
I have some work-in-progress patches that build on top of this patch to allow us to start doing parent-side intercept based on a pref.  It needs some more work, though.  I will move it to another bug since this bug is already quite long.
Valentin, this bug is working towards our end goal of performing service worker interception in the parent process.  To that end we need to propagate some channel properties from the child to the parent in order to construct the FetchEvent there.

This patch is the first of many passing more information over to the parent.  In this case it adding the docshell reload state to the LoadInfo and making it IPC serialize.  Note this value is different from the http cache reload flags.  While a docshell reload will trigger the use of certain http cache flags, there are other reasons why those cache flags might be set as well.  This boolean tracks the docshell intent.
Attachment #8942251 - Attachment is obsolete: true
Attachment #8944078 - Flags: review?(valentin.gosu)
Valentin, the next thing we need to do is pass the LoadInfo's mController ServiceWorkerDescriptor across the IPC boundary.  We need to pass this both from child-to-parent, but also back from parent-to-child in the OnStartRequest.  This parent-to-child step is necessary since the parent will be determining if a navigation is controlled and the resulting window will try to get this information from the LoadInfo.
Attachment #8942320 - Attachment is obsolete: true
Attachment #8944080 - Flags: review?(valentin.gosu)
Comment on attachment 8944081 [details] [diff] [review]
P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin

This patch serializes the LoadInfo's mClientInfo, mReservedClientInfo, and mInitialClientInfo.  This is needed to populate FetchEvent.clientId (today) and FetchEvent.resultingClientId (in the future).
Attachment #8944081 - Flags: review?(valentin.gosu)
Comment on attachment 8944082 [details] [diff] [review]
P4 Set isReload on LoadInfo in docshell. r=asuth

Andrew, this patch uses the new LoadInfo API introduced in P1 to set the docshell reload flag.  We need this on the channel in order to create the FetchEvent.
Attachment #8944082 - Flags: review?(bugmail)
Comment on attachment 8944083 [details] [diff] [review]
P5 Add nsContentUtils::IsNonSubresourceInternalPolicyType() method. r=asuth

This patch adds an nsContentUtils helper method to determine if a particular nsContentPolicyType is a NonSubresource.
Attachment #8944083 - Flags: review?(bugmail)
Comment on attachment 8944084 [details] [diff] [review]
P6 Automatically set the controller on the LoadInfo for subresource requests. r=asuth

This patch sets the window's controller on the LoadInfo automatically.  We used to only use LoadInfo::mController to communicate a navigation controller back to the resulting window.  Here we are using it to communicate the window's controller back down on a subresource load.

Note, a later patch in the queue will fix up the worker situation.
Attachment #8944084 - Flags: review?(bugmail)
Comment on attachment 8944085 [details] [diff] [review]
P7 Pass the nsIChannel to ShouldPrepareForIntercept(). r=asuth

We want the nsINetworkInterceptController to be able to operate completely independent from the docshell using only the data on the nsIChannel.  To allow that, though, we must pass the nsIChannel to the controller.  Previously we did not do this for ShouldPrepareForIntercept.

This patch modifies ShouldPrepareForIntercept() to take the nsIChannel.
Attachment #8944085 - Flags: review?(bugmail)
Comment on attachment 8944086 [details] [diff] [review]
P8 Make nsDocShell::ShouldPrepareForIntercept() only depend on channel data . r=asuth

This patch refactors some of nsDocShell::ShouldPrepareForIntercept() to use the nsIChannel instead of local data.  More changes are in later patches.
Attachment #8944086 - Flags: review?(bugmail)
Comment on attachment 8944087 [details] [diff] [review]
P9 Move logic out of nsDocShell::ChannelControlled() and into ServiceWorker Manager::DispatchFetchEvent(). r=asuth

This patch the ChannelIntercepted() logic out of nsDocShell and into the ServiceWorkerManager::DispatchFetchEvent().  This helps ensure that we're not accidentally touching anything related to docshell.

Note, I added a CancelInterception() here.  This is incorrect and I'm going to remove it in P20.  I am doing it in a follow-up patch to avoid rebasing everything.
Attachment #8944087 - Flags: review?(bugmail)
Comment on attachment 8944091 [details] [diff] [review]
P10 Move nsDocShell logic into separate ServiceWorkerInterceptController. r=asuth

This patch moves the guts of the nsINetworkInterceptController interface our of nsDocShell into a new ServiceWorkerInterceptController class.  Right now I just have docshell delegating to this new class.  In the future we can use a pref to control if we do this forwarding.  We can also now create instances of this class in the parent process, etc.
Attachment #8944091 - Flags: review?(bugmail)
Comment on attachment 8944092 [details] [diff] [review]
P11 Add a StorageAllowedForChannel() and use it in ServiceWorkerInterceptController. r=asuth

One of the things we lost in P10 was a storage check in docshell.  To fix this I am adding a StorageAllowedForChannel() helper method here.  The next patch will then add the storage check back to ServiceWorkerInterceptController.

Note, we must do the storage check in the intercept controller so that ultimately we are doing the storage check in the parent process.
Attachment #8944092 - Flags: review?(bugmail)
(In reply to Ben Kelly [:bkelly] from comment #74)
> One of the things we lost in P10 was a storage check in docshell.  To fix
> this I am adding a StorageAllowedForChannel() helper method here.  The next
> patch will then add the storage check back to
> ServiceWorkerInterceptController.

I mean, P11 uses it to add the storage check back.
Comment on attachment 8944093 [details] [diff] [review]
P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

Valentin, this is a pretty significant patch.  It adds a new NS_NewChannel()/NS_NewChannelWithTriggeringPrincipal() variant that takes ClientInfo and ServiceWorkerDescriptor controller.

When we are on a main thread we currently get ClientInfo and the controller from the loading node.  We can't do this on worker threads, of course, because there is no nsIDocument for the worker itself.

This new API is basically like the loading principal versions of NS_NewChannel*().  The ClientInfo and controller are simply passed through and set on the LoadInfo instead of getting them from the loading node.

Note, I added some validation that the ClientInfo is appropriate for the loading principal.  I could not completely get rid of the loading principal for these loads for a number of reasons.  The debugger wants to pass a completely different system principal, for example.  Also, there are mutable things like CSP on the principal which we want to pass through.  Ultimately those will likely get moved to ClientInfo.

One weakness of this API change is currently there is no way to specify a ClientInfo and controller ServiceWorkerDescriptor from chrome script.  Things like devtools must use a loadingNode approach to get them set.  If necessary I'll figure out how to fix that in a future bug.
Attachment #8944093 - Flags: review?(valentin.gosu)
Andrew, this patch makes the worker ScriptLoader use the new NS_NewChannel() API to pass the worker's ClientInfo and ServiceWorkerDescriptor.
Attachment #8943728 - Attachment is obsolete: true
Comment on attachment 8944095 [details] [diff] [review]
P13 Make worker ScriptLoader pass ClientInfo and ServiceWorkerDescriptor to NS_NewChannel() where appropriate. r=asuth

See comment 78.
Attachment #8944095 - Flags: review?(bugmail)
I have to pause for dinner, but will continue flagging patches later tonight.
Attachment #8944078 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8944080 [details] [diff] [review]
P2 Pass the controller ServiceWorkerDescriptor on the channel LoadInfo and back in PHttpChannel's OnStartRequest message. r=valentin

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

The info in comment 56 would go nicely in the patch description, or somewhere in the code.

::: ipc/glue/BackgroundUtils.cpp
@@ +369,5 @@
>    }
>  
> +  OptionalIPCServiceWorkerDescriptor ipcController = mozilla::void_t();
> +  const Maybe<ServiceWorkerDescriptor>& controller = aLoadInfo->GetController();
> +  if(controller.isSome()) {

nit: if _space_ (
Attachment #8944080 - Flags: review?(valentin.gosu) → review+
Attachment #8944081 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8944134 [details] [diff] [review]
P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_N ewChannel(). r=asuth

Make dom/fetch propagate the worker ClientInfo and controller to NS_NewChannel().
Attachment #8944134 - Flags: review?(bugmail)
Comment on attachment 8944135 [details] [diff] [review]
P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

This patch makes xhr propagate the worker ClientInfo and controller to NS_NewChannel().
Attachment #8944135 - Flags: review?(bugmail)
Update WPT test expectations for new passing tests.  This is a result of the improvements to how we treat the worker controller.
Attachment #8943395 - Attachment is obsolete: true
Attachment #8944136 - Flags: review?(bugmail)
This patch updates test_sandbox_intercept.html to check that we block service workers in sandboxes *without* allow-same-origin.  See this spec issue:

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

It clarifies that we should allow interception as long as allow-same-origin is on the sandboxed iframe.

Note, the next couple patches get us passing the service worker sandbox WPT as well for extra test coverage.
Attachment #8943769 - Attachment is obsolete: true
Attachment #8944137 - Flags: review?(bugmail)
This patch gets us to pass sandboxed-iframe-fetch-event.https.html.  There was a remaining issue that we were not properly inheriting the service worker for blob URL workers.  This test uses blob URL workers in order to consistently inherit the sandboxed iframe's origin regardless of its attributes.

Note, the spec says to inherit the service worker for local URLs here:

https://w3c.github.io/ServiceWorker/#selection

I filed a couple spec issues about this as well:

https://github.com/w3c/ServiceWorker/issues/1261
https://github.com/w3c/ServiceWorker/issues/1262

The cross-origin question does not apply here since workers will refuse to load a cross-origin URL.
Attachment #8944140 - Flags: review?(bugmail)
Attachment #8944052 - Attachment is obsolete: true
Comment on attachment 8944141 [details] [diff] [review]
P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans

Ryan, this patch modifies DevToolsUtils.newChannelForURL() as we discussed in IRC.  To recap:

1. I need to provide a loadingNode when devtools is loading on behalf of the page.  This is necessary to get the correct ClientInfo and service worker controller for the load.

2. It seems very strange to me that devtools would use the resource URI itself to manufacture the principal.  Generally it should be using the content windows node/principal or system principal.  It doesn't seem like we should be using cross-origin principals, etc.  Anyway, I left this in with a bit of a warning comment and the code will trigger it less frequently.

3. This should not impact network monitor since we determined that is based on the nsIChannel.associatedWindow.  That is in turn dictated by the docshell when set as the nsIChannel's callbacks object.  These devtools created channels don't have a docshell set as the callbacks object (AFAICT).  I also locally tested my changes and did not see anything in network monitor.  I don't know if there is automated testing for this.

4. All of this is to allow us to pass:

devtools/client/styleeditor/test/browser_styleeditor_bug_1405342_serviceworker_iframes.js

Which verifies that the style editor loads through the service worker. AFAICT its the only devtools test that verifies this kind of thing.  In theory we should probably add similar tests for debugger, etc.

The try build with this patch is green:

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

Although its possible there is some use of this function that is not covered by tests.  We should be on the look out for changes in FF60.
Attachment #8944141 - Flags: review?(jryans)
Andrew, this fixes the previous change I made to add an extra CancelInterception() on failure.  If ChannelIntercepted() returns failure the caller tries to do a ResetInterception().  So we should not perform the cancel here.  I added a comment to this effect as well.
Attachment #8944143 - Flags: review?(bugmail)
Comment on attachment 8944141 [details] [diff] [review]
P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans

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

Thanks for working on this! :) This change looks reasonable to me given our IRC discussion.

It's not clear to me either how much testing we have on all the special cases here...  Anyway, we'll stay on the lookout for issues after this lands.

::: devtools/shared/DevToolsUtils.js
@@ +684,5 @@
> +  } else {
> +    // If a window is not provided, then we must set a loading principal.
> +
> +    // If the caller did not provide a principal, then we use the URI
> +    // to create one.  Note, its not clear what use cases require this

Nit: its -> it's
Attachment #8944141 - Flags: review?(jryans) → review+
Comment on attachment 8944093 [details] [diff] [review]
P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

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

::: netwerk/base/nsIOService.cpp
@@ +830,5 @@
> +    // might still call NewChannelFromURIWithProxyFlags() which forwards
> +    // its calls to NewChannelFromURIWithProxyFlags2 using *null* values
> +    // as the arguments for aLoadingNode, aLoadingPrincipal, and also
> +    // aTriggeringPrincipal.
> +    // We do not want to break those addons, hence we only create a Loadinfo

nit: This comment isn't fully up to date.

::: netwerk/base/nsNetUtil.cpp
@@ +286,5 @@
> +                               aLoadFlags,
> +                               aIoService);
> +}
> +
> +nsresult /* NS_NewChannelPrincipal */

nit: does NS_NewChannelPrincipal have any meaning?

@@ +445,5 @@
> +                               aIoService);
> +}
> +
> +// See NS_NewChannelInternal for usage and argument description
> +nsresult /*NS_NewChannelWithPrincipalAndTriggeringPrincipal */

nit: does NS_NewChannelWithPrincipalAndTriggeringPrincipal mean anything?

::: netwerk/base/nsNetUtil.h
@@ +239,5 @@
>                nsLoadFlags            aLoadFlags = nsIRequest::LOAD_NORMAL,
>                nsIIOService          *aIoService = nullptr);
>  
> +// See NS_NewChannelInternal for usage and argument description
> +nsresult /* NS_NewChannelPrincipal */

nit: does NS_NewChannelPrincipal have any meaning?
Attachment #8944093 - Flags: review?(valentin.gosu) → review+
Blocks: 1432184
Attachment #8944428 - Attachment description: hg log → P2 Pass the controller ServiceWorkerDescriptor on the channel LoadInfo and back in PHttpChannel's OnStartRequest message. r=valentin
Attachment #8944428 - Flags: review+
Attachment #8944080 - Attachment is obsolete: true
(In reply to Valentin Gosu [:valentin] from comment #93)
> Comment on attachment 8944093 [details] [diff] [review]
> P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to
> NS_NewChannel() for principal based loads. r=valentin
> 
> Review of attachment 8944093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsIOService.cpp
> @@ +830,5 @@
> > +    // might still call NewChannelFromURIWithProxyFlags() which forwards
> > +    // its calls to NewChannelFromURIWithProxyFlags2 using *null* values
> > +    // as the arguments for aLoadingNode, aLoadingPrincipal, and also
> > +    // aTriggeringPrincipal.
> > +    // We do not want to break those addons, hence we only create a Loadinfo
> 
> nit: This comment isn't fully up to date.

I'm not quite sure what to do here.  Its not just the comment thats out of date, but the actual code seems like it should always create a LoadInfo now that addons are gone.  I don't really want to try changing that in this bug, though.  I'll file a follow-up.

For now I'll make sure the comment at least reference the correct name.  And I'll add a comment suggesting it could be cleaned up.
Blocks: 1432205
(In reply to Ben Kelly [:bkelly] from comment #95)
> > ::: netwerk/base/nsIOService.cpp
> > @@ +830,5 @@
> > > +    // might still call NewChannelFromURIWithProxyFlags() which forwards
> > > +    // its calls to NewChannelFromURIWithProxyFlags2 using *null* values
> > > +    // as the arguments for aLoadingNode, aLoadingPrincipal, and also
> > > +    // aTriggeringPrincipal.
> > > +    // We do not want to break those addons, hence we only create a Loadinfo
> > 
> > nit: This comment isn't fully up to date.
> 
> I'm not quite sure what to do here.  Its not just the comment thats out of
> date, but the actual code seems like it should always create a LoadInfo now
> that addons are gone.  I don't really want to try changing that in this bug,
> though.  I'll file a follow-up.
> 
> For now I'll make sure the comment at least reference the correct name.  And
> I'll add a comment suggesting it could be cleaned up.

Changing the comment to:

    // Ideally all callers of NewChannelFromURIWithProxyFlagsInternal provide
    // the necessary arguments to create a loadinfo.
    //
    // Note, historically this could be called with nullptr aLoadingNode,
    // aLoadingPrincipal, and aTriggeringPrincipal from addons using
    // newChannelFromURIWithProxyFlags().  This code tried to accomodate
    // by not creating a LoadInfo in such cases.  Now that both the legacy
    // addons and that API are gone we could possibly require always creating a
    // LoadInfo here.  See bug 1432205.
(In reply to Valentin Gosu [:valentin] from comment #93)
> nit: does NS_NewChannelPrincipal have any meaning?
> nit: does NS_NewChannelWithPrincipalAndTriggeringPrincipal mean anything?

I copied these from the current code, but I don't see any other references in the tree.  I'll remove them all.  I assume they are stale from older APIs.
(In reply to Ben Kelly [:bkelly] from comment #97)
> (In reply to Valentin Gosu [:valentin] from comment #93)
> > nit: does NS_NewChannelPrincipal have any meaning?
> > nit: does NS_NewChannelWithPrincipalAndTriggeringPrincipal mean anything?
> 
> I copied these from the current code, but I don't see any other references
> in the tree.  I'll remove them all.  I assume they are stale from older APIs.

Also /* NS_NewChannelNode */, /* NS_NewStreamLoaderNode */, /* NS_NewStreamLoaderPrincipal */, etc.
Attachment #8944082 - Flags: review?(bugmail) → review+
Attachment #8944083 - Flags: review?(bugmail) → review+
Attachment #8944084 - Flags: review?(bugmail) → review+
Attachment #8944085 - Flags: review?(bugmail) → review+
Attachment #8944086 - Flags: review?(bugmail) → review+
Comment on attachment 8944087 [details] [diff] [review]
P9 Move logic out of nsDocShell::ChannelControlled() and into ServiceWorker Manager::DispatchFetchEvent(). r=asuth

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

::: docshell/base/nsDocShell.cpp
@@ +14305,2 @@
>    if (NS_WARN_IF(error.Failed())) {
> +    aChannel->CancelInterception(NS_ERROR_INTERCEPTION_FAILED);

Thank you for explicitly calling this out and providing the improved comment in P20 where you un-do the change!
Attachment #8944087 - Flags: review?(bugmail) → review+
Comment on attachment 8944091 [details] [diff] [review]
P10 Move nsDocShell logic into separate ServiceWorkerInterceptController. r=asuth

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

::: docshell/base/nsDocShell.cpp
@@ -14275,5 @@
> -                                           loadInfo->GetOriginAttributes());
> -
> -  // For navigations, first check to see if we are allowed to control a
> -  // window with the given URL.
> -  if (!ServiceWorkerAllowedToControlWindow(principal, aURI)) {

Affirming this check comes back in P11.
Attachment #8944091 - Flags: review?(bugmail) → review+
Comment on attachment 8944092 [details] [diff] [review]
P11 Add a StorageAllowedForChannel() and use it in ServiceWorkerInterceptController. r=asuth

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

::: dom/base/nsContentUtils.cpp
@@ +9097,5 @@
> +      behavior == nsICookieService::BEHAVIOR_LIMIT_FOREIGN) {
> +
> +    bool thirdParty = false;
> +
> +    // In the absense of a window or a forced third party state, we assume

typo nit: s/absense/absence/

@@ +9109,5 @@
> +                                                   aURI,
> +                                                   &thirdParty);
> +    }
> +
> +    if (aChannel) {

How about adding a `MOZ_ASSERT(!aWindow, "If you provide a window you may not provide a channel");` or something like that so no one needs to worry about the interaction of this clause with the clause above.  Or maybe up at the top of the method over both arguments so the assert doesn't get hidden by control flow.
Attachment #8944092 - Flags: review?(bugmail) → review+
Attachment #8944095 - Flags: review?(bugmail) → review+
Comment on attachment 8944134 [details] [diff] [review]
P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_N ewChannel(). r=asuth

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

::: dom/fetch/FetchDriver.cpp
@@ +519,5 @@
>                         mLoadGroup,
>                         nullptr, /* aCallbacks */
>                         loadFlags,
>                         ios);
> +  } else if (mClientInfo.isSome()) {

Restating: The mDocument conditional still gets to go first because it's using the so-called "NS_NewChannelNode" NS_NewChannel path.  As comment 77 for part P12 explains, the client and controller can be extracted from the loading node.
Attachment #8944134 - Flags: review?(bugmail) → review+
Comment on attachment 8944135 [details] [diff] [review]
P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2510,5 @@
>                         nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST,
>                         loadGroup,
>                         nullptr,   // aCallbacks
>                         loadFlags);
> +  } else if (mClientInfo.isSome()) {

(Same deal as with fetch for why mDocument still gets to go first.)
Attachment #8944135 - Flags: review?(bugmail) → review+
Attachment #8944136 - Flags: review?(bugmail) → review+
Comment on attachment 8944137 [details] [diff] [review]
P17 Fix test_sandbox_intercept.html to not use allow-same-origin attribute. r=asuth

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

::: dom/workers/test/serviceworkers/test_sandbox_intercept.html
@@ +17,5 @@
>  </div>
>  <pre id="test"></pre>
>  <script class="testbody" type="text/javascript">
>  
> +  var normalFrame;;

Typo nit: s/;;/;/
Attachment #8944137 - Flags: review?(bugmail) → review+
Attachment #8944140 - Flags: review?(bugmail) → review+
Comment on attachment 8944143 [details] [diff] [review]
P20 Don't call CancelInterception() if an error occurs in ChannelIntercepted(). r=asuth

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

Amazing cleanup patchset!  Thank you!
Attachment #8944143 - Flags: review?(bugmail) → review+

Comment 110

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1567cc8c742
P1 Allow docshell reload state to be set on LoadInfo. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/3539077ab9a7
P2 Pass the controller ServiceWorkerDescriptor on the channel LoadInfo and back in PHttpChannel's OnStartRequest message. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8adf8c64ec
P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f17473a7b41
P4 Set isReload on LoadInfo in docshell. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/53679221bad1
P5 Add nsContentUtils::IsNonSubresourceInternalPolicyType() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a30f0dc503
P6 Automatically set the controller on the LoadInfo for subresource requests. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/93b38b36a962
P7 Pass the nsIChannel to ShouldPrepareForIntercept(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c384b60e2a0d
P8 Make nsDocShell::ShouldPrepareForIntercept() only depend on channel data. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3c7f0f0559
P9 Move logic out of nsDocShell::ChannelControlled() and into ServiceWorkerManager::DispatchFetchEvent(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c329c76fc13
P10 Move nsDocShell logic into separate ServiceWorkerInterceptController. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8c5b7bf796
P11 Add a StorageAllowedForChannel() and use it in ServiceWorkerInterceptController. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/60a29396c52f
P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc3f67ba98e
P13 Make worker ScriptLoader pass ClientInfo and ServiceWorkerDescriptor to NS_NewChannel() where appropriate. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0f00c16279
P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/267096d4a6e6
P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/20f013d92f4a
P16 Expect to pass shared-worker-controlled.https.html now. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0d7cffb81e
P17 Fix test_sandbox_intercept.html to not use allow-same-origin attribute. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/591601f47ee9
P18 Make blob URL worker scripts inherit the parent's controller. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/031f9a598f74
P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/470a289fb31c
P20 Don't call CancelInterception() if an error occurs in ChannelIntercepted(). r=asuth
Depends on: 1433044
Blocks: 1032521
Blocks: 1206947
Depends on: 1436256
You need to log in before you can comment on or make changes to this bug.