Bug 1218111 - Fix property enumeration order of unboxed objects with expando properties. r=bhackett

js/src/jit-test/tests/basic/unboxed-property-enumeration.js

@@ -0,0 +1,24 @@
+function O() {
+    this.x = 1;
+    this.y = 2;
+}
+function testUnboxed() {
+    var arr = [];
+    for (var i=0; i<100; i++)
+	arr.push(new O);
+
+    var o = arr[arr.length-1];
+    o[0] = 0;
+    o[2] = 2;
+    var sym = Symbol();
+    o[sym] = 1;
+    o.z = 3;
+    Object.defineProperty(o, '3', {value:1,enumerable:false,configurable:false,writable:false});
+    o[4] = 4;
+
+    var props = Reflect.ownKeys(o);
+    assertEq(props[props.length-1], sym);
+
+    assertEq(Object.getOwnPropertyNames(o).join(""), "0234xyz");
+}
+testUnboxed();

js/src/jsiter.cpp

@@ -135,6 +135,33 @@ Enumerate(JSContext* cx, HandleObject pobj, jsid id,
     return props->append(id);
 }
 
+static bool
+EnumerateExtraProperties(JSContext* cx, HandleObject obj, unsigned flags, Maybe<IdSet>& ht,
+                         AutoIdVector* props)
+{
+    MOZ_ASSERT(obj->getOps()->enumerate);
+
+    AutoIdVector properties(cx);
+    bool enumerableOnly = !(flags & JSITER_HIDDEN);
+    if (!obj->getOps()->enumerate(cx, obj, properties, enumerableOnly))
+        return false;
+
+    RootedId id(cx);
+    for (size_t n = 0; n < properties.length(); n++) {
+        id = properties[n];
+
+        // The enumerate hook does not indicate whether the properties
+        // it returns are enumerable or not. Since we already passed
+        // `enumerableOnly` to the hook to filter out non-enumerable
+        // properties, it doesn't really matter what we pass here.
+        bool enumerable = true;
+        if (!Enumerate(cx, obj, id, enumerable, flags, ht, props))
+            return false;
+    }
+
+    return true;
+}
+
 static bool
 SortComparatorIntegerIds(jsid a, jsid b, bool* lessOrEqualp)
 {
@@ -147,7 +174,7 @@ SortComparatorIntegerIds(jsid a, jsid b, bool* lessOrEqualp)
 
 static bool
 EnumerateNativeProperties(JSContext* cx, HandleNativeObject pobj, unsigned flags, Maybe<IdSet>& ht,
-                          AutoIdVector* props)
+                          AutoIdVector* props, Handle<UnboxedPlainObject*> unboxed = nullptr)
 {
     bool enumerateSymbols;
     if (flags & JSITER_SYMBOLSONLY) {
@@ -207,6 +234,16 @@ EnumerateNativeProperties(JSContext* cx, HandleNativeObject pobj, unsigned flags
                 return false;
         }
 
+        if (unboxed) {
+            // If |unboxed| is set then |pobj| is the expando for an unboxed
+            // plain object we are enumerating. Add the unboxed properties
+            // themselves here since they are all property names that were
+            // given to the object before any of the expando's properties.
+            MOZ_ASSERT(pobj->is<UnboxedExpandoObject>());
+            if (!EnumerateExtraProperties(cx, unboxed, flags, ht, props))
+                return false;
+        }
+
         size_t initialLength = props->length();
 
         /* Collect all unique property names from this object's shape. */
@@ -331,28 +368,23 @@ Snapshot(JSContext* cx, HandleObject pobj_, unsigned flags, AutoIdVector* props)
     RootedObject pobj(cx, pobj_);
 
     do {
-        if (JSNewEnumerateOp enumerate = pobj->getOps()->enumerate) {
-            AutoIdVector properties(cx);
-            bool enumerableOnly = !(flags & JSITER_HIDDEN);
-            if (!enumerate(cx, pobj, properties, enumerableOnly))
-                 return false;
-
-            RootedId id(cx);
-            for (size_t n = 0; n < properties.length(); n++) {
-                id = properties[n];
-
-                // The enumerate hook does not indicate whether the properties
-                // it returns are enumerable or not. Since we already passed
-                // `enumerableOnly` to the hook to filter out non-enumerable
-                // properties, it doesn't really matter what we pass here.
-                bool enumerable = true;
-                if (!Enumerate(cx, pobj, id, enumerable, flags, ht, props))
+        if (pobj->getOps()->enumerate) {
+            if (pobj->is<UnboxedPlainObject>() && pobj->as<UnboxedPlainObject>().maybeExpando()) {
+                // Special case unboxed objects with an expando object.
+                RootedNativeObject expando(cx, pobj->as<UnboxedPlainObject>().maybeExpando());
+                if (!EnumerateNativeProperties(cx, expando, flags, ht, props,
+                                               pobj.as<UnboxedPlainObject>()))
+                {
                     return false;
-            }
-
-            if (pobj->isNative()) {
-                if (!EnumerateNativeProperties(cx, pobj.as<NativeObject>(), flags, ht, props))
+                }
+            } else {
+                if (!EnumerateExtraProperties(cx, pobj, flags, ht, props))
                     return false;
+
+                if (pobj->isNative()) {
+                    if (!EnumerateNativeProperties(cx, pobj.as<NativeObject>(), flags, ht, props))
+                        return false;
+                }
             }
         } else if (pobj->isNative()) {
             // Give the object a chance to resolve all lazy properties

js/src/vm/UnboxedObject.cpp

@@ -893,17 +893,8 @@ UnboxedPlainObject::obj_watch(JSContext* cx, HandleObject obj, HandleId id, Hand
 UnboxedPlainObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
                                   bool enumerableOnly)
 {
-    UnboxedExpandoObject* expando = obj->as<UnboxedPlainObject>().maybeExpando();
-
-    // Add dense elements in the expando first, for consistency with plain objects.
-    if (expando) {
-        for (size_t i = 0; i < expando->getDenseInitializedLength(); i++) {
-            if (!expando->getDenseElement(i).isMagic(JS_ELEMENTS_HOLE)) {
-                if (!properties.append(INT_TO_JSID(i)))
-                    return false;
-            }
-        }
-    }
+    // Ignore expando properties here, they are special-cased by the property
+    // enumeration code.
 
     const UnboxedLayout::PropertyVector& unboxed = obj->as<UnboxedPlainObject>().layout().properties();
     for (size_t i = 0; i < unboxed.length(); i++) {
@@ -911,19 +902,6 @@ UnboxedPlainObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector&
             return false;
     }
 
-    if (expando) {
-        Vector<jsid> ids(cx);
-        for (Shape::Range<NoGC> r(expando->lastProperty()); !r.empty(); r.popFront()) {
-            if (enumerableOnly && !r.front().enumerable())
-                continue;
-            if (!ids.append(r.front().propid()))
-                return false;
-        }
-        ::Reverse(ids.begin(), ids.end());
-        if (!properties.append(ids.begin(), ids.length()))
-            return false;
-    }
-
     return true;
 }