Closed
Bug 1432272
Opened 7 years ago
Closed 7 years ago
fetch API is using GetEntryDocument() for base URL instead of the owning global's base URL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
![]() |
||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27a5cb1decfe504b4b23d9b63cd88d95d40c6614
Assignee | ||
Comment 3•7 years ago
|
||
Try is looking fairly green. All mochitests and WPT have completed on at least some platforms. I'm going to go ahead with flagging.
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
Maybe we should uplift this for webcompat reasons since its the beginning of the 59 beta cycle.
status-firefox58:
--- → wontfix
status-firefox59:
--- → affected
status-firefox60:
--- → affected
status-firefox-esr52:
--- → wontfix
![]() |
||
Comment 6•7 years ago
|
||
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•7 years 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•7 years ago
|
Keywords: dev-doc-needed
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b5432554c7bd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 9•7 years ago
|
||
Eh, lets just ride the trains.
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
(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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•