Service workers should not execute Request::Constructor() when creating FetchEvent

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bkelly, Assigned: Ehsan)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

Currently our service worker code runs the Request::Constructor() when creating a FetchEvent:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3818

The spec actually just says:

  "Let r be a new Request object associated with request."

Using the constructor is problematic because it has all kinds of extra checking to place restrictions on content script.  For example, it throws on URLs with userpass.  We still need to be able to create these Request objects in order to intercept them, though.
This also effects Cache API when reading a Request out of the database:

  https://bugzilla.mozilla.org/show_bug.cgi?id=1201664

We should be able to intercept a URL with userpass, store it in the Cache, and then pull it back out of the Cache.
Oh, actually that path is only hit for putting things into Cache.  Deserializing from Cache does not run the Request::Constructor() again.  Sorry for my confusion.
I'm happy to help somebody solve this! We should have another static method that creates a Response object given a set of arguments, and we can call that from Constructor.
Mentor: josh
Whiteboard: [lang=c++]
I think getting the right restrictions on the Request might be a bit tricky.  It's not clear to me what parts of the constructor we were relying on and its not really obvious from the spec.
Note, we also already have a lot of the code necessary to do this.  The SWM can create an InternalRequest to represent the channel and then just do |new Request(global, internalRequest)|.  This is what Cache currently does:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#367

So really we just need a ToInternalRequestFromChannel(nsIChannel) method.
Doesn't sound like I'm a good mentor here.
Mentor: josh
Whiteboard: [lang=c++]
Assignee

Updated

4 years ago
Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-postv1
Assignee: nobody → josh
Its a bug, but not blocking.
Blocks: ServiceWorkers-postv1
No longer blocks: ServiceWorkers-v1
Assignee

Updated

4 years ago
Assignee: josh → ehsan
Assignee

Updated

4 years ago
Blocks: 1209081
Assignee

Comment 9

4 years ago
This enables us to avoid a number of checks in Request::Constructor
which don't make sense for exposing FetchEvent.request.
Attachment #8707661 - Flags: review?(bkelly)
Comment on attachment 8707661 [details] [diff] [review]
Avoid using Request's constructor when creating FetchEvent.request

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

I'm slightly worried there are invariants in the fetch spec assumed for non-constructor Request objects that we may not capture here.  But the fetch spec does not lay them out in a single place like it does for the constructor.  I guess we must rely on our tests for now.  r=me
Attachment #8707661 - Flags: review?(bkelly) → review+

Comment 12

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc181e666300
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1251229
You need to log in before you can comment on or make changes to this bug.