Closed Bug 1210941 Opened 4 years ago Closed 4 years ago

replace ForceNoIntercept() with an nsLoadFlag to bypass service worker

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 + fixed
firefox44 + fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(13 files, 1 obsolete file)

2.95 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
6.37 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
4.40 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
1.20 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.51 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.24 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.84 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.49 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.70 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.99 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
5.88 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.01 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
6.06 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
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.
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.
Thanks, I'm planning to use 1<<25 in nsIChannel.
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.
Attachment #8669167 - Flags: review?(jduell.mcbugs)
Attachment #8669168 - Flags: review?(jduell.mcbugs)
Attachment #8669169 - Flags: review?(jduell.mcbugs)
Attachment #8669170 - Flags: review?(ehsan)
Attachment #8669172 - Flags: review?(ehsan)
Attachment #8669173 - Flags: review?(ehsan)
Note, idl uuid was updated in P1 already for this bug.
Attachment #8669755 - Flags: review?(jduell.mcbugs)
Attachment #8669170 - Flags: review?(ehsan) → review+
Attachment #8669172 - Flags: review?(ehsan) → review+
Attachment #8669173 - Flags: review?(ehsan) → review+
Attachment #8669750 - Flags: review?(ehsan) → review+
Attachment #8669751 - Flags: review?(ehsan) → review+
Attachment #8669752 - Flags: review?(ehsan) → review+
Attachment #8669753 - Flags: review?(ehsan) → review+
Attachment #8669754 - Flags: review?(ehsan) → review+
The try build is very unhappy.  I obviously borked something in the patches.  Investigating.
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.
Attachment #8669755 - Flags: review?(jduell.mcbugs)
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
Attachment #8669755 - Attachment is obsolete: true
Attachment #8669883 - Flags: review?(jduell.mcbugs)
Attachment #8669167 - Flags: review?(jduell.mcbugs) → review+
Attachment #8669168 - Flags: review?(jduell.mcbugs) → review+
Attachment #8669169 - Flags: review?(jduell.mcbugs) → review+
Attachment #8669756 - Flags: review?(jduell.mcbugs) → review+
Attachment #8669883 - Flags: review?(jduell.mcbugs) → review+
Blocks: 1201740
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
Attachment #8669167 - Flags: approval-mozilla-aurora?
Tracking since this is part of a feature and may need to be backed out or disabled.
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.
Attachment #8669167 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Oops. Parts 1-13. I missed that there was a 13.
You need to log in before you can comment on or make changes to this bug.