612334 - have typed arrays treat length params more sanely

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix webgl read-pixels-alignment test

Typed arrays currently require length params to be int32s explicitly.  We should be more lenient here, and allow ints-as-doubles and ints-as-strings at the very least.  (Allowing objects to be converted to values is trickier, so not done here.)

The patch here fixes this, along with a misc additonal fix for a webgl test that's failing because of this.

Asking r? from jorendorff on js bits, bjacob on webgl bits

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why can't we do the same thing as jsarray.cpp's ValueIsLength does?  [4] won't convert to "4" if you call ValueToNumber on it, so I'm not sure what that part of the comment is talking about...

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Patch has a bug, needs to be argc >= 1, not argc == 1.

Also, [4] does convert to 4, I believe by going through a string somewhere in ValueToNumber. (That is, [4] -> "4" -> 4.)  This may be a separate bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm. But |new Array([4]).toSource()| is "[[4]]".

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or is |vp[2].isNumber()| false in that case in js_Array?

We should really just be doing whatever js_Array does, imo, insofar as it makes sense for typed arrays.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, it must be false.. in which case my code has essentially the same behavior, no, except that I mistakenly added the isString -- without that, we just do the effective isNumber check in a different spot.  We can't reuse the jsarray code directly since it throws an error at the end.  I'll rework the patch and make things more explicit.

Comment 6

[Brendan Eich [:brendan]]

Is there a WebGL spec to follow or fix?

/be

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, the typed array spec is written in terms of WebIDL, so the WebIDL-JS rules for overload resolution and argument conversion should apply.  I need to walk through those again to see what exactly is supposed to happen here.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(those of you who came here expecting a pile of webgl fixes, the right bug is bug 612336)

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 490622 [details] [diff] [review]
fix webgl read-pixels-alignment test

TestValueIsLength can fail with or without reporting an error. The caller needs to know the difference so it can bail out on error.

I don't think this should accept strings. What JS does with "new Array(len)" is described in http://people.mozilla.org/~jorendorff/es5.html#sec-15.4.2.2 :

> If the argument len is a Number and ToUint32(len) is equal to len, then the
> length property of the newly constructed object is set to ToUint32(len).
> If the argument len is a Number and ToUint32(len) is not equal to len, a
> RangeError exception is thrown.
>
> If the argument len is not a Number, then [...you get an array of length 1.]

Even ES5 requires an actual number here and throws if it it's Infinity, or NaN, or too large, or negative, or fractional. That seems like the only sensible thing to me. If WebIDL-JS can't support "this works like Array", maybe a bit of prose is in order.

Clearing the request bit; re-set it if you really want this now. (I'd be happy to review a patch narrowly fixing the fact that we reject non-int32 numbers; that's definitely a bug worth fixing. It wants a test, ideally.)

Comment 10

[Philip Taylor]

