1214305 - Enable interception of secure upgraded channels in e10s mode

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

This is adding e10s support to bug 1198394.

I originally started to implementing this by getting the child process to ask the parent whether a channel needs to be upgraded before doing the interception but that has turned into a very complex implementation, that after a while we decided is less than ideal.

I'm going to work on another possible idea, which is mirroring the HSTS information in the child process, and decide whether an upgrade is needed directly in the child and performing a redirect from the child side.  This will have the extra benefit of keeping the e10s and non-e10s paths similar.  Also, since we can only start talking to the network in the parent process, we won't be trusting the child to not lie about whether a channel needs to be upgraded, since when we decide to finally go to the network, the parent process will check whether an upgrade is needed just like today, so the HSTS mirrored information in the child process will only be used for determining whether to dispatch the FetchEvent on the upgraded channel or not.

Comment 1

[(no longer active)]

Attachment: Part 1: Refactor the logic for querying whether a connection should go through a secure upgrade into NS_ShouldSecureUpgrade

Comment 2

[(no longer active)]

Attachment: Part 2: Refactor the logic for obtaining the secure upgraded URI into HttpBaseChannel

Comment 3

[(no longer active)]

Attachment: Part 3: Add a nsIInterceptedChannel.secureUpgradedChannelURI helper

Comment 4

[(no longer active)]

Attachment: Part 4: Use the secure upgraded channel URI in ServiceWorkerManager::PrepareFetchEvent

This is needed to ensure that the ServiceWorkerManager uses the
correct URI for non-subresource requests.  Note that we're relying
on the property that non-secure non-subresource requests can never
be intercepted, so we don't need to check the request type explicitly.

Comment 5

[(no longer active)]

Attachment: Part 5: Use the secure upgraded channel URI in FetchEventRunnable::Init

This ensures that FetchEvent.request.url will have the correct value
for secure upgraded channels.

Comment 6

[(no longer active)]

Attachment: Part 6: Use a non-IPC redirect for synthesized upgraded responses to ensure the response URL is correctly propagated

Comment 7

[(no longer active)]

Attachment: Part 7: Decide in the child process whether an intercepted channel should go through a secure upgrade

This is OK from a security perspective, since this decision only affects
whether the channel will be intercepted with the secure URI in the child
process.  If the intercepting service worker decides to fall back to an
actual network request, we send the request to the parent process with
the original pre-upgrade URI, and the parent process will still be in
charge of whether a network visible HTTP request should be upgraded.

Comment 8

[(no longer active)]

Attachment: Part 8: Enable secure upgrade service worker tests on e10s

Comment 9

[(no longer active)]

Attachment: Part 8: Enable secure upgrade service worker tests on e10s

Comment 10

[(no longer active)]

<https://treeherder.mozilla.org/#/jobs?repo=try&revision=22df212c58e7>

Comment 11

[(no longer active)]

