1440705 - Add some more assertions in ClientSource

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

There are a couple places in ClientSource we can add some more assertions to catch potentially bad errors.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make SWM clear the reserved client when performing an STS upgrade with child-process interception. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Separate ClientMatchPrincipalInfo() into a separate method and header. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Assert that a client and its controlling service worker have a matching principal. r=asuth

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

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954501 [details] [diff] [review]
P1 Make SWM clear the reserved client when performing an STS upgrade with child-process interception. r=asuth

Andrew, this patch works around a problem in our child e10s implementation.

When we do an upgrade of the channel URL to https we normally trigger a redirect.  The e10s intercept integration, however, triggers the fetch event *before* the redirect handlers are triggered.  This means ClientChannelHelper does not have a chance to remove the existing controller.

In theory I could have tried to fix child intercept to fire the redirect handlers before dispatching the FetchEvent.  Given we are close to removing this code and its so complication, though, I chose just to work around instead.

My work around is to manually do the clearing of the controller in ServiceWorkerManager when we detect a principal mismatch.  Once parent intercept is enabled by default we can remove this code.

Note, I did some local testing and verified that the final window does not have this origin mismatch problem exposed to script.  It seems to be an internal temporary situation before the window finally loads.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954501 [details] [diff] [review]
P1 Make SWM clear the reserved client when performing an STS upgrade with child-process interception. r=asuth

Hmm.  Actually, I discovered something else unusual here.  Sorry for the flag spam.  Let me look closer.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954501 [details] [diff] [review]
P1 Make SWM clear the reserved client when performing an STS upgrade with child-process interception. r=asuth

Please see comment 4.

What I noticed is that we get two FetchEvents for a navigation request that is upgraded to https.  You can see that this currently happens by:

1. Open https://fetch-event-echo.glitch.me/
2. Run in the console:

var f = document.createElement('iframe'); document.body.appendChild(f); f.src = "./upgrader.html"
var f2 = f.contentDocument.createElement('iframe'); f.contentDocument.body.appendChild(f2); f2.src = "http://fetch-event-echo.glitch.me/empty.html"

3. Observe that two empty.html fetch events are observed.

This only happens in e10s.  I tried a quick fix for this, but it then broke subresource requests.

I'm going to fall back to my work around here for now.  I don't think its worth debugging the e10s interception code at this point.  The multiple fetch events probably explains why the origin mismatch is not observable, though.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954502 [details] [diff] [review]
P2 Separate ClientMatchPrincipalInfo() into a separate method and header. r=asuth

This refactors the MatchPrincipalInfo() method out of ClientManagerService into a separate ClientMatchPrincipalInfo() that can be used in more places.  This will be used in P3.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954503 [details] [diff] [review]
P3 Assert that a client and its controlling service worker have a matching principal. r=asuth

Add a release assertion that the client and controlling service worker principals match.

Comment 9

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

Comment on attachment 8954501 [details] [diff] [review]
P1 Make SWM clear the reserved client when performing an STS upgrade with child-process interception. r=asuth

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

Restating: This is the !NS_SUCCEEDED(CheckSameOrigin(...redirectedChannels...)) from ClientChannelHelper::AsyncOnChannelRedirect branch ported over to deal with super-complicated child-intercept fallout because of how child intercept added STS-upgrades into its mess of permutations.  It will go away when we have right proper e10s handling.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd5d37582ca
P1 Make SWM clear the reserved client when performing an STS upgrade with child-process interception. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5553746a801
P2 Separate ClientMatchPrincipalInfo() into a separate method and header. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/f520860f51c3
P3 Assert that a client and its controlling service worker have a matching principal. r=asuth

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ccd5d37582ca
https://hg.mozilla.org/mozilla-central/rev/f5553746a801
https://hg.mozilla.org/mozilla-central/rev/f520860f51c3