Bug 647425 - Don't try to use js_PrototypeHasIndexedProperties in GetElements; its wrong for arguments objects (r=waldo)

js/src/jit-test/tests/arguments/testDelArg3.js

@@ -0,0 +1,42 @@
+function assertGood(x) {
+    assertEq(x, "good");
+}
+
+(function() {
+    var a = arguments;
+    return function() {
+        assertGood.apply(null, a);
+    }
+})("good")();
+
+(function() {
+    var a = arguments;
+    return function() {
+        a[0] = "good";
+        assertGood.apply(null, a);
+    }
+})("bad")();
+
+Object.prototype[0] = "good";
+
+(function() {
+    var a = arguments;
+    return function() {
+        delete a[0];
+        assertGood.apply(null, a);
+    }
+})("bad")();
+
+delete Object.prototype[0];
+
+function assertUndefined(x) {
+    assertEq(x, undefined);
+}
+
+(function() {
+    var a = arguments;
+    return function() {
+        a[0] = "bad";
+        assertUndefined.apply(null, a);
+    }
+})()();

js/src/jit-test/tests/arguments/testDelArg3Strict.js

@@ -0,0 +1,44 @@
+"use strict";
+
+function assertGood(x) {
+    assertEq(x, "good");
+}
+
+(function() {
+    var a = arguments;
+    return function() {
+        assertGood.apply(null, a);
+    }
+})("good")();
+
+(function() {
+    var a = arguments;
+    return function() {
+        a[0] = "good";
+        assertGood.apply(null, a);
+    }
+})("bad")();
+
+Object.prototype[0] = "good";
+
+(function() {
+    var a = arguments;
+    return function() {
+        delete a[0];
+        assertGood.apply(null, a);
+    }
+})("bad")();
+
+delete Object.prototype[0];
+
+function assertUndefined(x) {
+    assertEq(x, undefined);
+}
+
+(function() {
+    var a = arguments;
+    return function() {
+        a[0] = "bad";
+        assertUndefined.apply(null, a);
+    }
+})()();

js/src/jsarray.cpp

