Bug 1218111 - Fix property enumeration order of unboxed objects with expando properties. r=bhackett, a=lizzard
authorJan de Mooij <jdemooij@mozilla.com>
Wed, 28 Oct 2015 17:02:52 +0100
changeset 296697 842a741af958
parent 296696 9669bc030ba7
child 296698 45f9754edccf
push id5295
push userjandemooij@gmail.com
push date2015-11-11 21:25 +0000
treeherdermozilla-beta@842a741af958 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbhackett, lizzard
bugs1218111
milestone43.0
Bug 1218111 - Fix property enumeration order of unboxed objects with expando properties. r=bhackett, a=lizzard
js/src/jit-test/tests/basic/unboxed-property-enumeration.js
js/src/jsiter.cpp
js/src/vm/UnboxedObject.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/unboxed-property-enumeration.js
@@ -0,0 +1,26 @@
+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);
+
+    // Note: this should really be 0234xyz, this is bug 1175111. It has been
+    // fixed on newer branches.
+    assertEq(Object.getOwnPropertyNames(o).join(""), "024xyz3");
+}
+testUnboxed();
--- a/js/src/jsiter.cpp
+++ b/js/src/jsiter.cpp
@@ -133,18 +133,45 @@ Enumerate(JSContext* cx, HandleObject po
         return true;
     if (!enumerable && !(flags & JSITER_HIDDEN))
         return true;
 
     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
 EnumerateNativeProperties(JSContext* cx, HandleNativeObject pobj, unsigned flags, Maybe<IdSet>& ht,
-                          AutoIdVector* props)
+                          AutoIdVector* props, Handle<UnboxedPlainObject*> unboxed = nullptr)
 {
     bool enumerateSymbols;
     if (flags & JSITER_SYMBOLSONLY) {
         enumerateSymbols = true;
     } else {
         /* Collect any dense elements from this object. */
         size_t initlen = pobj->getDenseInitializedLength();
         const Value* vp = pobj->getDenseElements();
@@ -160,16 +187,26 @@ EnumerateNativeProperties(JSContext* cx,
         if (IsAnyTypedArray(pobj)) {
             size_t len = AnyTypedArrayLength(pobj);
             for (size_t i = 0; i < len; i++) {
                 if (!Enumerate(cx, pobj, INT_TO_JSID(i), /* enumerable = */ true, flags, ht, props))
                     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. */
         bool symbolsFound = false;
         Shape::Range<NoGC> r(pobj->lastProperty());
         for (; !r.empty(); r.popFront()) {
             Shape& shape = r.front();
             jsid id = shape.propid();
@@ -280,38 +317,33 @@ static bool
 Snapshot(JSContext* cx, HandleObject pobj_, unsigned flags, AutoIdVector* props)
 {
     // We initialize |ht| lazily (in Enumerate()) because it ends up unused
     // anywhere from 67--99.9% of the time.
     Maybe<IdSet> ht;
     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];
+        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;
+                }
+            } else {
+                if (!EnumerateExtraProperties(cx, pobj, flags, ht, props))
+                    return false;
 
-                // 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))
-                    return false;
-            }
-
-            if (pobj->isNative()) {
-                if (!EnumerateNativeProperties(cx, pobj.as<NativeObject>(), 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
             if (JSEnumerateOp enumerate = pobj->getClass()->enumerate) {
                 if (!enumerate(cx, pobj.as<NativeObject>()))
                     return false;
             }
             if (!EnumerateNativeProperties(cx, pobj.as<NativeObject>(), flags, ht, props))
--- a/js/src/vm/UnboxedObject.cpp
+++ b/js/src/vm/UnboxedObject.cpp
@@ -888,47 +888,25 @@ UnboxedPlainObject::obj_watch(JSContext*
         return false;
     return WatchProperty(cx, obj, id, callable);
 }
 
 /* static */ bool
 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++) {
         if (!properties.append(NameToId(unboxed[i].name)))
             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;
 }
 
 const Class UnboxedExpandoObject::class_ = {
     "UnboxedExpandoObject",
     0
 };