1231211 - separate service worker controller from docshell

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Part 1: Extract network interception controller out of nsDocShell

Step 1, ie. the simplest possible extraction.

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 8701053 [details] [diff] [review]
Part 1: Extract network interception controller out of nsDocShell

Maybe I'll wait until it passes tests...

Comment 3

[Josh Matthews [:jdm]]

Attachment: Part 1: Extract network interception controller out of nsDocShell

Comment 4

[Andrea Marchesini [:baku]]

Attachment: Part 1: Extract network interception controller out of nsDocShell

Comment 5

[Josh Matthews [:jdm]]

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 6

[Josh Matthews [:jdm]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow docshell reload state to be set on LoadInfo. r=valentin

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Pass the controller ServiceWorkerDescriptor on the channel LoadInfo and back in PHttpChannel's OnStartRequest message. r=valentin

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin

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.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin

This patch passes principal verification.  I think I will probably move that bit out into a separate IPC verification patch as P4, though.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set isReload on LoadInfo in docshell. r=asuth

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add nsContentUtils::IsNonSubresourceInternalPolicyType() method. r=asuth

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Automatically set the controller on the LoadInfo for subresource requests. r=asuth

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Pass the nsIChannel to ShouldPrepareForIntercept(). r=asuth

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make nsDocShell::ShouldPrepareForIntercept() only depend on channel data. r=asuth

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Move logic out of nsDocShell::ChannelControlled() and into ServiceWorkerManager::DispatchFetchEvent(). r=asuth

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Move nsDocShell logic into separate ServiceWorkerInterceptController. r=asuth

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Add a StorageAllowedForNonSubresourceChannel() and use it in ServiceWorkerInterceptController. r=asuth

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Add a StorageAllowedForChannel() and use it in ServiceWorkerInterceptController. r=asuth

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

I considered punting on this part, but lets see if we can do it the right way now.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Make worker ScriptLoader pass ClientInfo and ServiceWorkerDescriptor to NS_NewChannel() where appropriate. r=asuth

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Expect to pass shared-worker-controlled.https.html now. r=asuth

We also pass shared-worker-controlled.https.html now.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: unregister-fix.patch

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Move logic out of nsDocShell::ChannelControlled() and into ServiceWorkerManager::DispatchFetchEvent(). r=asuth

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Make worker ScriptLoader pass ClientInfo and ServiceWorkerDescriptor to NS_NewChannel() where appropriate. r=asuth

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

With this fix we are able to pass WPT tests as we do today.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Fix test_sandbox_intercept.html to not use allow-same-origin attribute. r=asuth

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans

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

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P18 Make blob URL worker scripts inherit the parent's controller. r=asuth

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P18 Make blob URL worker scripts inherit the parent's controller. r=asuth

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

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow docshell reload state to be set on LoadInfo. r=valentin

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.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Pass the controller ServiceWorkerDescriptor on the channel LoadInfo and back in PHttpChannel's OnStartRequest message. r=valentin

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.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Serialize LoadInfo's mClientInfo, mReservedClientInfo, and mReservedClientInfo members across IPC. r=valentin

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

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).

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set isReload on LoadInfo in docshell. r=asuth

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add nsContentUtils::IsNonSubresourceInternalPolicyType() method. r=asuth

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Automatically set the controller on the LoadInfo for subresource requests. r=asuth

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Pass the nsIChannel to ShouldPrepareForIntercept(). r=asuth

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make nsDocShell::ShouldPrepareForIntercept() only depend on channel data . r=asuth

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Move logic out of nsDocShell::ChannelControlled() and into ServiceWorker Manager::DispatchFetchEvent(). r=asuth

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Move nsDocShell logic into separate ServiceWorkerInterceptController. r=asuth

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 73

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Add a StorageAllowedForChannel() and use it in ServiceWorkerInterceptController. r=asuth

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

(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 76

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Make worker ScriptLoader pass ClientInfo and ServiceWorkerDescriptor to NS_NewChannel() where appropriate. r=asuth

Andrew, this patch makes the worker ScriptLoader use the new NS_NewChannel() API to pass the worker's ClientInfo and ServiceWorkerDescriptor.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

I have to pause for dinner, but will continue flagging patches later tonight.

Comment 81

[Valentin Gosu [:valentin] (he/him)]

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_ (

Comment 82

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Make fetch() pass worker ClientInfo and ServiceWorkerDescriptor to NS_N ewChannel(). r=asuth

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 84

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Make xhr pass worker ClientInfo and ServiceWorkerDescriptor to NS_NewChannel(). r=asuth

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 86

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Expect to pass shared-worker-controlled.https.html now. r=asuth

Update WPT test expectations for new passing tests.  This is a result of the improvements to how we treat the worker controller.

Comment 87

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Fix test_sandbox_intercept.html to not use allow-same-origin attribute. r=asuth

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.

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P18 Make blob URL worker scripts inherit the parent's controller. r=asuth

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.

Comment 89

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans

Comment 90

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 91

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P20 Don't call CancelInterception() if an error occurs in ChannelIntercepted(). r=asuth

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.

Comment 92

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 93

[Valentin Gosu [:valentin] (he/him)]

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?

Comment 94

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Pass the controller ServiceWorkerDescriptor on the channel LoadInfo and back in PHttpChannel's OnStartRequest message. r=valentin

Comment 95

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 96

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 97

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 98

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 99

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Allow the ClientInfo and ServiceWorkerDescriptor to be passed to NS_NewChannel() for principal based loads. r=valentin

Comment 100

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Make DevToolsUtils.newChannelForURL() set the loadingNode so that the SW controller is set on the channel. r=jryans

Comment 101

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

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!

Comment 102

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

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.

Comment 103

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

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.

Comment 104

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

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.

Comment 105

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

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.)

Comment 106

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

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/;;/;/

Comment 107

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

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!

Comment 108

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Add a StorageAllowedForChannel() and use it in ServiceWorkerInterceptController. r=asuth

Comment 109

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Fix test_sandbox_intercept.html to not use allow-same-origin attribute. r=asuth

Comment 110

[Pulsebot]

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

Comment 111

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d1567cc8c742
https://hg.mozilla.org/mozilla-central/rev/3539077ab9a7
https://hg.mozilla.org/mozilla-central/rev/eb8adf8c64ec
https://hg.mozilla.org/mozilla-central/rev/7f17473a7b41
https://hg.mozilla.org/mozilla-central/rev/53679221bad1
https://hg.mozilla.org/mozilla-central/rev/e3a30f0dc503
https://hg.mozilla.org/mozilla-central/rev/93b38b36a962
https://hg.mozilla.org/mozilla-central/rev/c384b60e2a0d
https://hg.mozilla.org/mozilla-central/rev/1d3c7f0f0559
https://hg.mozilla.org/mozilla-central/rev/0c329c76fc13
https://hg.mozilla.org/mozilla-central/rev/af8c5b7bf796
https://hg.mozilla.org/mozilla-central/rev/60a29396c52f
https://hg.mozilla.org/mozilla-central/rev/8fc3f67ba98e
https://hg.mozilla.org/mozilla-central/rev/8d0f00c16279
https://hg.mozilla.org/mozilla-central/rev/267096d4a6e6
https://hg.mozilla.org/mozilla-central/rev/20f013d92f4a
https://hg.mozilla.org/mozilla-central/rev/7b0d7cffb81e
https://hg.mozilla.org/mozilla-central/rev/591601f47ee9
https://hg.mozilla.org/mozilla-central/rev/031f9a598f74
https://hg.mozilla.org/mozilla-central/rev/470a289fb31c