Closed Bug 1209081 Opened 9 years ago Closed 8 years ago

Implement "navigate" RequestMode

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: bkelly, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

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.
Depends on: 1201664
Attachment #8708087 - Flags: review?(bkelly)
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.
Attachment #8708087 - Flags: review?(bkelly) → review-
(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.
Flags: needinfo?(bkelly)
(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!
Flags: needinfo?(bkelly)
Attachment #8708087 - Attachment is obsolete: true
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.
Attachment #8709613 - Attachment is obsolete: true
Attachment #8709613 - Flags: review?(bkelly)
Attachment #8709617 - Flags: review?(bkelly) → review+
Attachment #8709614 - Flags: review?(bkelly) → review+
https://hg.mozilla.org/mozilla-central/rev/5eabc5d60f17
https://hg.mozilla.org/mozilla-central/rev/741282f42b24
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.