Bug 1476955 - Structured clone algorithm doesn't serialize Array Length contrary to HTML spec, resulting in truncation of trailing sparse arrays like [1,2,3,,] (length=4) to [1,2,3] (length=3); r=sfink
authorJan Varga <jan.varga@gmail.com>
Tue, 24 Jul 2018 18:19:19 +0200
changeset 822109 958b3cdb98f2183547aa74638305020230dd7877
parent 822108 f85380831fe4ba2a902a93146b6d9ff0043d1b59
child 822110 a3f53213595e5c86b03254635b2d9aa056dfbd94
push id117296
push userbmo:gl@mozilla.com
push dateTue, 24 Jul 2018 20:28:07 +0000
reviewerssfink
bugs1476955
milestone63.0a1
Bug 1476955 - Structured clone algorithm doesn't serialize Array Length contrary to HTML spec, resulting in truncation of trailing sparse arrays like [1,2,3,,] (length=4) to [1,2,3] (length=3); r=sfink
js/src/tests/non262/extensions/clone-object.js
js/src/tests/non262/extensions/shell.js
js/src/vm/StructuredClone.cpp
testing/web-platform/meta/IndexedDB/key_invalid.htm.ini
--- a/js/src/tests/non262/extensions/clone-object.js
+++ b/js/src/tests/non262/extensions/clone-object.js
@@ -49,22 +49,23 @@ function test() {
     b[-1] = -1;
     b[0xffffffff] = null;
     b[0x100000000] = null;
     b[""] = 0;
     b["\xff\x7f\u7fff\uffff\ufeff\ufffe"] = 1;
     b["\ud800 \udbff \udc00 \udfff"] = 2;
     check(b);
 
-    // An array's .length property is not enumerable, so it is not cloned.
+    // Check that array's .length property is cloned.
     b = Array(5);
     assertEq(b.length, 5);
     a = check(b);
-    assertEq(a.length, 0);
+    assertEq(a.length, 5);
 
+    b = Array(0);
     b[1] = "ok";
     a = check(b);
     assertEq(a.length, 2);
 
     // Check that prototypes are not cloned, per spec.
     b = Object.create({x:1});
     b.y = 2;
     b.z = 3;
@@ -100,16 +101,29 @@ function test() {
     check([0, 1, 2, , 4, 5, 6]);
 
     // Array holes should not take up space.
     b = [];
     b[255] = 1;
     check(b);
     assertEq(serialize(b).clonebuffer.length < 255, true);
 
+    // Check that trailing holes in an array are preserved.
+    b = [1,2,3,,];
+    assertEq(b.length, 4);
+    a = check(b);
+    assertEq(a.length, 4);
+    assertEq(a.toString(), "1,2,3,");
+
+    b = [1,2,3,,,];
+    assertEq(b.length, 5);
+    a = check(b);
+    assertEq(a.length, 5);
+    assertEq(a.toString(), "1,2,3,,");
+
     // Self-modifying object.
     // This should never read through to b's prototype.
     b = Object.create({y: 2}, 
                       {x: {enumerable: true,
                            configurable: true,
                            get: function() { if (this.hasOwnProperty("y")) delete this.y; return 1; }},
                        y: {enumerable: true,
                            configurable: true,
--- a/js/src/tests/non262/extensions/shell.js
+++ b/js/src/tests/non262/extensions/shell.js
@@ -254,18 +254,22 @@
       return Object.prototype.toString.call(obj);
     }
 
     function ownProperties(obj) {
       return Object.getOwnPropertyNames(obj).
         map(function (p) { return [p, Object.getOwnPropertyDescriptor(obj, p)]; });
     }
 
-    function isCloneable(pair) {
-      return typeof pair[0] === 'string' && pair[1].enumerable;
+    function isArrayLength(obj, pair) {
+      return Array.isArray(obj) && pair[0] == "length";
+    }
+
+    function isCloneable(obj, pair) {
+      return isArrayLength(obj, pair) || (typeof pair[0] === 'string' && pair[1].enumerable);
     }
 
     function notIndex(p) {
       var u = p >>> 0;
       return !("" + u == p && u != 0xffffffff);
     }
 
     function assertIsCloneOf(a, b, path) {
@@ -274,31 +278,27 @@
       var ca = classOf(a);
       assertEq(ca, classOf(b), path);
 
       assertEq(Object.getPrototypeOf(a),
                ca == "[object Object]" ? Object.prototype : Array.prototype,
                path);
 
       // 'b', the original object, may have non-enumerable or XMLName
-      // properties; ignore them.  'a', the clone, should not have any
-      // non-enumerable properties (except .length, if it's an Array) or
-      // XMLName properties.
-      var pb = ownProperties(b).filter(isCloneable);
+      // properties; ignore them (except .length, if it's an Array).
+      // 'a', the clone, should not have any non-enumerable properties
+      // (except .length, if it's an Array) or XMLName properties.
+      var pb = ownProperties(b).filter(function(element) {
+        return isCloneable(b, element);
+      });
       var pa = ownProperties(a);
       for (var i = 0; i < pa.length; i++) {
         assertEq(typeof pa[i][0], "string", "clone should not have E4X properties " + path);
-        if (!pa[i][1].enumerable) {
-          if (Array.isArray(a) && pa[i][0] == "length") {
-            // remove it so that the comparisons below will work
-            pa.splice(i, 1);
-            i--;
-          } else {
-            throw new Error("non-enumerable clone property " + uneval(pa[i][0]) + " " + path);
-          }
+        if (!isCloneable(a, pa[i])) {
+          throw new Error("non-cloneable clone property " + uneval(pa[i][0]) + " " + path);
         }
       }
 
       // Check that, apart from properties whose names are array indexes, 
       // the enumerable properties appear in the same order.
       var aNames = pa.map(function (pair) { return pair[1]; }).filter(notIndex);
       var bNames = pa.map(function (pair) { return pair[1]; }).filter(notIndex);
       assertEq(aNames.join(","), bNames.join(","), path);
@@ -311,17 +311,19 @@
       for (var i = 0; i < pa.length; i++) {
         var aName = pa[i][0];
         var bName = pb[i][0];
         assertEq(aName, bName, path);
 
         var path2 = path + "." + aName;
         var da = pa[i][1];
         var db = pb[i][1];
-        assertEq(da.configurable, true, path2);
+        if (!isArrayLength(a, pa[i])) {
+          assertEq(da.configurable, true, path2);
+        }
         assertEq(da.writable, true, path2);
         assertEq("value" in da, true, path2);
         var va = da.value;
         var vb = b[pb[i][0]];
         if (typeof va === "object" && va !== null)
           queue.push([va, vb, path2]);
         else
           assertEq(va, vb, path2);
--- a/js/src/vm/StructuredClone.cpp
+++ b/js/src/vm/StructuredClone.cpp
@@ -1400,17 +1400,26 @@ JSStructuredCloneWriter::traverseObject(
         return false;
 
     checkStack();
 
     // Write the header for obj.
     ESClass cls;
     if (!GetBuiltinClass(context(), obj, &cls))
         return false;
-    return out.writePair(cls == ESClass::Array ? SCTAG_ARRAY_OBJECT : SCTAG_OBJECT_OBJECT, 0);
+
+    if (cls == ESClass::Array) {
+        uint32_t length = 0;
+        if (!JS_GetArrayLength(context(), obj, &length))
+            return false;
+
+        return out.writePair(SCTAG_ARRAY_OBJECT, NativeEndian::swapToLittleEndian(length));
+    }
+
+    return out.writePair(SCTAG_OBJECT_OBJECT, 0);
 }
 
 bool
 JSStructuredCloneWriter::traverseMap(HandleObject obj)
 {
     Rooted<GCVector<Value>> newEntries(context(), GCVector<Value>(context()));
     {
         // If there is no wrapper, the compartment munging is a no-op.
@@ -2354,17 +2363,17 @@ JSStructuredCloneReader::startRead(Mutab
             return false;
         vp.setObject(*reobj);
         break;
       }
 
       case SCTAG_ARRAY_OBJECT:
       case SCTAG_OBJECT_OBJECT: {
         JSObject* obj = (tag == SCTAG_ARRAY_OBJECT)
-                        ? (JSObject*) NewDenseEmptyArray(context())
+                        ? (JSObject*) NewDenseUnallocatedArray(context(), NativeEndian::swapFromLittleEndian(data))
                         : (JSObject*) NewBuiltinClassInstance<PlainObject>(context());
         if (!obj || !objs.append(ObjectValue(*obj)))
             return false;
         vp.setObject(*obj);
         break;
       }
 
       case SCTAG_BACK_REFERENCE_OBJECT: {
deleted file mode 100644
--- a/testing/web-platform/meta/IndexedDB/key_invalid.htm.ini
+++ /dev/null
@@ -1,3 +0,0 @@
-[key_invalid.htm]
-  [Invalid key - \[1,2,3,,\]]
-    expected: FAIL