(In reply to [Vacation: Nov 3-19] from comment #10)
> <https://treeherder.mozilla.org/#/jobs?repo=try&revision=22df212c58e7>

This was leaking pretty badly.  I fixed the leak and repushed: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3e3a434b80d>

I think there are a few other real test failures there too, but I won't get a chance to look at them...  Josh, I'd appreciate if you would take a look, please.  Note that bug 1215723 on its own was green on the try server (sorry, don't have a link handy.)

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 8682087 [details] [diff] [review]
Part 3: Add a nsIInterceptedChannel.secureUpgradedChannelURI helper

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

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +319,5 @@
>  
> +NS_IMETHODIMP
> +InterceptedChannelChrome::GetSecureUpgradedChannelURI(nsIURI** aURI)
> +{
> +  return mChannel->GetURI(aURI);

A comment here explaining why we don't need to actually return a different URI would be useful.

Comment 13

[Jason Duell]

Comment on attachment 8682091 [details] [diff] [review]
Part 7: Decide in the child process whether an intercepted channel should go through a secure upgrade

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

All looks OK to me.

Comment 14

[Josh Matthews [:jdm]]

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

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b930683c026adb4c3fd8b7ccf782db97abb7eb
Bug 1214305 - Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b85acbe7be7111f06f44ad36a324c9e10415483
Bug 1214305 - Part 1: Refactor the logic for querying whether a connection should go through a secure upgrade into NS_ShouldSecureUpgrade; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/be072bba15fc640071e76c99df7aee5e8b90e07d
Bug 1214305 - Part 2: Refactor the logic for obtaining the secure upgraded URI into HttpBaseChannel; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/347cd0f8f1e3cad2ef816d600310dd8100820ece
Bug 1214305 - Part 3: Add a nsIInterceptedChannel.secureUpgradedChannelURI helper; r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3e4f256d4656acd316a6af9a5a9b1e57acce68
Bug 1214305 - Part 4: Use the secure upgraded channel URI in ServiceWorkerManager::PrepareFetchEvent; r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/88ab26108f2de6cb3786208f86a8b570085b9b7c
Bug 1214305 - Part 5: Use the secure upgraded channel URI in FetchEventRunnable::Init; r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/226c83ca9a2aa4c16e42603d54c2636087e985b2
Bug 1214305 - Part 6: Use a non-IPC redirect for synthesized upgraded responses to ensure the response URL is correctly propagated; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/e926606aefbf83605f7125787a2b6cade59be193
Bug 1214305 - Part 7: Decide in the child process whether an intercepted channel should go through a secure upgrade; r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/82929be36e95db2e6dfa11fbd7e3d5f1ee9f3e52
Bug 1214305 - Part 8: Enable secure upgrade service worker tests on e10s; r=jdm

Comment 16

[Phil Ringnalda (:philor)]

Surprise! We've got e10s devtools tests now, including https://treeherder.mozilla.org/logviewer.html#?job_id=17284469&repo=mozilla-inbound which wasn't happy with you. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/50b5e58fd4cd

Comment 17

[Phil Ringnalda (:philor)]

And https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tochange=5e3e0fe5e172&fromchange=82929be36e95&filter-searchStr=e10s%20debug%20browser-chrome-7 I think the leak, with or without another failure, is yours too.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Not blocking v1 because its e10s only.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC

Comment 20

[Josh Matthews [:jdm]]

I spent my time looking into the failing output of `./mach mochitest --e10s devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js` . The `content.location = TEST_URI` line appears to fail in the child process, but when I stepped through it in the debugger two things happened:
* I continued to see the following test failure while the child was paused:
10 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js | A promise chain failed to handle a rejection:  - at chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js:45 - Error: cross-process JS call failed
* when I stepped completely through the operation, the child never returned a non-NS_OK/true value at any point

Setting the ipc timeout to 0 (indefinite) didn't change this result.

Comment 21

[Jim Mathies [:jimm]]

(In reply to Ben Kelly [:bkelly] from comment #18)
> Not blocking v1 because its e10s only.

I've seen this a few times in service worker bugs. FYI e10s is rolling out to 45 beta to a large number of users. If the service worker team has e10s specific show stoppers can we get those tracked to 45 and owned?

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jim Mathies [:jimm] from comment #21)
> I've seen this a few times in service worker bugs. FYI e10s is rolling out
> to 45 beta to a large number of users. If the service worker team has e10s
> specific show stoppers can we get those tracked to 45 and owned?

There is no way these are getting fixed in 45 given there is just over a week to when 45 merges to aurora.  The service worker team is going to be focusing on how to fix our e10s support at mozlando.  The solution, however, is likely to take longer than a single release cycle.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Move to SW component for easier tracking.

Comment 24

[(no longer active)]

Comment on attachment 8690108 [details] [diff] [review]
Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC

This is left-over from bug 1215723.

Comment 25

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #20)
> I spent my time looking into the failing output of `./mach mochitest --e10s
> devtools/client/webconsole/test/
> browser_bug1045902_console_csp_ignore_reflected_xss_message.js` . The
> `content.location = TEST_URI` line appears to fail in the child process, but
> when I stepped through it in the debugger two things happened:
> * I continued to see the following test failure while the child was paused:
> 10 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/webconsole/test/
> browser_bug1045902_console_csp_ignore_reflected_xss_message.js | A promise
> chain failed to handle a rejection:  - at
> chrome://mochitests/content/browser/devtools/client/webconsole/test/
> browser_bug1045902_console_csp_ignore_reflected_xss_message.js:45 - Error:
> cross-process JS call failed
> * when I stepped completely through the operation, the child never returned
> a non-NS_OK/true value at any point
> 
> Setting the ipc timeout to 0 (indefinite) didn't change this result.

OK, I finally figured this out.

The failure happens here: <https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/browser_bug1045902_console_csp_ignore_reflected_xss_message.js#45>.  Here, |content| is a CPOW.  The setter goes through WrapperOwner::set in the parent process which fails here: <https://dxr.mozilla.org/mozilla-central/source/js/ipc/WrapperOwner.cpp#553>.  The reason SendSet() fails is that the transaction has been canceled <https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#835>.  I tracked that down to <https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#881> in the child process.  Basically what's happening is that we're processing a sync message from the parent, and with my changes here, as part of that we send a message to the parent from the child which gets canceled, causing the sync CPOW call to fail.

Setting location off of a timeout bypasses this.

Comment 26

[(no longer active)]

Attachment: Part 9: Initiate the sync CPOW call in browser_bug1045902_console_csp_ignore_reflected_xss_message.js off of a timer

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It would be much better to avoid the use of CPOWs entirely since we want to remove them eventually. In this case, gBrowser.loadURI(TEST_FILE) would work (without the timeout). Also, it would be better to use BrowserTestUtils to wait for the load, since browser.addEventListener also uses CPOWs.

Comment 28

[(no longer active)]

Attachment: Part 9: Avoid using CPOWs in browser_bug1045902_console_csp_ignore_reflected_xss_message.js

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8703190 [details] [diff] [review]
Part 9: Avoid using CPOWs in browser_bug1045902_console_csp_ignore_reflected_xss_message.js

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

Thanks!

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8690108 [details] [diff] [review]
Part 0: Ensure site security service is initialized before trying to use DataStorage via IPC

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

LGTM, but consider the comment below.

::: dom/ipc/ContentParent.cpp
@@ +2677,5 @@
>  ContentParent::RecvReadDataStorageArray(const nsString& aFilename,
>                                          InfallibleTArray<DataStorageItem>* aValues)
>  {
> +    // Ensure the SSS is initialized before we try to use its storage.
> +    nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");

Right now we only have one consumer of DataStorage, so this works as expected. If this ever changes, we'll have to do something like map the filename to which service actually uses it, which feels a bit clunky. I think we can probably cross that bridge if/when we get to it, though.

Also, this will block on reading the backing file. Not sure if that's a concern or not.

Comment 31

[(no longer active)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #30)
> ::: dom/ipc/ContentParent.cpp
> @@ +2677,5 @@
> >  ContentParent::RecvReadDataStorageArray(const nsString& aFilename,
> >                                          InfallibleTArray<DataStorageItem>* aValues)
> >  {
> > +    // Ensure the SSS is initialized before we try to use its storage.
> > +    nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> 
> Right now we only have one consumer of DataStorage, so this works as
> expected. If this ever changes, we'll have to do something like map the
> filename to which service actually uses it, which feels a bit clunky. I
> think we can probably cross that bridge if/when we get to it, though.

Yeah...  I can't think of a clean generic way to do this.  I agree that we should probably cross that bridge when we need to, but the solution would probably end up being something ad-hoc like this again.  :(

> Also, this will block on reading the backing file. Not sure if that's a
> concern or not.

Good question.  Given that the content process needs to block on this being finished, my immediate reaction was to do the simple synchronous thing.  If it turns out to be an issue then we'd need to address it, but I prefer to wait for a real world performance issue, since chances are that the parent process has already initialized this data for its own purposes...

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4fd6615828
https://hg.mozilla.org/integration/mozilla-inbound/rev/52046e843c60
https://hg.mozilla.org/integration/mozilla-inbound/rev/c04efb57b0ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2daa86fc1fb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a59c453192dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/100f8e249007
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba4a5bcea41
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e94d2088219
https://hg.mozilla.org/integration/mozilla-inbound/rev/001b31489756
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f482566235a

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c315a91b6edb for some persistent leaks in m-e10s(bc7) on linux debug:
https://treeherder.mozilla.org/logviewer.html#?job_id=19223852&repo=mozilla-inbound

Comment 34

[(no longer active)]

We rely on profile-before-change to clean up sDataStorages, but that notification is not dispatched in the child process, so we hold all DataStorage objects alive during shutdown.  I believe that should fix the leaks: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab91aa00ee8c>

Comment 35

[(no longer active)]

Attachment: Part 10: Clean up global DataStorage references in the child process

Comment 36

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8703755 [details] [diff] [review]
Part 10: Clean up global DataStorage references in the child process

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

LGTM.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba69c6976af
https://hg.mozilla.org/integration/mozilla-inbound/rev/a32fea83f366
https://hg.mozilla.org/integration/mozilla-inbound/rev/abdbe16f6160
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77772061927
https://hg.mozilla.org/integration/mozilla-inbound/rev/d83763ea1293
https://hg.mozilla.org/integration/mozilla-inbound/rev/59bfdb452ecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/764c7f1cad9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe10782ab28c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bcb515228e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1000838678d
https://hg.mozilla.org/integration/mozilla-inbound/rev/344d31ef4cc6

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ba69c6976af
https://hg.mozilla.org/mozilla-central/rev/a32fea83f366
https://hg.mozilla.org/mozilla-central/rev/abdbe16f6160
https://hg.mozilla.org/mozilla-central/rev/e77772061927
https://hg.mozilla.org/mozilla-central/rev/d83763ea1293
https://hg.mozilla.org/mozilla-central/rev/59bfdb452ecb
https://hg.mozilla.org/mozilla-central/rev/764c7f1cad9e
https://hg.mozilla.org/mozilla-central/rev/fe10782ab28c
https://hg.mozilla.org/mozilla-central/rev/5bcb515228e3
https://hg.mozilla.org/mozilla-central/rev/d1000838678d
https://hg.mozilla.org/mozilla-central/rev/344d31ef4cc6

Comment 39

[Daniel Holbert [:dholbert]]

(historical note/nit: looks like parts 1, 2, 6, and 7 landed with "r=mcmanus" in the commit message, but they really had r=jduell)