Bug 1185653 - Fix enumerate hook on unboxed objects to skip non-enumerable properties. r=jorendorff, a=ritu
authorJan de Mooij <jdemooij@mozilla.com>
Tue, 11 Aug 2015 17:42:56 +0200
changeset 288738 ac952c7ae8ccb498eb774354cf6d00e088aae433
parent 288737 22ca2af337eb2b1d870ba392e1ede85576e98ab5
child 288739 b021448f2a386576107f2f696a98c21c35c857fc
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff, ritu
bugs1185653
milestone42.0a2
Bug 1185653 - Fix enumerate hook on unboxed objects to skip non-enumerable properties. r=jorendorff, a=ritu
dom/plugins/base/nsJSNPRuntime.cpp
js/public/Class.h
js/src/builtin/TypedObject.cpp
js/src/builtin/TypedObject.h
js/src/jit-test/tests/basic/bug1185653.js
js/src/jsiter.cpp
js/src/vm/UnboxedObject.cpp
js/src/vm/UnboxedObject.h
js/xpconnect/src/XPCWrappedNativeJSOps.cpp
--- a/dom/plugins/base/nsJSNPRuntime.cpp
+++ b/dom/plugins/base/nsJSNPRuntime.cpp
@@ -171,17 +171,18 @@ NPObjWrapper_DelProperty(JSContext *cx, 
 static bool
 NPObjWrapper_SetProperty(JSContext *cx, JS::Handle<JSObject*> obj, JS::Handle<jsid> id,
                          JS::MutableHandle<JS::Value> vp, JS::ObjectOpResult &result);
 
 static bool
 NPObjWrapper_GetProperty(JSContext *cx, JS::Handle<JSObject*> obj, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp);
 
 static bool
-NPObjWrapper_Enumerate(JSContext *cx, JS::Handle<JSObject*> obj, JS::AutoIdVector &properties);
+NPObjWrapper_Enumerate(JSContext *cx, JS::Handle<JSObject*> obj, JS::AutoIdVector &properties,
+                       bool enumerableOnly);
 
 static bool
 NPObjWrapper_Resolve(JSContext *cx, JS::Handle<JSObject*> obj, JS::Handle<jsid> id,
                      bool *resolvedp);
 
 static bool
 NPObjWrapper_Convert(JSContext *cx, JS::Handle<JSObject*> obj, JSType type, JS::MutableHandle<JS::Value> vp);
 
@@ -1610,17 +1611,17 @@ CallNPMethod(JSContext *cx, unsigned arg
   if (!obj)
       return false;
 
   return CallNPMethodInternal(cx, obj, args.length(), args.array(), vp, false);
 }
 
 static bool
 NPObjWrapper_Enumerate(JSContext *cx, JS::Handle<JSObject*> obj,
-                       JS::AutoIdVector &properties)
+                       JS::AutoIdVector &properties, bool enumerableOnly)
 {
   NPObject *npobj = GetNPObject(cx, obj);
   if (!npobj || !npobj->_class) {
     ThrowJSException(cx, "Bad NPObject as private data!");
     return false;
   }
 
   PluginDestructionGuard pdg(LookupNPP(npobj));
--- a/js/public/Class.h
+++ b/js/public/Class.h
@@ -250,29 +250,30 @@ typedef bool
 // If no error occurred and the deletion wasn't disallowed (this is *not* the
 // same as saying that a deletion actually occurred -- deleting a non-existent
 // property, or an inherited property, is allowed -- it's just pointless),
 // call result.succeed() and return true.
 typedef bool
 (* JSDeletePropertyOp)(JSContext* cx, JS::HandleObject obj, JS::HandleId id,
                        JS::ObjectOpResult& result);
 
-// The type of ObjectOps::enumerate. This callback overrides a portion of SpiderMonkey's default
-// [[Enumerate]] internal method. When an ordinary object is enumerated, that object and each object
-// on its prototype chain is tested for an enumerate op, and those ops are called in order.
-// The properties each op adds to the 'properties' vector are added to the set of values the
-// for-in loop will iterate over. All of this is nonstandard.
+// The type of ObjectOps::enumerate. This callback overrides a portion of
+// SpiderMonkey's default [[Enumerate]] internal method. When an ordinary object
+// is enumerated, that object and each object on its prototype chain is tested
+// for an enumerate op, and those ops are called in order. The properties each
+// op adds to the 'properties' vector are added to the set of values the for-in
+// loop will iterate over. All of this is nonstandard.
 //
-// An object is "enumerated" when it's the target of a for-in loop or JS_Enumerate().
-// All other property inspection, including Object.keys(obj), goes through [[OwnKeys]].
-//
-// The callback's job is to populate 'properties' with all property keys that the for-in loop
-// should visit.
+// An object is "enumerated" when it's the target of a for-in loop or
+// JS_Enumerate(). The callback's job is to populate 'properties' with the
+// object's property keys. If `enumerableOnly` is true, the callback should only
+// add enumerable properties.
 typedef bool
-(* JSNewEnumerateOp)(JSContext* cx, JS::HandleObject obj, JS::AutoIdVector& properties);
+(* JSNewEnumerateOp)(JSContext* cx, JS::HandleObject obj, JS::AutoIdVector& properties,
+                     bool enumerableOnly);
 
 // The old-style JSClass.enumerate op should define all lazy properties not
 // yet reflected in obj.
 typedef bool
 (* JSEnumerateOp)(JSContext* cx, JS::HandleObject obj);
 
 // Resolve a lazy property named by id in obj by defining it directly in obj.
 // Lazy properties are those reflected from some peer native property space
--- a/js/src/builtin/TypedObject.cpp
+++ b/js/src/builtin/TypedObject.cpp
@@ -2054,17 +2054,18 @@ TypedObject::obj_deleteProperty(JSContex
     RootedObject proto(cx, obj->getProto());
     if (!proto)
         return result.succeed();
 
     return DeleteProperty(cx, proto, id, result);
 }
 
 bool
-TypedObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties)
+TypedObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
+                           bool enumerableOnly)
 {
     MOZ_ASSERT(obj->is<TypedObject>());
     Rooted<TypedObject*> typedObj(cx, &obj->as<TypedObject>());
     Rooted<TypeDescr*> descr(cx, &typedObj->typeDescr());
 
     RootedId id(cx);
     switch (descr->kind()) {
       case type::Scalar:
--- a/js/src/builtin/TypedObject.h
+++ b/js/src/builtin/TypedObject.h
@@ -539,17 +539,18 @@ class TypedObject : public JSObject
                                 HandleValue receiver, ObjectOpResult& result);
 
     static bool obj_getOwnPropertyDescriptor(JSContext* cx, HandleObject obj, HandleId id,
                                              MutableHandle<JSPropertyDescriptor> desc);
 
     static bool obj_deleteProperty(JSContext* cx, HandleObject obj, HandleId id,
                                    ObjectOpResult& result);
 
-    static bool obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties);
+    static bool obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
+                              bool enumerableOnly);
 
   public:
     TypedProto& typedProto() const {
         return getProto()->as<TypedProto>();
     }
 
     TypeDescr& typeDescr() const {
         return group()->typeDescr();
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug1185653.js
@@ -0,0 +1,28 @@
+function f() {
+    var arr = [];
+    for (var i=0; i<80; i++) {
+        var o3 = {foo: i};
+        var o2 = {owner: o3};
+        arr.push(o2);
+    }
+    for (var i=0; i<80; i++) {
+        var o2 = arr[i];
+        var o3 = o2.owner;
+        Object.defineProperty(o3, "bar", {value: arr, enumerable: false});
+    }
+    assertEq(JSON.stringify(arr).length, 1671);
+}
+f();
+
+function g() {
+    var arr = [];
+    for (var i=0; i<100; i++) {
+        arr.push([1, i]);
+    }
+    for (var i=0; i<100; i++) {
+        for (var p in arr[i]) {
+            assertEq(p === "0" || p === "1", true);
+        }
+    }
+}
+g();
--- a/js/src/jsiter.cpp
+++ b/js/src/jsiter.cpp
@@ -281,34 +281,30 @@ Snapshot(JSContext* cx, HandleObject pob
 {
     // 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) {
-            // This hook has the full control over what gets enumerated.
             AutoIdVector properties(cx);
-            if (!enumerate(cx, pobj, properties))
+            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];
-                bool enumerable = true;
 
                 // The enumerate hook does not indicate whether the properties
-                // it returns are enumerable or not. There is no non-effectful
-                // way to determine this from the object, so carve out
-                // exceptions here for places where the property is not
-                // enumerable.
-                if (pobj->is<UnboxedArrayObject>() && id == NameToId(cx->names().length))
-                    enumerable = false;
-
+                // 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;
             }
--- a/js/src/vm/UnboxedObject.cpp
+++ b/js/src/vm/UnboxedObject.cpp
@@ -879,17 +879,18 @@ UnboxedPlainObject::obj_deleteProperty(J
 UnboxedPlainObject::obj_watch(JSContext* cx, HandleObject obj, HandleId id, HandleObject callable)
 {
     if (!convertToNative(cx, obj))
         return false;
     return WatchProperty(cx, obj, id, callable);
 }
 
 /* static */ bool
-UnboxedPlainObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties)
+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)))
@@ -902,16 +903,18 @@ UnboxedPlainObject::obj_enumerate(JSCont
     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;
     }
 
