Skip to content
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

Editorial: Fix realmless ArrayBuffer creation #1751

Merged
merged 3 commits into from
May 7, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented May 4, 2024

Fixes #1675, maybe.

@annevk in that issue also points out that the extract a body [...] algorithm also creates a Uint8Array in an ad-hoc way, which I've also tried to fix. Unfortunately I don't know what realm to use there - unlike the original issue, these aren't method steps. I am guessing that when the realm is unspecified (which it very often isn't - for example, earlier in the same algorithm there is "set stream to a new ReadableStream object" without a realm argument), it's implicitly the current realm? But maybe we need to thread a realm argument into this algorithm (and quite a few others), or possibly just explicitly pass "the current realm".

(If this right I'll follow up with a PR for #1732.)


Preview | Diff

@annevk
Copy link
Member

annevk commented May 6, 2024

This looks correct, but I wonder if we have test coverage for this.

It also needs some wrapping adjustment, but happy to push a fix for that myself before landing.

The longer term plan is to fix this more generally through Web IDL by making an ambient realm available to be used for creating (most) new objects.

@bakkot
Copy link
Contributor Author

bakkot commented May 6, 2024

This looks correct, but I wonder if we have test coverage for this.

There are no tests for the realm of created objects at all, that I can find. I could add some but I'd be inventing the conventions.

The longer term plan is to fix this more generally through Web IDL by making an ambient realm available to be used for creating (most) new objects.

Presumably that would apply to the first hunk (the one in extract a body with type), but not the second (the one in arrayBuffer()), right? Since the ambient realm is not the one used in the second case. (Specifically, if you do await firstRealm.Response.prototype.arrayBuffer.call(new secondRealm.Response('asdf')), the resulting ArrayBuffer comes from the second realm, not the first, i.e. it is derived from the realm of this rather than the realm of the arrayBuffer function.)

@annevk
Copy link
Member

annevk commented May 6, 2024

Well, "ambient" was doing a lot of work there. The idea would be for Web IDL to handle these kind of cases correctly so nobody has to think about them again.

If you could add a minimal window.js test I think inventing the conventions is fine?

@bakkot
Copy link
Contributor Author

bakkot commented May 6, 2024

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request May 7, 2024
@annevk annevk merged commit 5d9d67b into whatwg:main May 7, 2024
2 checks passed
@bakkot bakkot deleted the fix-realmless-arraybuffer branch May 7, 2024 14:40
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 14, 2024
… arrayBuffer(), a=testonly

Automatic update from web-platform-tests
Fetch: add test for realm of result of Response's arrayBuffer()

For whatwg/fetch#1751.
--

wpt-commits: ef64e9e98aff5f530368ee2213b01b5fe2acc2ec
wpt-pr: 46104
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request May 15, 2024
… arrayBuffer(), a=testonly

Automatic update from web-platform-tests
Fetch: add test for realm of result of Response's arrayBuffer()

For whatwg/fetch#1751.
--

wpt-commits: ef64e9e98aff5f530368ee2213b01b5fe2acc2ec
wpt-pr: 46104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

“Realmless” ArrayBuffer creation in Body’s arrayBuffer() steps
2 participants