323582 - JS_HasArrayLength() throws errors if there is no length property

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Michael Daumling]

Calling JS_HasArrayLength() should not throw any errors. Currently, ValueIsLength() (called in the process) throws errors if there is not a length property, or if the length property is not a number, or out of range.

Comment 1

[Michael Daumling]

I'll take this bug.

Comment 2

[Michael Daumling]

Attachment: Fix

The fix adds a new argument reportError to ValueIsLength(). If JS_FALSE, the function does not report an error. it is only JS_FALSE in js_HasLengthProperty().

I've also cleaned up ValueIsLength() a little.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 212532 [details] [diff] [review]
Fix

Thanks for patching this -- nits only below.

>+ValueIsLength(JSContext *cx, jsval v, jsuint *lengthp, JSBool reportError)

Nit: out params after in params, generally -- so lengthp last.

> {
>     jsint i;
>     jsdouble d;
> 
>     if (JSVAL_IS_INT(v)) {
>         i = JSVAL_TO_INT(v);
>-        if (i < 0) {
>-            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>-                                 JSMSG_BAD_ARRAY_LENGTH);
>-            return JS_FALSE;
>-        }
>+        if (i < 0)
>+            goto bad;
>         *lengthp = (jsuint) i;
>         return JS_TRUE;

Note early true return.

>     }
> 
>-    if (!js_ValueToNumber(cx, v, &d)) {
>-        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>-                             JSMSG_BAD_ARRAY_LENGTH);
>-        return JS_FALSE;
>-    }
>-    if (!js_DoubleToECMAUint32(cx, d, (uint32 *)lengthp)) {
>-        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>-                             JSMSG_BAD_ARRAY_LENGTH);
>-        return JS_FALSE;
>-    }
>-    if (JSDOUBLE_IS_NaN(d) || d != *lengthp) {
>+    if (js_ValueToNumber(cx, v, &d)
>+     && js_DoubleToECMAUint32(cx, d, (uint32 *)lengthp)
>+     && !JSDOUBLE_IS_NaN(d) 
>+     && d == *lengthp)

Nit: prevailing style puts && and || at end of line, indents terms to underhang one another (as above, but without the leading && ;-).

Nit: brace the then part because the condition is multiline -- prevailing style braces when any of condition, then, or else parts are (or appear to be, even if only due to comments) multiline.

>+        return JS_TRUE;

Suggestion: invert the && chain's terms and use || to guard a goto bad; then part -- this allows the return true to be least-indented and also commoned with the earlier one (noted above).

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 212532 [details] [diff] [review]
Fix

Would prefer new patch and bug assignee (IIRC Michael is busy elsewhere). Igor, if you don't care, please obsolete the patch and put the bug into the [e-mail] pool.

/be

Comment 5

[Igor Bukanov]

To Michael Daumling:

Is it still relevant? If so, I will review/land the patch as soon as there would be update against the current CVS tip.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> To Michael Daumling:
> 
> Is it still relevant? If so, I will review/land the patch as soon as there
> would be update against the current CVS tip.

And if you are busy, I will update the patch myself, but then I need to know if it is relevant.