The DataView constructor can return DVs backed by detached ArrayBuffers #1025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tor is detached one last time before returning the fresh `DataView` in case creating the fresh `DataView` object incidentally detached the buffer.
littledan
added
needs test262 tests
|
The change makes sense to me, and fits with the rest of the design where we're trying to prevent this sort of thing from happening. |
|
@jswalden would you be able to file a PR to test262 with tests for this change? |
|
Filed tc39/test262#1504 for the test262 change. |
littledan
added
has test262 tests
and removed
needs test262 tests
|
OK, this change seems ready to land then. |
ljharb
approved these changes
Apr 4, 2018
Confirm: this was merged |
bmeck
approved these changes
Jul 18, 2018
bterlson
approved these changes
Jul 18, 2018
ljharb
force-pushed
the
extra-dv-detached-check
branch
from
a246676 to
aeb2026
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two ways this can happen. One, "Let viewByteLength be ? ToIndex(byteLength)." step 9a could detach the already-checked buffer. And two, "Let O be ? OrdinaryCreateFromConstructor(NewTarget, "%DataViewPrototype%", « [[DataView]], [[ViewedArrayBuffer]], [[ByteLength]], [[ByteOffset]] »)." step 10 could do so.
The super-easy fix, that alters the existing order of checks and potential side effects the least, is to doubly check for detachment by duplicating step 5 as a new step 10.5.
A more complicated fix, that would be consistent with how typed arrays handle the similar problem, but that would reorder some of the checking and side effects, would be to move the single
IsDetachedArrayBuffercheck to the end of the algorithm and adjust the other steps' ordering as needed. Finicky, but doable.I don't like creating newborn-and-incompletely-initialized objects, then putting them aside for other operations for a bit, as the typed array constructor algorithm does right now. But avoiding doing that in
DataViewprobably would warrant also doing so for typed arrays, which would mean both algorithms would have to change. That might not be desirable.This PR does the super-easy fix. Is that the fix we want?
This is a followup to both Mozilla bug 1285960 and, it so happens, #852 (comment) as well. (I started from the former and was pointed at the latter.)