@@ -1531,23 +1534,28 @@ UnboxedArrayObject::obj_deleteProperty(J
 UnboxedArrayObject::obj_watch(JSContext* cx, HandleObject obj, HandleId id, HandleObject callable)
 {
     if (!convertToNative(cx, obj))
         return false;
     return WatchProperty(cx, obj, id, callable);
 }
 
 /* static */ bool
-UnboxedArrayObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties)
+UnboxedArrayObject::obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
+                                  bool enumerableOnly)
 {
     for (size_t i = 0; i < obj->as<UnboxedArrayObject>().initializedLength(); i++) {
         if (!properties.append(INT_TO_JSID(i)))
             return false;
     }
-    return properties.append(NameToId(cx->names().length));
+
+    if (!enumerableOnly && !properties.append(NameToId(cx->names().length)))
+        return false;
+
+    return true;
 }
 
 const Class UnboxedArrayObject::class_ = {
     "Array",
     Class::NON_NATIVE |
     JSCLASS_IMPLEMENTS_BARRIERS |
     JSCLASS_SKIP_NURSERY_FINALIZE |
     JSCLASS_BACKGROUND_FINALIZE,
--- a/js/src/vm/UnboxedObject.h
+++ b/js/src/vm/UnboxedObject.h
@@ -257,17 +257,18 @@ class UnboxedPlainObject : public JSObje
                                 HandleValue receiver, ObjectOpResult& result);
 
     static bool obj_getOwnPropertyDescriptor(JSContext* cx, HandleObject obj, HandleId id,
                                              MutableHandle<JSPropertyDescriptor> desc);
 
     static bool obj_deleteProperty(JSContext* cx, HandleObject obj, HandleId id,
                                    ObjectOpResult& result);
 
