The recent integration of fetch into HTML revealed an issue with using "same-origin" RequestMode for navigations. There is now a separate "navigate" RequestMode instead. See: https://github.com/whatwg/fetch/issues/126 I'm marking this v3 since I believe our current implementation is ok for v1. We don't have the spec issue with "same-origin" RequestMode because we don't really force the origin header like the spec does. We can fix this later.

Comment on attachment 8708087 [details] [diff] [review] Implement the "navigate" value for RequestMode Review of attachment 8708087 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, but I think you need to deal with the casting of RequestMode to the nsIHttpChannelInternal's CorsMode value: https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#322 https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp?from=InternalRequest.cpp#321 Its possible this value is never read today, but we're still setting a bogus enum value. I say it may never be read because: 1) We can't re-intercept a fetch(evt.request) from a navigation fetch event. 2) There is no other way to get a channel with a navigation mode, but a non-navigation content policy. Or do we propagate the content policy for fetch(evt.request) as well? I'd like to review it again after updating the CorsMode enum. ::: dom/cache/DBSchema.cpp @@ +192,5 @@ > static_assert(int(RequestMode::Same_origin) == 0 && > int(RequestMode::No_cors) == 1 && > int(RequestMode::Cors) == 2 && > + int(RequestMode::Navigate) == 3 && > + int(RequestMode::EndGuard_) == 4, We don't need a migration because this is backward compatible, right? ::: dom/webidl/Request.webidl @@ +54,5 @@ > "sharedworker", "subresource", "style", "track", "video", "worker", "xmlhttprequest", > "xslt" > }; > > +enum RequestMode { "same-origin", "no-cors", "cors", "navigate" }; Does it matter to you this is not a direct copy of the spec webidl? It has the order differently. I know we have it this way, though, because of our enum values.

(In reply to Ben Kelly [:bkelly] from comment #2) > Comment on attachment 8708087 [details] [diff] [review] > Implement the "navigate" value for RequestMode > > Review of attachment 8708087 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, but I think you need to deal with the casting of RequestMode to the > nsIHttpChannelInternal's CorsMode value: > > > https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#322 > > https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest. > cpp?from=InternalRequest.cpp#321 > > Its possible this value is never read today, but we're still setting a bogus > enum value. I say it may never be read because: > > 1) We can't re-intercept a fetch(evt.request) from a navigation fetch event. > 2) There is no other way to get a channel with a navigation mode, but a > non-navigation content policy. Or do we propagate the content policy for > fetch(evt.request) as well? > > I'd like to review it again after updating the CorsMode enum. Fair. I think what we should do is add a CORS_MODE_NAVIGATE, and MOZ_ASSERT in the above two places that we're not observing that value. Does that sound good? > ::: dom/cache/DBSchema.cpp > @@ +192,5 @@ > > static_assert(int(RequestMode::Same_origin) == 0 && > > int(RequestMode::No_cors) == 1 && > > int(RequestMode::Cors) == 2 && > > + int(RequestMode::Navigate) == 3 && > > + int(RequestMode::EndGuard_) == 4, > > We don't need a migration because this is backward compatible, right? Hmm. I originally didn't do a migration because I thought we can't differentiate between "same-origin" and "navigate", but thinking about this more carefully, we can definitely look at the request_contentpolicytype field and update the request_mode to 3 for navigation content policy types! That would mean that if you stored such a Request in the Cache, when you read it again, its mode will be "navigate". What do you think? > ::: dom/webidl/Request.webidl > @@ +54,5 @@ > > "sharedworker", "subresource", "style", "track", "video", "worker", "xmlhttprequest", > > "xslt" > > }; > > > > +enum RequestMode { "same-origin", "no-cors", "cors", "navigate" }; > > Does it matter to you this is not a direct copy of the spec webidl? It has > the order differently. I know we have it this way, though, because of our > enum values. I think in this case it's OK to order things differently (the order is not observable from Web content anyways.) But perhaps I should add a comment explaining the difference in the ordering.

(In reply to :Ehsan Akhgari from comment #3) > Hmm. I originally didn't do a migration because I thought we can't > differentiate between "same-origin" and "navigate", but thinking about this > more carefully, we can definitely look at the request_contentpolicytype > field and update the request_mode to 3 for navigation content policy types! > That would mean that if you stored such a Request in the Cache, when you > read it again, its mode will be "navigate". > > What do you think? On the one hand it seems like overkill. On the other hand it might be necessary so we can lock down asserts other places. I guess we should do it. I don't mind the changing value because people are going to have to deal with the new value from FetchEvent anyway. Thanks!

Hmm, the assertion in FetchDriver::HttpFetch() is actually wrong, since we can totally observe a navigate request mode being passed to fetch(). All you need to do is to call fetch(ev.request) from a fetch event handler.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5eabc5d60f17 https://hg.mozilla.org/integration/mozilla-inbound/rev/741282f42b24

https://hg.mozilla.org/mozilla-central/rev/5eabc5d60f17 https://hg.mozilla.org/mozilla-central/rev/741282f42b24

I've updated the docs to mention and explain this new mode value: https://developer.mozilla.org/en-US/docs/Web/API/Request https://developer.mozilla.org/en-US/docs/Web/API/Request/Request https://developer.mozilla.org/en-US/docs/Web/API/Request/mode And added a note to the release notes: https://developer.mozilla.org/en-US/Firefox/Releases/46#Fetch Let me know if these are ok. Thanks!