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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: fetch-base.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27a5cb1decfe504b4b23d9b63cd88d95d40c6614

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Try is looking fairly green.  All mochitests and WPT have completed on at least some platforms.  I'm going to go ahead with flagging.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Maybe we should uplift this for webcompat reasons since its the beginning of the 59 beta cycle.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Dorel Luca [:dluca]]

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Eh, lets just ride the trains.

Comment 10

[Chris Mills [:cmills]]

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!

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 12

[Chris Mills [:cmills]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Chris Mills [:cmills]]

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