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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment 1 should reference this link instead of the bug URL again:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp?offset=0#488

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Josh Matthews [:jdm]]

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Josh Matthews [:jdm]]

Doesn't sound like I'm a good mentor here.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Its a bug, but not blocking.

Comment 9

[(no longer active)]

Attachment: Avoid using Request's constructor when creating FetchEvent.request

This enables us to avoid a number of checks in Request::Constructor
which don't make sense for exposing FetchEvent.request.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc181e666300

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc181e666300