fetch API is using GetEntryDocument() for base URL instead of the owning global's base URL

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We currently are failing a number of WPT tests dom/fetch is using GetEntryDocument() to find a base URL instead owning global's base URL.  For example, these tests explicitly test for this:

https://github.com/w3c/web-platform-tests/blob/master/fetch/api/request/multi-globals/url-parsing.html

The spec says to use "current settings object's API base URL" in step 4 here:

https://github.com/w3c/web-platform-tests/blob/master/fetch/api/request/multi-globals/url-parsing.html

Its unclear to me if that is correct spec language for this.  It seems like "relevant settings object" may be more appropriate:

https://html.spec.whatwg.org/multipage/webappapis.html#relevant-settings-object

But I'm not 100% clear on the spec language here.

Boris, I intent to flag you for review since you were reviewer on the change to use GetEntryDocument() previously and tend to have opinions on this stuff.  Or feel free to pass and I'll find someone else.  Thanks
(Assignee)

Updated

a year ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Oh, boy.  ;)

So there are a bunch of things going on here, including strong disagreements about how this stuff would work between different people writing specs.

1) Historically (think 90s), base URLs typically came from what we now call
   the "entry settings object".  This is, for example, how Location works.
2) There are some people who have a project of killing off the entry settings object.
   This involves not using it for base URLs in new APIs, and trying to change existing
   APIs to use something else.  See https://github.com/whatwg/html/issues/1431
3) This, of course, leads to inconsistencies in how different APIs derive the base URL, which can
   lead to developer confusion.  Thus one could make the argument that the effort described in
   point 2 is misguided, unless we think we can switch _everything_ off entry settings for base URLs.
4) The use of entry settings for base URLs is slightly daft in some ways, because
   it leads to bizarre action-at-a-distance effects.  If one were designing the
   web platform from scratch and actually thinking about this problem, one would
   never choose entry settings for base URLs.  This is the impetus for the effort from point 2.
5) For the specific case of the fetch API, it used to use entry settings, but that got changed
   in https://github.com/whatwg/fetch/issues/367 as far as I can tell.  This predated
   the "file bugs on UAs" policy, I think.

I assume your second link was meant to be to <https://fetch.spec.whatwg.org/#dom-request>, where step 4 uses "current settings object"?  If so, that should in fact be current settings, because there is no meaningful "relevant settings" in a constructor.  That said, the "current settings" of the constructor becomes the "relevant settings" of the object returned from the constructor, and is the "owning global" of the to-be-returned thing in terms of our code.

TL;DR: implementing what the spec currently says here makes sense, I think.
Try is looking fairly green.  All mochitests and WPT have completed on at least some platforms.  I'm going to go ahead with flagging.
Commit message is:

Bug 1432272 Make Fetch API use the global's base URL instead of the entry document's base URL. r=bz

The fetch spec used to use the entry settings as the base for parsing relative
Request/Response URL's, but this is no longer the case.  This was changed in:

https://github.com/whatwg/fetch/issues/367

Update our code to match this behavior.  We basically convert GetEntryDocument()
to QI the global to nsGlobalWindowInner and use its ExtantDoc instead.

No changes are needed for workers since its not possible to perform cross-global
javascript access in worker threads
Attachment #8944529 - Attachment is obsolete: true
Attachment #8944558 - Flags: review?(bzbarsky)
Maybe we should uplift this for webcompat reasons since its the beginning of the 59 beta cycle.
Comment on attachment 8944558 [details] [diff] [review]
Make Fetch API use the global's base URL instead of the entry document's base URL. r=bz

r=me

Once we have a sane threadsafe URL implementation, ideally we would just have an "api base url" hanging off of nsIGlobalObject and common up the worker and non-worker paths...  Might be worth filing a followup on that pointing to this code and depending on the threadsafe URL bugs.
Attachment #8944558 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

a year ago
See Also: → 1432481

Comment 7

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5432554c7bd
Make Fetch API use the global's base URL instead of the entry document's base URL. r=bz
(Assignee)

Updated

a year ago
Keywords: dev-doc-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5432554c7bd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Eh, lets just ride the trains.
I'm not really sure what to say about this in the docs. This is a very spec-centric point, and I don't really understand how this would affect web developers, and how to phrase this information. Ben, can you give me a bit of help here?

Thanks in advance!
Flags: needinfo?(bkelly)
Chris, its an esoteric point that perhaps most sites will never run into.  To run into this:

1. You need a same-origin iframe.
2. That same-origin iframe needs to have a location with a different base URL.
3. You have to use the fetch function cross-global like `frame.contentWindow.fetch()`.
4. The URL passed to fetch needs to be relative.

In the past we would resolve the relative URL against the current global.  So like:

  let absolute = new URL(relative, window.location.href)

Now we resolve the relative URL against the global that owns the fetch() function being used.  So in the case I describe above, its resolved against the iframe's location.  So like:

  let absolute = new URL(relative, frame.contentWindow.location.href)

I'm not sure its really worth putting all of this in the fetch() documentation.  Its almost worth having some page dedicated to the corner case of cross-global usage, although I'm not sure APIs are consistent in how they handle this stuff.  The spec is trying to align new APIs to what we did in this bug.

Hope that helps.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #11)
> 
> Hope that helps.

Thanks Ben, this is really useful. I've had a go at writing this into a short article:

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Cross-global_fetch_usage

I've let a few placeholders in where I think we could probably use more information. Can you help me fill it in?

Thanks again.
Flags: needinfo?(bkelly)
I guess this page is ok.  I'm not sure there is a particular problem the change fixes.  Its more just that we're trying to be consistent in the spec for this kind of thing now and this is the behavior we decided to go with.  So the change was basically to conform to the new convention we are using in the spec.

Honestly, a separate "cross global issues" page on MDN would probably be helpful.  It could talk about this issue and probably more.  Its an area with a lot of inconsistencies between APIs and browsers, I think.

I don't feel knowledgeable enough to write that page, though.  You might need to talk to Boris about it.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #13)
> I guess this page is ok.  I'm not sure there is a particular problem the
> change fixes.  Its more just that we're trying to be consistent in the spec
> for this kind of thing now and this is the behavior we decided to go with. 
> So the change was basically to conform to the new convention we are using in
> the spec.
> 
> Honestly, a separate "cross global issues" page on MDN would probably be
> helpful.  It could talk about this issue and probably more.  Its an area
> with a lot of inconsistencies between APIs and browsers, I think.
> 
> I don't feel knowledgeable enough to write that page, though.  You might
> need to talk to Boris about it.

OK; I've updated it just to say that. And I'll talk to Boris about the cross global issues suggestion.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.