Closed Bug 612334 Opened 14 years ago Closed 14 years ago

have typed arrays treat length params more sanely

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(2 files, 1 obsolete file)

fix webgl read-pixels-alignment test
3.75 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
updated
2.54 KB, patch
Waldo
: review+
Details | Diff | Splinter Review 
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
Attachment #490622 - Flags: review?(jorendorff)
Attachment #490622 - Flags: review?(bjacob)
blocking2.0: --- → betaN+
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...
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.
Hmm. But |new Array([4]).toSource()| is "[[4]]".
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.
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.
Is there a WebGL spec to follow or fix?

/be
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.
(those of you who came here expecting a pile of webgl fixes, the right bug is bug 612336)
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.)
Attachment #490622 - Flags: review?(jorendorff)
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.)
Review ping for bjacob.
Assignee: general → vladimir
It turns out what I wasnt CC'd to the bug. I received the original review request and forgot about it.
(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? :)
Attachment #490622 - Flags: review?(bjacob) → review+
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 :(
Lots of fail on my side too --- I had forgotten again and didn't do it until today
Attached patch updated patch (obsolete) — — Splinter Review 
Ok, here's an updated patch.  Correctly handles strings (by throwing) and various other bits.
Attachment #498818 - Flags: review?
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)
Attachment #498818 - Flags: review?(jwalden+bmo)
Attachment #498818 - Flags: review?(jorendorff)
Attachment #498818 - Flags: review?
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.
Attachment #498818 - Flags: review?(jwalden+bmo)
Attachment #498818 - Flags: review?(jorendorff)
Attachment #498818 - Flags: review-
> >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.
Attached patch updated — — Splinter Review 
ok, updated with comments; also fixed the test so that it actually sends down a integer-as-double value.
Attachment #498818 - Attachment is obsolete: true
Attachment #499220 - Flags: review?(jwalden+bmo)
Blocks: 620351
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?
Attachment #499220 - Flags: review?(jwalden+bmo) → review+
> >+    // 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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: