Closed Bug 1440705 Opened 4 years ago Closed 4 years ago

Add some more assertions in ClientSource

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(3 files)

There are a couple places in ClientSource we can add some more assertions to catch potentially bad errors.
Priority: -- → P2
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.
Attachment #8954501 - Flags: review?(bugmail)
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.
Attachment #8954501 - Flags: review?(bugmail)
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.
Attachment #8954501 - Flags: review?(bugmail)
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.
Attachment #8954502 - Flags: review?(bugmail)
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.
Attachment #8954503 - Flags: review?(bugmail)
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.
Attachment #8954501 - Flags: review?(bugmail) → review+
Attachment #8954502 - Flags: review?(bugmail) → review+
Attachment #8954503 - Flags: review?(bugmail) → review+
Blocks: 1441932
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ccd5d37582ca
https://hg.mozilla.org/mozilla-central/rev/f5553746a801
https://hg.mozilla.org/mozilla-central/rev/f520860f51c3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1462077
You need to log in before you can comment on or make changes to this bug.