Bug 1090636, part 6 - Change DefineNativeProperty to support redefining array elements. r=efaust.

As it stands, this code is relatively obscure, so the fact that it's amazingly busted is of little consequence. But once we implement ES6 assignment, this code will be fairly easy to trigger when assigning to any Proxy whose target is an Array.

--HG--
extra : rebase_source : f939b92b42f0470fb0309097fb82f46383a61b25
extra : source : 1ecbc62d82a38e6e36e6ede090d711f3b3ad13ba

js/src/jsapi.cpp

@@ -2714,23 +2714,10 @@ JS_AlreadyHasOwnPropertyById(JSContext *cx, HandleObject obj, HandleId id, bool
         return true;
     }
 
-    // Check for an existing native property on the object. Be careful not to
-    // call any lookup or resolve hooks.
-    if (JSID_IS_INT(id)) {
-        uint32_t index = JSID_TO_INT(id);
-
-        if (obj->as<NativeObject>().containsDenseElement(index)) {
-            *foundp = true;
-            return true;
-        }
-
-        if (IsAnyTypedArray(obj) && index < AnyTypedArrayLength(obj)) {
-            *foundp = true;
-            return true;
-        }
-    }
-
-    *foundp = obj->as<NativeObject>().contains(cx, id);
+    RootedNativeObject nativeObj(cx, &obj->as<NativeObject>());
+    RootedShape prop(cx);
+    NativeLookupOwnPropertyNoResolve(cx, nativeObj, id, &prop);
+    *foundp = !!prop;
     return true;
 }
 

js/src/vm/NativeObject-inl.h

@@ -574,6 +574,36 @@ LookupOwnPropertyInline(ExclusiveContext *cx,
     return true;
 }
 
+/*
+ * Simplified version of LookupOwnPropertyInline that doesn't call resolve
+ * hooks.
+ */
+static inline void
+NativeLookupOwnPropertyNoResolve(ExclusiveContext *cx, HandleNativeObject obj, HandleId id,
+                                 MutableHandleShape result)
+{
+    // Check for a native dense element.
+    if (JSID_IS_INT(id) && obj->containsDenseElement(JSID_TO_INT(id))) {
+        MarkDenseOrTypedArrayElementFound<CanGC>(result);
+        return;
+    }
+
+    // Check for a typed array element.
+    if (IsAnyTypedArray(obj)) {
+        uint64_t index;
+        if (IsTypedArrayIndex(id, &index)) {
+            if (index < AnyTypedArrayLength(obj))
+                MarkDenseOrTypedArrayElementFound<CanGC>(result);
+            else
+                result.set(nullptr);
+            return;
+        }
+    }
+
+    // Check for a native property.
+    result.set(obj->lookup(cx, id));
+}
+
 template <AllowGC allowGC>
 static MOZ_ALWAYS_INLINE bool
 LookupPropertyInline(ExclusiveContext *cx,

js/src/vm/NativeObject.cpp

@@ -1472,32 +1472,38 @@ js::DefineNativeProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId
             }
         }
     } else if (!(attrs & JSPROP_IGNORE_VALUE)) {
-        /*
-         * We might still want to ignore redefining some of our attributes, if the
-         * request came through a proxy after Object.defineProperty(), but only if we're redefining
-         * a data property.
-         * FIXME: All this logic should be removed when Proxies use PropDesc, but we need to
-         *        remove JSPropertyOp getters and setters first.
-         * FIXME: This is still wrong for various array types, and will set the wrong attributes
-         *        by accident, but we can't use NativeLookupOwnProperty in this case, because of resolve
-         *        loops.
-         */
-        shape = obj->lookup(cx, id);
+        // If we did a normal lookup here, it would cause resolve hook recursion in
+        // the following case. Suppose the first script we run in a lazy global is
+        // |parseInt()|.
+        //   - js_InitNumber is called to resolve parseInt.
+        //   - js_InitNumber tries to define the Number constructor on the global.
+        //   - We end up here.
+        //   - This lookup for 'Number' triggers the global resolve hook.
+        //   - js_InitNumber is called again, this time to resolve Number.
+        //   - It creates a second Number constructor, which trips an assertion.
+        //
+        // Therefore we do a special lookup that does not call the resolve hook.
+        NativeLookupOwnPropertyNoResolve(cx, obj, id, &shape);
+
         if (shape) {
-            if (shape->isAccessorDescriptor() &&
-                !CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
-            {
-                return false;
-            }
-            if (shape->isDataDescriptor())
+            // If any other JSPROP_IGNORE_* attributes are present, copy the
+            // corresponding JSPROP_* attributes from the existing property.
+            if (IsImplicitDenseOrTypedArrayElement(shape)) {
+                attrs = ApplyAttributes(attrs, true, true, !IsAnyTypedArray(obj));
+            } else {
                 attrs = ApplyOrDefaultAttributes(attrs, shape);
+
+                // Do not redefine a nonconfigurable accessor property.
+                if (shape->isAccessorDescriptor()) {
+                    if (!CheckAccessorRedefinition(cx, obj, shape, getter, setter, id, attrs))
+                        return false;
+                }
+            }
         }
     } else {
-        /*
-         * We have been asked merely to update some attributes by a caller of
-         * Object.defineProperty, laundered through the proxy system, and returning here. We can
-         * get away with just using JSObject::changeProperty here.
-         */
+        // We have been asked merely to update some attributes. If the
+        // property already exists and it's a data property, we can just
+        // call JSObject::changeProperty.
         if (!NativeLookupOwnProperty(cx, obj, id, &shape))
             return false;