Closed Bug 1452571 Opened 4 years ago Closed 4 years ago

js::IsBufferSource() is missing |return true| for the DataView case

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: anba, Assigned: luke)

Details

Attachments

(1 file)

As currently written, the return values for the DataView case [1] are simply ignored.

Btw, this [2] seems really unsafe, because it can detach the ArrayBuffer which is not handled by the following code...


[1] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/vm/TypedArrayObject.cpp#2162-2166
[2] https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/shell/js.cpp#5742-5764
Luke, please take a look.
Flags: needinfo?(luke)
Priority: -- → P1
Attached patch fix-bugsSplinter Review
Thanks!
Assignee: nobody → luke
Flags: needinfo?(luke)
Attachment #8969448 - Flags: review?(andrebargull)
Comment on attachment 8969448 [details] [diff] [review]
fix-bugs

Review of attachment 8969448 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! :-)

Btw, are there any plans to explicitly disallow detached ArrayBuffers instead of silently treating them as zero-length ArrayBuffers?
Attachment #8969448 - Flags: review?(andrebargull) → review+
Ah, interesting question.  Looks like Web IDL says to throw:
  https://heycam.github.io/webidl/#es-buffer-source-types
while implementations generally don't:
  https://github.com/heycam/webidl/issues/151
Seems like a good idea to throw though (or at least try to and see if it breaks anything).
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/907f224f35c0
Baldr: fix IsBufferSource on DataView and prevent shell-only rooting bug (r=anba)
https://hg.mozilla.org/mozilla-central/rev/907f224f35c0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.