@@ -400,6 +400,19 @@ GetElement(JSContext *cx, JSObject *obj, jsdouble index, JSBool *hole, Value *vp
 
 namespace js {
 
+struct STATIC_SKIP_INFERENCE CopyNonHoleArgsTo
+{
+    CopyNonHoleArgsTo(JSObject *aobj, Value *dst) : aobj(aobj), dst(dst) {}
+    JSObject *aobj;
+    Value *dst;
+    bool operator()(uintN argi, Value *src) {
+        if (aobj->getArgsElement(argi).isMagic(JS_ARGS_HOLE))
+            return false;
+        *dst++ = *src;
+        return true;
+    }
+};
+
 bool
 GetElements(JSContext *cx, JSObject *aobj, jsuint length, Value *vp)
 {
@@ -413,21 +426,28 @@ GetElements(JSContext *cx, JSObject *aobj, jsuint length, Value *vp)
     } else if (aobj->isArguments() && !aobj->isArgsLengthOverridden() &&
                !js_PrototypeHasIndexedProperties(cx, aobj)) {
         /*
-         * Two cases, two loops: note how in the case of an active stack frame
-         * backing aobj, even though we copy from fp->argv, we still must check
-         * aobj->getArgsElement(i) for a hole, to handle a delete on the
-         * corresponding arguments element. See args_delProperty.
+         * If the argsobj is for an active call, then the elements are the
+         * live args on the stack. Otherwise, the elements are the args that
+         * were copied into the argsobj by PutActivationObjects when the
+         * function returned. In both cases, it is necessary to fall off the
+         * fast path for deleted properties (MagicValue(JS_ARGS_HOLE) since
+         * this requires general-purpose property lookup.
          */
         if (JSStackFrame *fp = (JSStackFrame *) aobj->getPrivate()) {
             JS_ASSERT(fp->numActualArgs() <= JS_ARGS_LENGTH_MAX);
-            fp->forEachCanonicalActualArg(CopyNonHoleArgsTo(aobj, vp));
+            if (!fp->forEachCanonicalActualArg(CopyNonHoleArgsTo(aobj, vp)))
+                goto found_deleted_prop;
         } else {
             Value *srcbeg = aobj->getArgsElements();
             Value *srcend = srcbeg + length;
-            for (Value *dst = vp, *src = srcbeg; src < srcend; ++dst, ++src)
-                *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src;
+            for (Value *dst = vp, *src = srcbeg; src < srcend; ++dst, ++src) {
+                if (src->isMagic(JS_ARGS_HOLE))
+                    goto found_deleted_prop;
+                *dst = *src;
+            }
         }
     } else {
+      found_deleted_prop:
         for (uintN i = 0; i < length; i++) {
             if (!aobj->getProperty(cx, INT_TO_JSID(jsint(i)), &vp[i]))
                 return JS_FALSE;

js/src/jsfun.cpp

@@ -223,10 +223,11 @@ struct STATIC_SKIP_INFERENCE PutArg
 {
     PutArg(Value *dst) : dst(dst) {}
     Value *dst;
-    void operator()(uintN, Value *src) {
+    bool operator()(uintN, Value *src) {
         if (!dst->isMagic(JS_ARGS_HOLE))
             *dst = *src;
         ++dst;
+        return true;
     }
 };
 

js/src/jsinterp.h

@@ -396,8 +396,13 @@ struct JSStackFrame
     inline js::Value *actualArgsEnd() const;
 
     inline js::Value &canonicalActualArg(uintN i) const;
-    template <class Op> inline void forEachCanonicalActualArg(Op op);
-    template <class Op> inline void forEachFormalArg(Op op);
+
+    /*
+     * Apply 'op' to each arg of the specified type. Stop if 'op' returns
+     * false. Return 'true' iff all 'op' calls returned true.
+     */
+    template <class Op> inline bool forEachCanonicalActualArg(Op op);
+    template <class Op> inline bool forEachFormalArg(Op op);
 
     inline void clearMissingArgs();
 

js/src/jsinterpinlines.h

@@ -287,7 +287,7 @@ JSStackFrame::canonicalActualArg(uintN i) const
 }
 
 template <class Op>
-inline void
+inline bool
 JSStackFrame::forEachCanonicalActualArg(Op op)
 {
     uintN nformal = fun()->nargs;
@@ -296,53 +296,50 @@ JSStackFrame::forEachCanonicalActualArg(Op op)
     if (nactual <= nformal) {
         uintN i = 0;
         js::Value *actualsEnd = formals + nactual;
-        for (js::Value *p = formals; p != actualsEnd; ++p, ++i)
-            op(i, p);
+        for (js::Value *p = formals; p != actualsEnd; ++p, ++i) {
+            if (!op(i, p))
+                return false;
+        }
     } else {
         uintN i = 0;
         js::Value *formalsEnd = formalArgsEnd();
-        for (js::Value *p = formals; p != formalsEnd; ++p, ++i)
-            op(i, p);
+        for (js::Value *p = formals; p != formalsEnd; ++p, ++i) {
+            if (!op(i, p))
+                return false;
+        }
         js::Value *actuals = formalsEnd - (nactual + 2);
         js::Value *actualsEnd = formals - 2;
-        for (js::Value *p = actuals; p != actualsEnd; ++p, ++i)
-            op(i, p);
+        for (js::Value *p = actuals; p != actualsEnd; ++p, ++i) {
+            if (!op(i, p))
+                return false;
         }
+    }
+    return true;
 }
 
 template <class Op>
-inline void
+inline bool
 JSStackFrame::forEachFormalArg(Op op)
 {
     js::Value *formals = formalArgsEnd() - fun()->nargs;
     js::Value *formalsEnd = formalArgsEnd();
     uintN i = 0;
-    for (js::Value *p = formals; p != formalsEnd; ++p, ++i)
-        op(i, p);
+    for (js::Value *p = formals; p != formalsEnd; ++p, ++i) {
+        if (!op(i, p))
+            return false;
+    }
+    return true;
 }
 
 namespace js {
 
-struct STATIC_SKIP_INFERENCE CopyNonHoleArgsTo
-{
-    CopyNonHoleArgsTo(JSObject *aobj, Value *dst) : aobj(aobj), dst(dst) {}
-    JSObject *aobj;
-    Value *dst;
-    void operator()(uintN argi, Value *src) {
-        if (aobj->getArgsElement(argi).isMagic(JS_ARGS_HOLE))
-            dst->setUndefined();
-        else
-            *dst = *src;
-        ++dst;
-    }
-};
-
 struct CopyTo
 {
     Value *dst;
     CopyTo(Value *dst) : dst(dst) {}
-    void operator()(uintN, Value *src) {
+    bool operator()(uintN, Value *src) {
         *dst++ = *src;
+        return true;
     }
 };
 

js/src/jstracer.cpp

@@ -10155,8 +10155,9 @@ class BoxArg
         : tr(tr), addr(addr) {}
     TraceRecorder *tr;
     Address addr;
-    void operator()(uintN argi, Value *src) {
+    bool operator()(uintN argi, Value *src) {
         tr->box_value_into(*src, tr->get(src), OffsetAddress(addr, argi * sizeof(Value)));
+        return true;
     }
 };