1209081 - Implement "navigate" RequestMode

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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 1

[(no longer active)]

Attachment: Implement the "navigate" value for RequestMode

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 3

[(no longer active)]

(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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(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!

Comment 5

[(no longer active)]

Attachment: Part 1: Implement the "navigate" value for RequestMode

Comment 6

[(no longer active)]

Attachment: Part 2: Upgrade the saved Requests in the DOM Cache to reflect the "navigate" RequestMode if they represent navigation content policy types

Comment 7

[(no longer active)]

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.

Comment 8

[(no longer active)]

Attachment: Part 1: Implement the "navigate" value for RequestMode

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Chris Mills [:cmills]]

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!