Bug 866700 - Assertion when redefining a non-writable length to a non-numeric value. r=bhackett
authorJeff Walden <jwalden@mit.edu>
Mon, 29 Apr 2013 12:30:21 -0700
changeset 141222 ed6ed6288ee27098cd1ffad21ff46127a64a6fd8
parent 141221 efa30046d86ea5c106d4c86cdce99aa7f7f50f3e
child 141223 7f6151df416096615b047e3097b9c480fb37f417
push id2579
push userakeybl@mozilla.com
push dateMon, 24 Jun 2013 18:52:47 +0000
treeherdermozilla-beta@b69b7de8a05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbhackett
bugs866700
milestone23.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 866700 - Assertion when redefining a non-writable length to a non-numeric value. r=bhackett
js/src/jsarray.cpp
js/src/jsarray.h
js/src/jsobj.cpp
js/src/tests/ecma_5/Array/redefine-nonwritable-length-custom-conversion-call-counts.js
js/src/tests/ecma_5/Array/redefine-nonwritable-length-custom-conversion-throw.js
js/src/tests/ecma_5/Array/redefine-nonwritable-length-nonnumeric.js
--- a/js/src/jsarray.cpp
+++ b/js/src/jsarray.cpp
@@ -415,42 +415,49 @@ struct ReverseIndexComparator
 {
     bool operator()(const uint32_t& a, const uint32_t& b, bool *lessOrEqualp) {
         MOZ_ASSERT(a != b, "how'd we get duplicate indexes?");
         *lessOrEqualp = b <= a;
         return true;
     }
 };
 
+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,
                    HandleValue value, bool setterIsStrict)
 {
     MOZ_ASSERT(obj->isArray());
     MOZ_ASSERT(id == NameToId(cx->names().length));
     MOZ_ASSERT(attrs & JSPROP_PERMANENT);
     MOZ_ASSERT(!(attrs & JSPROP_ENUMERATE));
 
     /* 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
     {
         RootedShape lengthShape(cx, obj->nativeLookup(cx, id));
         MOZ_ASSERT(lengthShape);
         MOZ_ASSERT(lengthShape->writable() == lengthIsWritable);
     }
--- a/js/src/jsarray.h
+++ b/js/src/jsarray.h
@@ -75,16 +75,23 @@ NewDenseCopiedArray(JSContext *cx, uint3
  * Determines whether a write to the given element on |obj| should fail because
  * |obj| is an Array with a non-writable length, and writing that element would
  * increase the length of the array.
  */
 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);
 
 extern JSBool
 GetLengthProperty(JSContext *cx, HandleObject obj, uint32_t *lengthp);
 
 extern JSBool
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -916,35 +916,45 @@ DefinePropertyOnObject(JSContext *cx, Ha
 static JSBool
 DefinePropertyOnArray(JSContext *cx, HandleObject obj, HandleId id, const PropDesc &desc,
                       bool throwError, bool *rval)
 {
     JS_ASSERT(obj->isArray());
 
     /* 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())
             return Reject(cx, id, JSMSG_CANT_REDEFINE_PROP, throwError, rval);
 
         if (desc.isAccessorDescriptor())
             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;
         }
 
         return ArraySetLength(cx, obj, id, attrs, v, throwError);
     }
 
new file mode 100644
--- /dev/null
+++ b/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");
new file mode 100644
--- /dev/null
+++ b/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");
new file mode 100644
--- /dev/null
+++ b/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");