Bug 647425 - Don't try to use js_PrototypeHasIndexedProperties in GetElements; its wrong for arguments objects (r=waldo)
authorLuke Wagner <lw@mozilla.com>
Thu, 07 Apr 2011 20:35:02 -0700
changeset 68528 0c727da2164d
parent 68527 a4e83114bfee
child 68529 3dc5d38ba870
child 74382 ce3107de2110
push id19680
push usercleary@mozilla.com
push dateTue, 26 Apr 2011 17:44:40 +0000
treeherdermozilla-central@28bc239d3d9d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswaldo
bugs647425
milestone6.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
js/src/jit-test/tests/arguments/testDelArg3Strict.js
js/src/jsarray.cpp
js/src/jsfun.cpp
js/src/jsinterp.h
js/src/jsinterpinlines.h
js/src/jstracer.cpp
new file mode 100644
--- /dev/null
+++ b/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);
+    }
+})()();
new file mode 100644
--- /dev/null
+++ b/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);
+    }
+})()();
--- a/js/src/jsarray.cpp
+++ b/js/src/jsarray.cpp
@@ -395,44 +395,64 @@ GetElement(JSContext *cx, JSObject *obj,
             return JS_FALSE;
         *hole = JS_FALSE;
     }
     return JS_TRUE;
 }
 
 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)
 {
     if (aobj->isDenseArray() && length <= aobj->getDenseArrayCapacity() &&
         !js_PrototypeHasIndexedProperties(cx, aobj)) {
         /* The prototype does not have indexed properties so hole = undefined */
         Value *srcbeg = aobj->getDenseArrayElements();
         Value *srcend = srcbeg + length;
         for (Value *dst = vp, *src = srcbeg; src < srcend; ++dst, ++src)
             *dst = src->isMagic(JS_ARRAY_HOLE) ? UndefinedValue() : *src;
     } 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;
         }
     }
 
     return true;
 }
--- a/js/src/jsfun.cpp
+++ b/js/src/jsfun.cpp
@@ -218,20 +218,21 @@ NewArguments(JSContext *cx, JSObject *pa
 
     return argsobj;
 }
 
 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;
     }
 };
 
 JSObject *
 js_GetArgsObject(JSContext *cx, JSStackFrame *fp)
 {
     /*
      * We must be in a function activation; the function must be lightweight
--- a/js/src/jsinterp.h
+++ b/js/src/jsinterp.h
@@ -391,18 +391,23 @@ struct JSStackFrame
                : NULL;
     }
 
     inline uintN numActualArgs() const;
     inline js::Value *actualArgs() const;
     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();
 
     bool hasArgsObj() const {
         return !!(flags_ & JSFRAME_HAS_ARGS_OBJ);
     }
 
     JSObject &argsObj() const {
--- a/js/src/jsinterpinlines.h
+++ b/js/src/jsinterpinlines.h
@@ -282,72 +282,69 @@ JSStackFrame::canonicalActualArg(uintN i
 {
     if (i < numFormalArgs())
         return formalArg(i);
     JS_ASSERT(i < numActualArgs());
     return actualArgs()[i];
 }
 
 template <class Op>
-inline void
+inline bool
 JSStackFrame::forEachCanonicalActualArg(Op op)
 {
     uintN nformal = fun()->nargs;
     js::Value *formals = formalArgsEnd() - nformal;
     uintN nactual = numActualArgs();
     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_ALWAYS_INLINE void
 JSStackFrame::clearMissingArgs()
 {
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -10150,18 +10150,19 @@ TraceRecorder::clearReturningFrameFromNa
 
 class BoxArg
 {
   public:
     BoxArg(TraceRecorder *tr, Address addr)
         : 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;
     }
 };
 
 /*
  * If we have created an |arguments| object for the frame, we must copy the
  * argument values into the object as properties in case it is used after
  * this frame returns.
  */