Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix updating DataView length when backing buffer is resized #990

Merged
merged 2 commits into from
Mar 23, 2025

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Mar 20, 2025

Fixes: #988

@saghul
Copy link
Contributor Author

saghul commented Mar 20, 2025

Somewhat surprised 262 didn't catch this. Am I being too naive or is this it?

@bnoordhuis
Copy link
Contributor

Yours might be the better fix but I took a different approach in #989 because the ECMA spec language is woolly enough that I'm not sure if what you're doing here is legal.

And no, there are no test262 tests that cover this (or at least, I haven't been able to find them.)

@saghul
Copy link
Contributor Author

saghul commented Mar 20, 2025

Interesting!

If i'm reading this right, with your PR the byteLength won't be updated, right? V8 / JSC seem to update it, when checking with Node / Bun.

@bnoordhuis
Copy link
Contributor

Yeah, and updating is sensible behavior, but common sense never stopped TC39 before, so 🤷

Note how your change breaks a number of DataView tests, although maybe that's simply because they expect RangeErrors instead of a TypeErrors, or vice versa.

@saghul
Copy link
Contributor Author

saghul commented Mar 20, 2025

What's your take? I don't have a strong opinion :-) If you think yours is the better fix I'm happy to close this.

@bnoordhuis
Copy link
Contributor

If you can get the tests to pass, your way is probably better. Aligning with what other engines do would be good.

@saghul
Copy link
Contributor Author

saghul commented Mar 20, 2025

Alright, I'll give that a try!

@saghul
Copy link
Contributor Author

saghul commented Mar 20, 2025

I think I got it. The problem is we need to differentiate between the AUTO byte length and the non-auto case in order to adjust it: https://tc39.es/ecma262/multipage/structured-data.html#sec-dataview-constructor

We currently don't, and updating the length will be dependent on that.

I'll try to get it fixed tonight.

@saghul saghul force-pushed the dataview-update-length branch from 99ef20d to e78334c Compare March 21, 2025 12:22
@saghul
Copy link
Contributor Author

saghul commented Mar 21, 2025

@bnoordhuis Updated, PTAL! I also incorporated your test.

Fixes: #988
Closes: #989
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@saghul saghul force-pushed the dataview-update-length branch from e78334c to 558c4ce Compare March 21, 2025 12:25
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@saghul saghul merged commit 57fe819 into master Mar 23, 2025
128 checks passed
@saghul saghul deleted the dataview-update-length branch March 23, 2025 16:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setUint8 throws when ArrayBuffer is resizable
2 participants