1210941 - replace ForceNoIntercept() with an nsLoadFlag to bypass service worker

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we have nsIHttpChannelInternal::ForceNoIntercept() to skip service worker interception of a channel.

This has worked ok until now, but its problematic.  If the decision to bypass the service worker is disconnected at all from where the channel is created, then we must pass a new policy variable through the code to conditionally call ForceNoIntercept().

If we had an nsLoadFlag for bypassing the service worker, however, we would generally not have to modify much code.  Almost all channel creating code accepts nsLoadFlags as an argument.

Therefore, I think we should create a LOAD_BYPASS_SERVICE_WORKER flag and remove ForceNoIntercept().  This will make the code cleaner by using the existing mechanisms in the channel API.

Comment 1

[Jason Duell]

As discussed on IRC, we're low on bits for new flags, but we still have 1<<1 through 1<<5 in nsIRequest and 1<<25 in nsIChannel, so I've given bkelly my +f to use a bit for this.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Thanks, I'm planning to use 1<<25 in nsIChannel.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Create the LOAD_BYPASS_SERVICE_WORKER flag. r=jduell

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Use LOAD_BYPASS_SERVICE_WORKER in HttpBaseChannel instead of mForceNoIntercept. r=jduell

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make jar channels use LOAD_BYPASS_SERVICE_WORKER internally. r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use LOAD_BYPASS_SERVICE_WORKER instead of ForceNoIntercept in nsDocShell. r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Use LOAD_BYPASS_SERVICE_WORKER in nsObjectLoadingContent instead of ForceNoIntercept(). r=ehsan

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use LOAD_BYPASS_SERVICE_WORKER instead of ForceNoIntercept in FetchDriver. r=ehsan

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Patches are work-in-progress.  Still need to fix:

 dom/workers/ScriptLoader.cpp
 dom/workers/ServiceWorkerScriptCache.cpp
 dom/xslt/base/txURIUtils.cpp
 netwerk/protocol/http/nsCORSListenerProxy.cpp
 security/manager/ssl/nsNSSCallbacks.cpp

And then de-orbit the ForceNoIntercept() method itself.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Use LOAD_BYPASS_SERVICE_WORKER in ServiceWorkerScriptCache. r=ehsan

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Use LOAD_BYPASS_SERVICE_WORKER in xslt txURIUtils. r=ehsan

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Use LOAD_BYPASS_SERVICE_WORKER in nsCORSListenerProxy. r=ehsan

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Use LOAD_BYPASS_SERVICE_WORKER in nsNSSCallbacks. r=ehsan

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Use LOAD_BYPASS_SERVICE_WORKER in worker ScriptLoader. r=ehsan

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Remove http channel's ForceNoIntercept. r=jduell

Note, idl uuid was updated in P1 already for this bug.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Remove ForceNoIntercept from jar channel. r=jduell

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

The try build is very unhappy.  I obviously borked something in the patches.  Investigating.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8669755 [details] [diff] [review]
P12 Remove http channel's ForceNoIntercept. r=jduell

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

The problem is with this patch.  Fixed version up shortly.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 Remove http channel's ForceNoIntercept. r=jduell

There was a bug in how I was setting the load flag in the redirect case.  This updates to fix that.  The tests that were failing now pass locally.

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

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95357a330321
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b44bb42ae5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9525edea379d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b51bece75129
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd2b8a58acc
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4f02b05213
https://hg.mozilla.org/integration/mozilla-inbound/rev/6532d08e71c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9184e91f177d
https://hg.mozilla.org/integration/mozilla-inbound/rev/783ef9cb5802
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6376757d9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3945685c83e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/581fec38b1d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/89416f2918ed

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8669167 [details] [diff] [review]
P1 Create the LOAD_BYPASS_SERVICE_WORKER flag. r=jduell

This request is for all 13 patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: service workers
[User impact if declined]: This bug is necessary to uplift further service worker work to 43 aurora.
[Describe test coverage new/current, TreeHerder]: Many existing mochitests and web-platform-tests cover this feature.
[Risks and why]: Risk limited to service workers only.  No other feature uses the ForceNoIntercept() API.
[String/UUID change made/needed]: None

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/95357a330321
https://hg.mozilla.org/mozilla-central/rev/8b44bb42ae5c
https://hg.mozilla.org/mozilla-central/rev/9525edea379d
https://hg.mozilla.org/mozilla-central/rev/b51bece75129
https://hg.mozilla.org/mozilla-central/rev/1bd2b8a58acc
https://hg.mozilla.org/mozilla-central/rev/6d4f02b05213
https://hg.mozilla.org/mozilla-central/rev/6532d08e71c6
https://hg.mozilla.org/mozilla-central/rev/9184e91f177d
https://hg.mozilla.org/mozilla-central/rev/783ef9cb5802
https://hg.mozilla.org/mozilla-central/rev/ac6376757d9c
https://hg.mozilla.org/mozilla-central/rev/3945685c83e3
https://hg.mozilla.org/mozilla-central/rev/581fec38b1d9
https://hg.mozilla.org/mozilla-central/rev/89416f2918ed

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is part of a feature and may need to be backed out or disabled.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8669167 [details] [diff] [review]
P1 Create the LOAD_BYPASS_SERVICE_WORKER flag. r=jduell

Uplifting parts 1-12 since we're aiming to release the feature in 43.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Oops. Parts 1-13. I missed that there was a 13.

Comment 27

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

There were a few conflicts on the uplift, but I think I dealt with them correctly:
http://hg.mozilla.org/releases/mozilla-aurora/7f44441192a0
http://hg.mozilla.org/releases/mozilla-aurora/204704fd78e0
http://hg.mozilla.org/releases/mozilla-aurora/80dc74df7add
http://hg.mozilla.org/releases/mozilla-aurora/3009a16d818b
http://hg.mozilla.org/releases/mozilla-aurora/d523fa9b2ac1
http://hg.mozilla.org/releases/mozilla-aurora/6c4ddce7e2b2
http://hg.mozilla.org/releases/mozilla-aurora/d75b303dd499
http://hg.mozilla.org/releases/mozilla-aurora/f023556d31e7
http://hg.mozilla.org/releases/mozilla-aurora/6332cd31df15
http://hg.mozilla.org/releases/mozilla-aurora/035593bb1fdf
http://hg.mozilla.org/releases/mozilla-aurora/e11cf29e0820
http://hg.mozilla.org/releases/mozilla-aurora/b75ce95a21fc
http://hg.mozilla.org/releases/mozilla-aurora/e0532d91a2f9