Closed Bug 1264941 Opened 4 years ago Closed 4 years ago

byteLength calculation for ArrayBuffer object in TypedArray(typedArray) differs between lazily allocated case and eagerly allocated case.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: arai, Assigned: arai)

Details

Attachments

(1 file)

Code:
  var abuf = new ArrayBuffer(16);
  var a1 = new Float64Array(abuf, 0, 0);
  var a2 = new Float64Array(a1);
  a2.buffer.byteLength

Expected output
  16

Actual output
  0

a2.buffer is created lazily with a2.byteLength, but a2.byteLength can be different than expected a2.buffer.byteLength.

https://tc39.github.io/ecma262/#sec-clonearraybuffer
> 4. Let srcLength be the value of srcBuffer's [[ArrayBufferByteLength]] internal slot.
> 5. Assert: srcByteOffset ≤ srcLength.
> 6. Let cloneLength be srcLength - srcByteOffset.
> 7. Let srcBlock be the value of srcBuffer's [[ArrayBufferData]] internal slot.
> 8. Let targetBuffer be ? AllocateArrayBuffer(cloneConstructor, cloneLength).

srcLength is 16, and srcByteOffset is 0, so cloneLength should be 16, even if that area is not used by owning TypedArray instance.
The computation of cloneLength is actually considered a bug (even Allen thinks so), if I read this correctly: https://github.com/tc39/ecma262/issues/447
Ah, in that case what we should track is the behavior difference between lazily allocated case and eagerly allocated case.

  // lazily allocated case
  var abuf1 = new ArrayBuffer(16);
  var a1 = new Float64Array(abuf1, 0, 0);
  var a2 = new Float64Array(a1);
  print(a2.buffer.byteLength); // 0

  // eagerly allocated case
  var abuf2 = new ArrayBuffer(128);
  var a3 = new Float64Array(abuf2, 0, 0);
  var a4 = new Float64Array(a3);
  print(a4.buffer.byteLength); // 128

either one should be fixed to follow the fixed spec.
So, eagerly allocated case follows current spec (cloneLength = srcLength - srcByteOffset),
and lazily allocated case doesn't, and maybe it's the expected behavior for the *fixed* spec in https://github.com/tc39/ecma262/issues/447 .
Summary: ArrayBuffer object is created with less capacity than expected in TypedArray(typedArray) → byteLength calculation for ArrayBuffer object in TypedArray(typedArray) differs between lazily allocated case and eagerly allocated case.
I don't suppose the patch in bug 1255128 fixes this, does it?

I need to land it, but we have some browser tests that expect the nonstandard constructor behavior.
The code change in bug 1255128 have no effect on this behavior.

https://github.com/tc39/ecma262/issues/447 is closed with following commit
  https://github.com/tc39/ecma262/commit/e1cecfa320879912701fddf7d317f4636bd10457
so we should fix the eagerly allocated case to use the byteLength of typedArray.
CloneArrayBuffer is used also from %TypedArray%.prototype.set, but it uses totally different codepath, and doesn't call our CloneArrayBufferNoCopy function.
So I'd like to leave that part to another bug.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0d773b00cb8
Assignee: nobody → arai.unmht
Attachment #8742877 - Flags: review?(lhansen)
Comment on attachment 8742877 [details] [diff] [review]
Use byteLength of source typedArray in CloneArrayBuffer.

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

::: js/src/tests/ecma_6/ArrayBuffer/CloneArrayBuffer.js
@@ +2,5 @@
> +var summary = 'CloneArrayBuffer should be called with byteLength of source typedArray';
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +function test(ctor, byteLength, offsets) {

"offsets" is unused.
Attachment #8742877 - Flags: review?(lhansen) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e92704fae381eb6b3363a87372663c5a6466e6fd
Bug 1264941 - Use byteLength of source typedArray in CloneArrayBuffer. r=lth
https://hg.mozilla.org/mozilla-central/rev/e92704fae381
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.