FYI: I encountered this problem in code that only passes integer-like arguments: the tracing JIT can apparently choose to represent the value as a double jsval, so the argv[0].isInt32() test fails, resulting in "invalid argument" errors after the JIT kicks in. (Can't easily get a minimal test case to reproduce it, though.)

Comment 11

[David Mandelin [:dmandelin]]

Review ping for bjacob.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

It turns out what I wasnt CC'd to the bug. I received the original review request and forgot about it.

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to comment #12)
> It turns out what I wasnt CC'd to the bug. I received the original review
> request and forgot about it.

Now that you're CC'd you won't forget about it, right? :)

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sorry, this bug is my fail -- I originally flagged both jorendorff and bjacob, but one of the request bits was cleared making it seem like it was just waiting on bjacob.  I'm updating the patch now :(

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Lots of fail on my side too --- I had forgotten again and didn't do it until today

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

Ok, here's an updated patch.  Correctly handles strings (by throwing) and various other bits.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 498818 [details] [diff] [review]
updated patch

(Flagging both jorendorff and waldo for review, whoever gets it first.. or if someone else wants to steal, go for it)

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 498818 [details] [diff] [review]
updated patch

>diff --git a/content/canvas/src/WebGLContextGL.cpp b/content/canvas/src/WebGLContextGL.cpp

These changes are all unrelated and go in separate patches (or at least revisions when pushed), right?


>diff --git a/js/src/jstypedarray.cpp b/js/src/jstypedarray.cpp

>+// This is similar to the one from jsarray.cpp, but modified
>+// for slightly different semantics.

I can't find the method to which you refer, in near-latest TM code.

>+static bool
>+TestValueIsLength(JSContext *cx, Value& vp, jsuint *len)

ValueIsTypedArrayLength seems like a better name to me.  This just seems like some sort of testing-framework method, which doesn't make much sense in a non-test file, at least to me.

vp is a name for Value*, not Value& -- you want v for this.  And make that const Value& v, to absolutely prevent callee-mutating-caller typos or similar.


>+{
>+    if (vp.isInt32()) {
>+        int32_t i = vp.toInt32();
>+        if (i < 0)
>+            return false;
>+        *len = i;
>+        return true;
>+    }
>+
>+    // we don't want null/undefined to become 0
>+    if (vp.isNullOrUndefined())
>+        return false;
>+
>+    // we don't want things like [4] to be turned into 4,
>+    // so only allow doubles and strings here.  This means
>+    // we'll lose out on some kind of objects, but that's
>+    // ok for this use.
>+    if (!vp.isDouble())
>+        return false;
>+
>+    jsdouble d;
>+    if (!ValueToNumber(cx, vp, &d))
>+        return false;
>+
>+    if (JSDOUBLE_IS_NaN(d))
>+        return false;
>+
>+    jsuint length;
>+    length = (jsuint) d;
>+    if (d != (jsdouble) length)
>+        return false;
>+
>+    *len = length;
>+    return true;
>+}

This is a bit verbose and unclear, seems to me.  All you want is to accept any unsigned 32-bit integer, in any possible value format, right?  This alternative (still needs renaming requested above) should do the same a bit more clearly:

>+{
>+    if (vp.isInt32()) {
>+        int32_t i = vp.toInt32();
>+        if (i < 0)
>+            return false;
>+        *len = i;
>+        return true;
>+    }
>+
>+    if (vp.isDouble()) {
>+        jsdouble d = vp.toDouble();
>+        if (JSDOUBLE_IS_NaN(d))
>+            return false;
>+        jsuint length = jsuint(d);
>+        if (d != jsdouble(length))
>+            return false;
>+        *len = length;
>+        return true;
>+    }
>+
>+    return false;
>+}


>         // figure out the type of the first argument;
>         // no args is treated like an int arg of 0.
>-        if (argc == 0 || argv[0].isInt32()) {
>-            int32 len = 0;
>+        jsuint len = 0;
>+        bool hasLen = false;
>+        if (argc >= 1) {
>+            // special-case null and undefined; make sure it
>+            // falls through instead of being converted to 0.
>+            hasLen = TestValueIsLength(cx, argv[0], &len);
>+        }

I wouldn't comment this.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> >diff --git a/js/src/jstypedarray.cpp b/js/src/jstypedarray.cpp
> 
> >+// This is similar to the one from jsarray.cpp, but modified
> >+// for slightly different semantics.
> 
> I can't find the method to which you refer, in near-latest TM code.

http://hg.mozilla.org/tracemonkey/file/tip/js/src/jsarray.cpp#l176

> >+static bool
> >+TestValueIsLength(JSContext *cx, Value& vp, jsuint *len)
> 
> ValueIsTypedArrayLength seems like a better name to me.  This just seems like
> some sort of testing-framework method, which doesn't make much sense in a
> non-test file, at least to me.
>
> vp is a name for Value*, not Value& -- you want v for this.  And make that
> const Value& v, to absolutely prevent callee-mutating-caller typos or similar.

k, will change both

> >+{
> >+    if (vp.isInt32()) {
> >+        int32_t i = vp.toInt32();
> >+        if (i < 0)
> >+            return false;
> >+        *len = i;
> >+        return true;
> >+    }
> >+
> >+    // we don't want null/undefined to become 0
> >+    if (vp.isNullOrUndefined())
> >+        return false;
> >+
> >+    // we don't want things like [4] to be turned into 4,
> >+    // so only allow doubles and strings here.  This means
> >+    // we'll lose out on some kind of objects, but that's
> >+    // ok for this use.
> >+    if (!vp.isDouble())
> >+        return false;
> >+
> >+    jsdouble d;
> >+    if (!ValueToNumber(cx, vp, &d))
> >+        return false;
> >+
> >+    if (JSDOUBLE_IS_NaN(d))
> >+        return false;
> >+
> >+    jsuint length;
> >+    length = (jsuint) d;
> >+    if (d != (jsdouble) length)
> >+        return false;
> >+
> >+    *len = length;
> >+    return true;
> >+}
> 
> This is a bit verbose and unclear, seems to me.  All you want is to accept any
> unsigned 32-bit integer, in any possible value format, right?  This alternative
> (still needs renaming requested above) should do the same a bit more clearly:

Yep, that's true -- I'll revise.  The above version had code snipped out of it that needed the earlier form, but your code's better/cleaner.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated

ok, updated with comments; also fixed the test so that it actually sends down a integer-as-double value.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 499220 [details] [diff] [review]
updated

>+static bool
>+ValueIsLength(JSContext *cx, const Value& v, jsuint *len)

const Value &v, though it sticks in my gut to say it.


>+        jsuint length = (jsuint) d;
>+        if (d != (jsdouble) length)
>+            return false;

We generally use C++-style casts now, jsuint(d) and jsdouble(length).


>+        jsuint len = 0;
>+        bool hasLen = true;
>+        if (argc > 0) {
>+            hasLen = ValueIsLength(cx, argv[0], &len);
>+        }

Ifs with one-line conditions and bodies shouldn't be braced.


>+    // test various types of args; Math.sqrt(4) is used to ensure that the
>+    // function gets a double, and not a demoted int

Doesn't need to happen here and now, but maybe we should add a shell primitive that guarantees it'll return its argument as a double-valued integer.  Mind filing that?

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> >+    // test various types of args; Math.sqrt(4) is used to ensure that the
> >+    // function gets a double, and not a demoted int
> 
> Doesn't need to happen here and now, but maybe we should add a shell primitive
> that guarantees it'll return its argument as a double-valued integer.  Mind
> filing that?

Filed bug 621061.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/026750b84bc2
http://hg.mozilla.org/mozilla-central/rev/ad071e6d8bb3