Closed Bug 1040402 Opened 6 years ago Closed 3 years ago

incorrect handling of 'undefined' length arg in typed array view constructor

Categories

(Core :: JavaScript: Standard Library, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: luke, Assigned: jandem, Mentored)

Details

Attachments

(1 file, 1 obsolete file)

If I'm reading 22.2.1.4 correctly, there isn't a difference between passing <3 arguments and explicitly passing 'undefined' as the 3rd (length) argument.  Yet:

js> var ab = new ArrayBuffer(16)
js> new Int32Array(ab).length
4
js> new Int32Array(ab, undefined, undefined).length
0

Other browsers seem to get this right.  Credit to :kael for hitting and tracking this down.
Component: JavaScript Engine → JavaScript: Standard Library
Looks an easy fix, someone just needs to use (I think?) hasDefined(2) instead of args.length() direct comparison.
Mentor: jwalden+bmo
Hello, i would like to work on this bug where can i find the code for it.
Probably in js/src/vm/TypedArrayObject.{cpp,h}, around about makeInstance(), IIRC.
I tried to follow the spec, but I have to say that I'm very unsure about the relationship between spec(ToIndex), code(ToIntegerIndex), and code(ToInt32).
Attachment #8772609 - Flags: review?(jwalden+bmo)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8772609 [details] [diff] [review]
Treat undefined argument the same as not passing the argument for typed array constructor

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

Punting this for one that uses a new ToIndex function (with a subsequent, not-in-spec check for our own implementation limits on typed array length), rather than open-coding it hairily.
Attachment #8772609 - Flags: review?(jwalden+bmo)
Just want to mention this still happens, and violates 22.2.1.5 section 13 (http://www.ecma-international.org/ecma-262/6.0/#sec-%typedarray%-buffer-byteoffset-length).

Took me quite a while to figure why my perfectly valid code that works on Chrome, fails for no reason on Firefox.
By the way, I have no clue how the implementation side of Firefox looks, but this is all it takes on the client to fix this:

byteOffset = byteOffset || 0;
if (byteLength === undefined) {
    byteLength = buffer.byteLength - byteOffset;
}
This is breaking real code, let's get this fixed in 53 (and maybe backport to ESR 52 as the patch will be simple).
Flags: needinfo?(sphink)
Priority: -- → P1
Attached patch PatchSplinter Review
This is just the simplest fix possible: use args.hasDefined(x) instead of args.length().
Assignee: sphink → jdemooij
Attachment #8772609 - Attachment is obsolete: true
Flags: needinfo?(sphink)
Attachment #8812216 - Flags: review?(andrebargull)
FWIW, I do think it would be nice to add ToIndex, add spec steps, etc, but I also think perfect is the enemy of good here; multiple people have wasted time on the |undefined| issue so let's get that fixed first.
Comment on attachment 8812216 [details] [diff] [review]
Patch

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

::: js/src/tests/ecma_6/TypedArray/constructor-undefined-args.js
@@ +4,5 @@
> +
> +assertEq(new Int32Array(ab).length, 4);
> +assertEq(new Int32Array(ab, undefined).length, 4);
> +assertEq(new Int32Array(ab, undefined, undefined).length, 4);
> +assertEq(new Int32Array(ab, 0).length, 4);

Maybe also add a test for `new Int32Array(ab, 0, undefined)`.

@@ +5,5 @@
> +assertEq(new Int32Array(ab).length, 4);
> +assertEq(new Int32Array(ab, undefined).length, 4);
> +assertEq(new Int32Array(ab, undefined, undefined).length, 4);
> +assertEq(new Int32Array(ab, 0).length, 4);
> +assertEq(new Int32Array(ab, 4).length, 3);

And here `new Int32Array(ab, 4, undefined)`.
Attachment #8812216 - Flags: review?(andrebargull) → review+
(In reply to Jan de Mooij [:jandem] from comment #10)
> FWIW, I do think it would be nice to add ToIndex, add spec steps, etc, but I
> also think perfect is the enemy of good here; multiple people have wasted
> time on the |undefined| issue so let's get that fixed first.

Agreed. There's already bug 1317383 for test262 failures because we don't use ToIndex.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74bcf8fa3404
Fix handling of |undefined| args in typed array view constructor. r=anba
https://hg.mozilla.org/mozilla-central/rev/74bcf8fa3404
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Jan you mentioned fixing this for 52 in comment 8, if you still think this should happen could you request uplift for this patch?
Flags: needinfo?(jdemooij)
Comment on attachment 8812216 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Broken websites, webdevs wasting time.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The patch is simple and has been on m-c for almost a month.
[String changes made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8812216 - Flags: approval-mozilla-aurora?
Comment on attachment 8812216 [details] [diff] [review]
Patch

fix for javascript array view with undefined length, take in aurora52
Attachment #8812216 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We've built 51 RC. This is too late for 51. Mark 51 won't fix.
You need to log in before you can comment on or make changes to this bug.