Fix method barrier not to brand, period (branding without reshaping is worse, branding correctly is unnecessary; 524826, r=jorendorff).

js/src/jsinterp.cpp

@@ -2586,7 +2586,7 @@ AssertValidPropertyCacheHit(JSContext *cx, JSScript *script, JSFrameRegs& regs,
         jsval v;
         JS_ASSERT(PCVAL_IS_OBJECT(entry->vword));
         JS_ASSERT(entry->vword != PCVAL_NULL);
-        JS_ASSERT(OBJ_SCOPE(pobj)->branded());
+        JS_ASSERT(OBJ_SCOPE(pobj)->branded() || OBJ_SCOPE(pobj)->hasMethodBarrier());
         JS_ASSERT(SPROP_HAS_STUB_GETTER_OR_IS_METHOD(sprop));
         JS_ASSERT(SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj)));
         v = LOCKED_OBJ_GET_SLOT(pobj, sprop->slot);

js/src/jsops.cpp

@@ -3263,12 +3263,6 @@ BEGIN_CASE(JSOP_LAMBDA)
                 /*
                  * Optimize ({method: function () { ... }, ...}) and
                  * this.method = function () { ... }; bytecode sequences.
-                 *
-                 * Note that we jump to the entry points for JSOP_SETPROP and
-                 * JSOP_INITPROP without calling the trace recorder, because
-                 * the record hooks for those ops are essentially no-ops (this
-                 * can't change given the predictive shape guarding the
-                 * recorder must do).
                  */
                 if (op == JSOP_SETMETHOD) {
 #ifdef DEBUG
@@ -3585,19 +3579,7 @@ BEGIN_CASE(JSOP_INITMETHOD)
                 JS_ASSERT(sprop2 == sprop);
             } else {
                 JS_ASSERT(scope->owned());
-
-                /* Inline-specialized version of JSScope::extend. */
-                js_LeaveTraceIfGlobalObject(cx, obj);
-                scope->shape = sprop->shape;
-                ++scope->entryCount;
-                scope->lastProp = sprop;
-
-                jsuint index;
-                if (js_IdIsIndex(sprop->id, &index))
-                    scope->setIndexedProperties();
-
-                if (sprop->isMethod())
-                    scope->setMethodBarrier();
+                scope->extend(cx, sprop);
             }
 
             /*

js/src/jsscope.h

@@ -364,7 +364,7 @@ struct JSScope : public JSObjectMap
      * function objects (functions that do not use lexical bindings above their
      * scope, only free variable names) that have a correct JSSLOT_PARENT value
      * thanks to the COMPILE_N_GO optimization are stored as newly added direct
-     * property values.
+     * property values of the scope's object.
      *
      * The de-facto standard JS language requires each evaluation of such a
      * closure to result in a unique (according to === and observable effects)
@@ -374,14 +374,15 @@ struct JSScope : public JSObjectMap
      * implementations that join and do not join.
      *
      * To stay compatible with the de-facto standard, we store the compiler-
-     * created function object as the method value, set the METHOD_BARRIER
-     * flag, and brand the scope with a predictable shape that reflects its
-     * method values, which are cached and traced without being loaded, based
-     * on shape-qualified cache hit logic and equivalent trace guards. See
-     * BRANDED above.
+     * created function object as the method value and set the METHOD_BARRIER
+     * flag.
      *
-     * This means scope->hasMethodBarrier() => scope->branded(), but of course
-     * not the other way around.
+     * The method value is part of the method property tree node's identity, so
+     * it effectively  brands the scope with a predictable shape corresponding
+     * to the method value, but without the overhead of setting the BRANDED
+     * flag, which requires assigning a new shape peculiar to each branded
+     * scope. Instead the shape is shared via the property tree among all the
+     * scopes referencing the method property tree node.
      *
      * Then when reading from a scope for which scope->hasMethodBarrier() is
      * true, we count on the scope's qualified/guarded shape being unique and
@@ -390,10 +391,16 @@ struct JSScope : public JSObjectMap
      *
      * This read barrier is bypassed when evaluating the callee sub-expression
      * of a call expression (see the JOF_CALLOP opcodes in jsopcode.tbl), since
-     * such ops do not present an identity or mutation hazard.
+     * such ops do not present an identity or mutation hazard. The compiler
+     * performs this optimization only for null closures that do not use their
+     * own name or equivalent built-in references (arguments.callee).
+     *
+     * The BRANDED write barrier, JSScope::methodWriteBarrer, must check for
+     * METHOD_BARRIER too, and regenerate this scope's shape if the method's
+     * value is in fact changing.
      */
     bool hasMethodBarrier()     { return flags & METHOD_BARRIER; }
-    void setMethodBarrier()     { flags |= METHOD_BARRIER | BRANDED; }
+    void setMethodBarrier()     { flags |= METHOD_BARRIER; }
 
     bool owned()                { return object != NULL; }
 };

