Skip to content

fetch() is unclear on which realm's Request it uses #777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
domenic opened this issue Jul 12, 2018 · 11 comments
Open

fetch() is unclear on which realm's Request it uses #777

domenic opened this issue Jul 12, 2018 · 11 comments
Labels
clarification Standard could be clearer

Comments

@domenic
Copy link
Member

domenic commented Jul 12, 2018

https://fetch.spec.whatwg.org/commit-snapshots/e6cbef27724dd6111d1646898ef4f3f9ad56740b/#fetch-method

Let requestObject be the result of invoking the initial value of Request as constructor with input and init as arguments. If this throws an exception, reject p with it and return p

Which realm does the Request constructor come from?

I think we want it to be the relevant realm of this WindowOrWorkerGlobalScope object. That seems most in line with our plan in whatwg/webidl#135, although that is about slightly different phrasing ("a new Response" vs. this line's "invoking the initial value of Request as constructor").

BTW, editorial issues with this sentence:

  • "as constructor" -> "as a constructor"
  • Missing period
  • "Request" links to the class, but should probably link to the constructor.

Found by @benjamingr and @TimothyGu in IRC: https://freenode.logbot.info/whatwg/20180712#c1621394

(BTW, @TimothyGu found another good thing to test: what if you create a Request from another window, and pass it to fetch()? What client does it use? Some inspection of Chrome's source code reveals it may not be using the other window, but that's what the spec says should happen.)

@TimothyGu
Copy link
Member

TimothyGu commented Jul 13, 2018

(For those who don't want to read IRC logs, all of this is observable through the Referer header and using the fetch() function from an iframe for example.)

We probably don't want to use the word "invoking" here at all, since it can mean multiple things: the ES Call abstract operation; invoke a callback function algorithm (this is probably the closest, since it contains logic to convert IDL arguments to back ES, even though Request is not a callback function in the IDL sense); or just "execute the steps listed for that constructor"?

I think we want it to be the relevant realm of this WindowOrWorkerGlobalScope object.

This would cause a slight discrepancy between fetch() and Request(). The Request() constructor explicitly uses the current settings object as the client of the new request it's creating; i.e., the realm for which the Request function was created.

Edit: Hmm, I guess this discrepancy is fine, since if we pass a Request object to fetch() it'll recreate the request anyway, so a circumstance where fetch.call(otherWindow, "url") and fetch.call(otherWindow, new Request("url")) are different should never happen.

I think we would want more testing before deciding if we want toshould use the relevant realm or the current realm, but philosophically the question really is "do we want to make fetch() in a realm bound to that realm?"

@domenic
Copy link
Member Author

domenic commented Jul 13, 2018

I mean, it has to be bound to some realm. And we generally want to pick the relevant realm of this, for methods. (For constructors the "relevant realm of this" is not super well-defined, since the this object is still being created, and is derived from the realm of new.target, which is usually-but-not-with-Reflect.construct the current realm.)

@TimothyGu
Copy link
Member

TimothyGu commented Jul 13, 2018

Right, I see.

This kind of spawns another issue though. Currently in Chrome and Firefox fetch.call(otherWindow, 'url') actually returns an instance of otherWindow.Promise that resolves to an instance of otherWindow.Response, even though the fetch belongs to the current window, but not in Safari. I feel like Chrome and Firefox’s behavior is quite unexpected.

@domenic
Copy link
Member Author

domenic commented Jul 13, 2018

Chrome and Firefox's behavior is the desired one. This is a clear-cut instance of whatwg/webidl#135 (triggered by "let p be a new promise").

More information on why at https://html.spec.whatwg.org/#realms-settings-objects-global-objects, starting with "In general, web platform specifications should use..."

@annevk annevk added the clarification Standard could be clearer label Jul 31, 2018
@annevk
Copy link
Member

annevk commented Aug 27, 2018

@jakearchibald @wanderview and I discovered that implementations might not follow this for the Request constructor steps either, which typically say to use the "current settings object". Instead, if you do something like frame.contentWindow.fetch(...), the service worker of frame ends up being used, rather than of the parent which did the invocation. So maybe most of these need to be changed to "relevant".

@annevk
Copy link
Member

annevk commented Jul 15, 2020

@rwlbuis this might be another interesting WebKit bug. I put some initial tests at web-platform-tests/wpt#24601. I need #1054 merged before working on specification changes though.

@rwlbuis
Copy link

rwlbuis commented Jul 15, 2020

@rwlbuis this might be another interesting WebKit bug. I put some initial tests at web-platform-tests/wpt#24601. I need #1054 merged before working on specification changes though.

Yes it may well be. Are you going to file bugs for the implementations?

@annevk
Copy link
Member

annevk commented Jul 15, 2020

Yeah, before committing the change to Fetch bugs will be filed.

@annevk
Copy link
Member

annevk commented Jul 15, 2020

@domenic @TimothyGu does this mean https://infra.spec.whatwg.org/#parse-json-from-bytes needs to accept a realm?

@annevk
Copy link
Member

annevk commented Jul 15, 2020

See also #400, #730, #731, #825, and #826.

Some other tests (already landed): web-platform-tests/wpt#13850.

@domenic
Copy link
Member Author

domenic commented Jul 15, 2020

@domenic @TimothyGu does this mean https://infra.spec.whatwg.org/#parse-json-from-bytes needs to accept a realm?

I suppose so. It currently always uses the current Realm, because that's what ECMAScript always does. But if we want the web platform to continue its inconsistency with ECMAScript (which was reaffirmed semi-recently in whatwg/webidl#135 (comment)), then we'll need to make that overridable.

annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 1, 2021
annevk added a commit that referenced this issue Feb 2, 2021
This also:

* Clarifies some Realms, but more needs to be done in #777 and other issues.
* Ensures that Headers's header list is set correctly for Response.error() and redirect().

Closes #771.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer
Development

No branches or pull requests

4 participants