Bug 866700 - Assertion when redefining a non-writable length to a non-numeric value. r=bhackett

js/src/jsarray.cpp

@@ -420,6 +420,22 @@ struct ReverseIndexComparator
     }
 };
 
+bool
+js::CanonicalizeArrayLengthValue(JSContext *cx, HandleValue v, uint32_t *newLen)
+{
+    if (!ToUint32(cx, v, newLen))
+        return false;
+
+    double d;
+    if (!ToNumber(cx, v, &d))
+        return false;
+    if (d == *newLen)
+        return true;
+
+    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_ARRAY_LENGTH);
+    return false;
+}
+
 /* ES6 20130308 draft 8.4.2.4 ArraySetLength */
 bool
 js::ArraySetLength(JSContext *cx, HandleObject obj, HandleId id, unsigned attrs,
@@ -432,20 +448,11 @@ js::ArraySetLength(JSContext *cx, HandleObject obj, HandleId id, unsigned attrs,
 
     /* Steps 1-2 are irrelevant in our implementation. */
 
-    /* Step 3. */
+    /* Steps 3-5. */
     uint32_t newLen;
-    if (!ToUint32(cx, value, &newLen))
+    if (!CanonicalizeArrayLengthValue(cx, value, &newLen))
         return false;
 
-    /* Steps 4-5. */
-    double d;
-    if (!ToNumber(cx, value, &d))
-        return false;
-    if (d != newLen) {
-        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_ARRAY_LENGTH);
-        return false;
-    }
-
     /* Steps 6-7. */
     bool lengthIsWritable = obj->arrayLengthIsWritable();
 #ifdef DEBUG

js/src/jsarray.h

@@ -80,6 +80,13 @@ extern bool
 WouldDefinePastNonwritableLength(JSContext *cx, HandleObject obj, uint32_t index, bool strict,
                                  bool *definesPast);
 
+/*
+ * Canonicalize |vp| to a uint32_t value potentially suitable for use as an
+ * array length.
+ */
+extern bool
+CanonicalizeArrayLengthValue(JSContext *cx, HandleValue v, uint32_t *canonicalized);
+
 /* Get the common shape used by all dense arrays with a prototype at globalObj. */
 extern RawShape
 GetDenseArrayShape(JSContext *cx, HandleObject globalObj);

js/src/jsobj.cpp

@@ -921,6 +921,23 @@ DefinePropertyOnArray(JSContext *cx, HandleObject obj, HandleId id, const PropDe
 
     /* Step 2. */
     if (id == NameToId(cx->names().length)) {
+        // Canonicalize value, if necessary, before proceeding any further.  It
+        // would be better if this were always/only done by ArraySetLength.
+        // But canonicalization may throw a RangeError (or other exception, if
+        // the value is an object with user-defined conversion semantics)
+        // before other attributes are checked.  So as long as our internal
+        // defineProperty hook doesn't match the ECMA one, this duplicate
+        // checking can't be helped.
+        RootedValue v(cx);
+        if (desc.hasValue()) {
+            uint32_t newLen;
+            if (!CanonicalizeArrayLengthValue(cx, desc.value(), &newLen))
+                return false;
+            v.setNumber(newLen);
+        } else {
+            v.setNumber(obj->getArrayLength());
+        }
+
         if (desc.hasConfigurable() && desc.configurable())
             return Reject(cx, id, JSMSG_CANT_REDEFINE_PROP, throwError, rval);
         if (desc.hasEnumerable() && desc.enumerable())
@@ -930,16 +947,9 @@ DefinePropertyOnArray(JSContext *cx, HandleObject obj, HandleId id, const PropDe
             return Reject(cx, id, JSMSG_CANT_REDEFINE_PROP, throwError, rval);
 
         unsigned attrs = obj->nativeLookup(cx, id)->attributes();
-
-        RootedValue v(cx, desc.hasValue() ? desc.value() : NumberValue(obj->getArrayLength()));
         if (!obj->arrayLengthIsWritable()) {
             if (desc.hasWritable() && desc.writable())
                 return Reject(cx, id, JSMSG_CANT_REDEFINE_PROP, throwError, rval);
-
-            if (desc.hasValue()) {
-                if (obj->getArrayLength() != desc.value().toNumber())
-                    return Reject(cx, id, JSMSG_CANT_REDEFINE_PROP, throwError, rval);
-            }
         } else {
             if (desc.hasWritable() && !desc.writable())
                 attrs = attrs | JSPROP_READONLY;

js/src/tests/ecma_5/Array/redefine-nonwritable-length-custom-conversion-call-counts.js

@@ -0,0 +1,45 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ * Contributor:
+ *   Jeff Walden <jwalden+code@mit.edu>
+ */
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 866700;
+var summary = "Assertion redefining non-writable length to a non-numeric value";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var count = 0;
+
+var convertible =
+  {
+    valueOf: function()
+    {
+      count++;
+      return 0;
+    }
+  };
+
+var arr = [];
+Object.defineProperty(arr, "length", { value: 0, writable: false });
+
+Object.defineProperty(arr, "length", { value: convertible });
+assertEq(count, 2);
+
+Object.defineProperty(arr, "length", { value: convertible });
+assertEq(count, 4);
+
+assertEq(arr.length, 0);
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");

js/src/tests/ecma_5/Array/redefine-nonwritable-length-custom-conversion-throw.js

@@ -0,0 +1,58 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ * Contributor:
+ *   Jeff Walden <jwalden+code@mit.edu>
+ */
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 866700;
+var summary = "Assertion redefining non-writable length to a non-numeric value";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var count = 0;
+
+var convertible =
+  {
+    valueOf: function()
+    {
+      count++;
+      if (count > 2)
+        return 0;
+      throw new SyntaxError("fnord");
+    }
+  };
+
+var arr = [];
+Object.defineProperty(arr, "length", { value: 0, writable: false });
+
+try
+{
+  Object.defineProperty(arr, "length",
+                        {
+                          value: convertible,
+                          writable: true,
+                          configurable: true,
+                          enumerable: true
+                        });
+  throw new Error("didn't throw");
+}
+catch (e)
+{
+  assertEq(e instanceof SyntaxError, true, "expected SyntaxError, got " + e);
+}
+
+assertEq(count, 2);
+assertEq(arr.length, 0);
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");

js/src/tests/ecma_5/Array/redefine-nonwritable-length-nonnumeric.js

@@ -0,0 +1,32 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ * Contributor:
+ *   Jeff Walden <jwalden+code@mit.edu>
+ */
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 866700;
+var summary = "Assertion redefining non-writable length to a non-numeric value";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var arr = [];
+Object.defineProperty(arr, "length", { value: 0, writable: false });
+
+// Per Array's magical behavior, the value in the descriptor gets canonicalized
+// *before* SameValue comparisons occur, so this shouldn't throw.
+Object.defineProperty(arr, "length", { value: '' });
+
+assertEq(arr.length, 0);
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");