Bug 614051 - TM: wrong behavior setting existing properties to joined function object values again. r=brendan.
authorJason Orendorff <jorendorff@mozilla.com>
Thu, 09 Dec 2010 12:04:35 -0600
changeset 59009 52d20032116aa1ecd79113d0413d2f83ae9400d5
parent 59008 010bd7365328ec688cc934f57cf5d6b360d7756c
child 59010 ce3aa8d61749e13300e4738e942106636661f137
child 59216 d416528b57827abb0be99fdf220229eefb51af30
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersbrendan
bugs614051
milestone2.0b8pre
Bug 614051 - TM: wrong behavior setting existing properties to joined function object values again. r=brendan.
js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt-2.js
js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt-3.js
js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt-4.js
js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt.js
js/src/jit-test/tests/pic/to-dictionary.js
js/src/jsobj.cpp
js/src/methodjit/PolyIC.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt-2.js
@@ -0,0 +1,8 @@
+function f() {
+    var a = [], i, N = HOTLOOP + 2;
+    for (i = 0; i < N; i++)
+        a[i] = {m: i, m: function() { return 0; }};
+    assertEq(a[N - 2].m === a[N - 1].m, false);
+}
+f();
+f();
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt-3.js
@@ -0,0 +1,15 @@
+function f() {
+    var a = [], i, N = HOTLOOP + 2;
+    for (i = 0; i < N; i++) {
+        a[i] = {};
+        a[i].m = function() { return 0; };
+        a[i].m = function() { return 1; };
+    }
+    assertEq(a[N - 2].m === a[N - 1].m, false);
+    for (i = 0; i < N; i++) {
+        var f = a[i].m;
+        assertEq(f(), 1);
+    }
+}
+f();
+f();
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt-4.js
@@ -0,0 +1,12 @@
+function f() {
+    var a = [], i, N = HOTLOOP + 2;
+    for (i = 0; i < N; i++)
+        a[i] = {m: function() { return 0; }, m: function() { return 1; }};
+    assertEq(a[N - 2].m === a[N - 1].m, false);
+    for (i = 0; i < N; i++) {
+        var f = a[i].m;
+        assertEq(f(), 1);
+    }
+}
+f();
+f();
--- a/js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt.js
+++ b/js/src/jit-test/tests/basic/testExistingPropToJoinedMethodAttempt.js
@@ -1,17 +1,10 @@
 function f() {
-  var a = [{m: 0}, {m: 1}, {m: 2}, {m: 3}, {m: 4}];
-  var b = [{}, {}, {}, {}, {}];
-  for (var i = 0; i < a.length; i++) {
-    a[i].m = function() { return 0; };
-    b[i].m = function() { return 1; };
-  }
-  assertEq(false, a[0].m === a[1].m);
-  assertEq(false, a[0].m === a[2].m);
-  assertEq(false, a[0].m === a[3].m);
-  assertEq(false, a[0].m === a[4].m);
-  assertEq(false, b[0].m === b[1].m);
-  assertEq(false, b[0].m === b[2].m);
-  assertEq(false, b[0].m === b[3].m);
-  assertEq(false, b[0].m === b[4].m);
+    var a = [], i, N = HOTLOOP + 2;
+    for (i = 0; i < N; i++)
+        a[i] = {m: i};
+    for (i = 0; i < N; i++)
+        a[i].m = function() { return 0; };
+    assertEq(a[N - 2].m === a[N - 1].m, false);
 }
 f();