-    static bool obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties);
+    static bool obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
+                              bool enumerableOnly);
     static bool obj_watch(JSContext* cx, HandleObject obj, HandleId id, HandleObject callable);
 
     inline const UnboxedLayout& layout() const;
 
     const UnboxedLayout& layoutDontCheckGeneration() const {
         return group()->unboxedLayoutDontCheckGeneration();
     }
 
@@ -392,17 +393,18 @@ class UnboxedArrayObject : public JSObje
                                 HandleValue receiver, ObjectOpResult& result);
 
     static bool obj_getOwnPropertyDescriptor(JSContext* cx, HandleObject obj, HandleId id,
                                              MutableHandle<JSPropertyDescriptor> desc);
 
     static bool obj_deleteProperty(JSContext* cx, HandleObject obj, HandleId id,
                                    ObjectOpResult& result);
 
-    static bool obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties);
+    static bool obj_enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
+                              bool enumerableOnly);
     static bool obj_watch(JSContext* cx, HandleObject obj, HandleId id, HandleObject callable);
 
     inline const UnboxedLayout& layout() const;
 
     const UnboxedLayout& layoutDontCheckGeneration() const {
         return group()->unboxedLayoutDontCheckGeneration();
     }
 
--- a/js/xpconnect/src/XPCWrappedNativeJSOps.cpp
+++ b/js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ -919,17 +919,18 @@ XPC_WN_Helper_Enumerate(JSContext* cx, H
     if (NS_FAILED(rv))
         return Throw(rv, cx);
     return retval;
 }
 
 /***************************************************************************/
 
 static bool
-XPC_WN_JSOp_Enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties)
+XPC_WN_JSOp_Enumerate(JSContext* cx, HandleObject obj, AutoIdVector& properties,
+                      bool enumerableOnly)
 {
     XPCCallContext ccx(JS_CALLER, cx, obj);
     XPCWrappedNative* wrapper = ccx.GetWrapper();
     THROW_AND_RETURN_IF_BAD_WRAPPER(cx, wrapper);
 
     XPCNativeScriptableInfo* si = wrapper->GetScriptableInfo();
     if (!si || !si->GetFlags().WantNewEnumerate())
         return Throw(NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, cx);