js/src/jsscopeinlines.h

@@ -42,6 +42,7 @@
 
 #include "jscntxt.h"
 #include "jsfun.h"
+#include "jsinterp.h"
 #include "jsobj.h"
 #include "jsscope.h"
 
@@ -90,7 +91,7 @@ JSScope::methodReadBarrier(JSContext *cx, JSScopeProperty *sprop, jsval *vp)
 inline bool
 JSScope::methodWriteBarrier(JSContext *cx, JSScopeProperty *sprop, jsval v)
 {
-    if (branded()) {
+    if (flags & (BRANDED | METHOD_BARRIER)) {
         jsval prev = LOCKED_OBJ_GET_SLOT(object, sprop->slot);
 
         if (prev != v && VALUE_IS_FUNCTION(cx, prev))
@@ -102,7 +103,7 @@ JSScope::methodWriteBarrier(JSContext *cx, JSScopeProperty *sprop, jsval v)
 inline bool
 JSScope::methodWriteBarrier(JSContext *cx, uint32 slot, jsval v)
 {
-    if (branded()) {
+    if (flags & (BRANDED | METHOD_BARRIER)) {
         jsval prev = LOCKED_OBJ_GET_SLOT(object, slot);
 
         if (prev != v && VALUE_IS_FUNCTION(cx, prev))

js/src/jstracer.cpp

@@ -7034,7 +7034,7 @@ JS_REQUIRES_STACK AbortableRecordingStatus
 TraceRecorder::monitorRecording(JSOp op)
 {
     JSTraceMonitor &localtm = JS_TRACE_MONITOR(cx);
-    JSContext *localcx = cx;
+    debug_only_stmt( JSContext *localcx = cx; )
 
     /* Process needFlush requests now. */
     if (localtm.needFlush) {
@@ -13388,11 +13388,23 @@ TraceRecorder::record_JSOP_LAMBDA()
      * via identity and mutation. But don't clone if our result is consumed by
      * JSOP_SETMETHOD or JSOP_INITMETHOD, since we optimize away the clone for
      * these combinations and clone only if the "method value" escapes.
+     *
+     * See jsops.cpp, the JSOP_LAMBDA null closure case. The JSOP_SETMETHOD and
+     * JSOP_INITMETHOD logic governing the early ARECORD_CONTINUE returns below
+     * must agree with the corresponding break-from-do-while(0) logic there.
      */
     if (FUN_NULL_CLOSURE(fun) && OBJ_GET_PARENT(cx, FUN_OBJECT(fun)) == globalObj) {
         JSOp op2 = JSOp(cx->fp->regs->pc[JSOP_LAMBDA_LENGTH]);
 
-        if (op2 == JSOP_SETMETHOD || op2 == JSOP_INITMETHOD) {
+        if (op2 == JSOP_SETMETHOD) {
+            jsval lval = stackval(-1);
+
+            if (!JSVAL_IS_PRIMITIVE(lval) &&
+                OBJ_GET_CLASS(cx, JSVAL_TO_OBJECT(lval)) == &js_ObjectClass) {
+                stack(0, INS_CONSTOBJ(FUN_OBJECT(fun)));
+                return ARECORD_CONTINUE;
+            }
+        } else if (op2 == JSOP_INITMETHOD) {
             stack(0, INS_CONSTOBJ(FUN_OBJECT(fun)));
             return ARECORD_CONTINUE;
         }

js/src/tests/ecma_3/FunExpr/jstests.list

@@ -3,3 +3,4 @@ script fe-001-n.js
 script fe-001.js
 script fe-002.js
 script regress-518103.js
+script regress-524826.js

js/src/tests/ecma_3/FunExpr/regress-524826.js

@@ -0,0 +1,29 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+var gTestfile = 'regress-524826.js';
+var BUGNUMBER = 524826;
+var summary = 'null-closure property initialiser mis-brands object literal scope';
+var actual;
+var expect;
+
+printBugNumber(BUGNUMBER);
+printStatus(summary);
+
+function make(g) {
+    var o = {f: function(a,b) { return a*b; }, g: g};
+    return o;
+}
+var z = -1;
+var x = make(function(c) { return c*z; });
+var y = make(function(c) { return -c*z; });
+
+function callg(o, c) { return o.g(c); };
+actual = callg(x, 1);
+expect = -callg(y, 1);
+
+reportCompare(expect, actual, summary);
+
+printStatus("All tests passed!");

js/src/trace-test/tests/basic/bug524826.js

@@ -0,0 +1,10 @@
+var x = 42;
+function f() {
+    var a = [new Date, new Date, new Date, new Date, new Date];
+    for (var i = 0; i < 5; i++)
+        a[i].m = function () {return x};
+    for (i = 0; i < 4; i++)
+        if (a[i].m == a[i+1].m)
+            throw "FAIL!";
+}
+f();