+f();
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/pic/to-dictionary.js
@@ -0,0 +1,9 @@
+function f() {
+    var MAX_HEIGHT = 64;
+    var obj = {};
+    for (var i = 0; i < MAX_HEIGHT; i++)
+        obj['a' + i] = i;
+    obj.m = function () { return 0; };
+}
+f();
+f();
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -4622,22 +4622,26 @@ js_DefineNativeProperty(JSContext *cx, J
     /* XXXbe called with lock held */
     valueCopy = value;
     if (!CallAddPropertyHook(cx, clasp, obj, shape, &valueCopy)) {
         obj->removeProperty(cx, id);
         return false;
     }
 
     if (defineHow & JSDNP_CACHE_RESULT) {
+        JS_ASSERT_NOT_ON_TRACE(cx);
+        if (added) {
 #ifdef JS_TRACER
-        JS_ASSERT_NOT_ON_TRACE(cx);
-        PropertyCacheEntry *entry =
+            PropertyCacheEntry *entry =
 #endif
-            JS_PROPERTY_CACHE(cx).fill(cx, obj, 0, 0, obj, shape, added);
-        TRACE_2(SetPropHit, entry, shape);
+                JS_PROPERTY_CACHE(cx).fill(cx, obj, 0, 0, obj, shape, true);
+            TRACE_2(SetPropHit, entry, shape);
+        } else {
+            TRACE_2(SetPropHit, JS_NO_PROP_CACHE_FILL, shape);
+        }
     }
     if (propp)
         *propp = (JSProperty *) shape;
     return true;
 
 #ifdef JS_TRACER
 error: // TRACE_2 jumps here on error.
 #endif
@@ -5489,32 +5493,49 @@ js_SetPropertyHelper(JSContext *cx, JSOb
 
             /*
              * Forget we found the proto-property now that we've copied any
              * needed member values.
              */
             shape = NULL;
         }
 
-        if (shape) {
-            if (shape->isMethod()) {
-                JS_ASSERT(pobj->hasMethodBarrier());
-            } else if ((defineHow & JSDNP_SET_METHOD) && obj->canHaveMethodBarrier()) {
+        JS_ASSERT_IF(shape && shape->isMethod(), pobj->hasMethodBarrier());
+        JS_ASSERT_IF(shape && shape->isMethod(),
+                     &pobj->getSlot(shape->slot).toObject() == &shape->methodObject());
+        if (shape && (defineHow & JSDNP_SET_METHOD)) {
+            /*
+             * JSOP_SETMETHOD is assigning to an existing own property. If it
+             * is an identical method property, do nothing. Otherwise downgrade
+             * to ordinary assignment. Either way, do not fill the property
+             * cache, as the interpreter has no fast path for these unusual
+             * cases.
+             */
+            bool identical = shape->isMethod() && &shape->methodObject() == &vp->toObject();
+            if (!identical) {
+                if (!obj->methodShapeChange(cx, *shape))
+                    return false;
+
                 JS_ASSERT(IsFunctionObject(*vp));
                 JS_ASSERT(!(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
 
                 JSObject *funobj = &vp->toObject();
                 JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
                 if (fun == funobj) {
                     funobj = CloneFunctionObject(cx, fun, fun->parent);
                     if (!funobj)
                         return JS_FALSE;
                     vp->setObject(*funobj);
                 }
             }
+            if (defineHow & JSDNP_CACHE_RESULT) {
+                JS_ASSERT_NOT_ON_TRACE(cx);
+                TRACE_2(SetPropHit, JS_NO_PROP_CACHE_FILL, shape);
+            }
+            return identical || js_NativeSet(cx, obj, shape, false, vp);
         }
     }
 
     added = false;
     if (!shape) {
         if (!obj->isExtensible()) {
             if (defineHow & JSDNP_CACHE_RESULT) {
                 JS_ASSERT_NOT_ON_TRACE(cx);
@@ -6577,16 +6598,18 @@ DumpShape(const Shape &shape)
     fprintf(stderr, "    ");
     if (attrs & JSPROP_ENUMERATE) fprintf(stderr, "enumerate ");
     if (attrs & JSPROP_READONLY) fprintf(stderr, "readonly ");
     if (attrs & JSPROP_PERMANENT) fprintf(stderr, "permanent ");
     if (attrs & JSPROP_GETTER) fprintf(stderr, "getter ");
     if (attrs & JSPROP_SETTER) fprintf(stderr, "setter ");
     if (attrs & JSPROP_SHARED) fprintf(stderr, "shared ");
     if (shape.isAlias()) fprintf(stderr, "alias ");
+    if (shape.isMethod()) fprintf(stderr, "method(%p) ", (void *) &shape.methodObject());
+
     if (JSID_IS_ATOM(id))
         dumpString(JSID_TO_STRING(id));
     else if (JSID_IS_INT(id))
         fprintf(stderr, "%d", (int) JSID_TO_INT(id));
     else
         fprintf(stderr, "unknown jsid %p", (void *) JSID_BITS(id));
     fprintf(stderr, ": slot %d", shape.slot);
     fprintf(stderr, "\n");
--- a/js/src/methodjit/PolyIC.cpp
+++ b/js/src/methodjit/PolyIC.cpp
@@ -571,22 +571,28 @@ class SetPropCompiler : public PICStubCo
                 JSObject *funobj = &f.regs.sp[-1].toObject();
                 if (funobj != GET_FUNCTION_PRIVATE(cx, funobj))
                     return disable("mismatched function");
 
                 flags |= Shape::METHOD;
                 getter = CastAsPropertyOp(funobj);
             }
 
+            /*
+             * Define the property but do not set it yet. For setmethod,
+             * populate the slot to satisfy the method invariant (in case we
+             * hit an early return below).
+             */
             const Shape *shape =
                 obj->putProperty(cx, id, getter, clasp->setProperty,
                                  SHAPE_INVALID_SLOT, JSPROP_ENUMERATE, flags, 0);
-
             if (!shape)
                 return error();
+            if (flags & Shape::METHOD)
+                obj->nativeSetSlot(shape->slot, f.regs.sp[-1]);
 
             /*
              * Test after calling putProperty since it can switch obj into
              * dictionary mode, specifically if the shape tree ancestor line
              * exceeds PropertyTree::MAX_HEIGHT.
              */
             if (obj->inDictionaryMode())
                 return disable("dictionary");