Bug 554955: Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. r=jorendorff

The comments for js::Bindings::extensibleParents explain why this is necessary.

AssertValidPropertyCacheHit should have been catching this bug, but for
reasons I don't understand, it is restricted from checking this case. This
patch extends it to assert when the bug is detected.

I've gathered the infallible parts of the initialization for Call objects
and cloned block objects into their own functions.

js/src/jsemit.cpp

@@ -1956,6 +1956,15 @@ EmitEnterBlock(JSContext *cx, JSParseNode *pn, JSCodeGenerator *cg)
         blockObj->setSlot(slot, BooleanValue(isClosed));
     }
 
+    /*
+     * If clones of this block will have any extensible parents, then the clones
+     * must get unique shapes; see the comments for js::Bindings::
+     * extensibleParents.
+     */
+    if ((cg->flags & TCF_FUN_EXTENSIBLE_SCOPE) ||
+        cg->bindings.extensibleParents())
+        blockObj->setBlockOwnShape(cx);
+
     return true;
 }
 

js/src/jsemit.h

@@ -266,6 +266,16 @@ struct JSStmtInfo {
  */
 #define TCF_IN_WITH             0x10000000
 
+/* 
+ * This function does something that can extend the set of bindings in its
+ * call objects --- it does a direct eval in non-strict code, or includes a
+ * function statement (as opposed to a function definition).
+ *
+ * This flag is *not* inherited by enclosed or enclosing functions; it
+ * applies only to the function in whose flags it appears.
+ */
+#define TCF_FUN_EXTENSIBLE_SCOPE 0x20000000
+
 /*
  * Flags to check for return; vs. return expr; in a function.
  */
@@ -284,7 +294,8 @@ struct JSStmtInfo {
                                  TCF_FUN_CALLS_EVAL      |                    \
                                  TCF_FUN_MIGHT_ALIAS_LOCALS |                 \
                                  TCF_FUN_MUTATES_PARAMETER |                  \
-                                 TCF_STRICT_MODE_CODE)
+                                 TCF_STRICT_MODE_CODE    |                    \
+                                 TCF_FUN_EXTENSIBLE_SCOPE)
 
 struct JSTreeContext {              /* tree context for semantic checks */
     uint32          flags;          /* statement state flags, see above */
@@ -457,6 +468,14 @@ struct JSTreeContext {              /* tree context for semantic checks */
     bool needsEagerArguments() const {
         return inStrictMode() && ((usesArguments() && mutatesParameter()) || callsEval());
     }
+
+    void noteHasExtensibleScope() {
+        flags |= TCF_FUN_EXTENSIBLE_SCOPE;
+    }
+
+    bool hasExtensibleScope() const {
+        return flags & TCF_FUN_EXTENSIBLE_SCOPE;
+    }
 };
 
 /*

js/src/jsfun.cpp

@@ -964,8 +964,7 @@ NewCallObject(JSContext *cx, Bindings *bindings, JSObject &scopeChain, JSObject
         return NULL;
 
     /* Init immediately to avoid GC seeing a half-init'ed object. */
-    callobj->init(cx, &js_CallClass, NULL, &scopeChain, NULL, false);
-    callobj->setMap(bindings->lastShape());
+    callobj->initCall(cx, bindings, &scopeChain);
 
     /* This must come after callobj->lastProp has been set. */
     if (!callobj->ensureInstanceReservedSlots(cx, argsVars))

js/src/jsfun.h

@@ -96,7 +96,7 @@
                                        JSFunctionSpec::call points to a
                                        JSNativeTraceInfo. */
 #define JSFUN_INTERPRETED   0x4000  /* use u.i if kind >= this value else u.n */
-#define JSFUN_FLAT_CLOSURE  0x8000  /* flag (aka "display") closure */
+#define JSFUN_FLAT_CLOSURE  0x8000  /* flat (aka "display") closure */
 #define JSFUN_NULL_CLOSURE  0xc000  /* null closure entrains no scope chain */
 #define JSFUN_KINDMASK      0xc000  /* encode interp vs. native and closure
                                        optimization level -- see above */

js/src/jsinterp.cpp

@@ -2045,7 +2045,7 @@ AssertValidPropertyCacheHit(JSContext *cx, JSScript *script, JSFrameRegs& regs,
     }
     if (!ok)
         return false;
-    if (cx->runtime->gcNumber != sample || entry->vshape() != pobj->shape())
+    if (cx->runtime->gcNumber != sample)
         return true;
     JS_ASSERT(prop);
     JS_ASSERT(pobj == found);

js/src/jsobj.cpp

@@ -3287,9 +3287,8 @@ js_CloneBlockObject(JSContext *cx, JSObject *proto, JSStackFrame *fp)
     JSStackFrame *priv = js_FloatingFrameIfGenerator(cx, fp);
 
     /* The caller sets parent on its own. */
-    clone->init(cx, &js_BlockClass, proto, NULL, priv, false);
+    clone->initClonedBlock(cx, proto, priv);
 
-    clone->setMap(proto->map);
     if (!clone->ensureInstanceReservedSlots(cx, count + 1))
         return NULL;
 

js/src/jsobj.h

@@ -515,6 +515,11 @@ struct JSObject : js::gc::Cell {
         setMap(const_cast<JSObjectMap *>(&JSObjectMap::sharedNonNative));
     }
 
+    /* Functions for setting up scope chain object maps and shapes. */
+    void initCall(JSContext *cx, const js::Bindings *bindings, JSObject *parent);
+    void initClonedBlock(JSContext *cx, JSObject *proto, JSStackFrame *priv);
+    void setBlockOwnShape(JSContext *cx);
+
     void deletingShapeChange(JSContext *cx, const js::Shape &shape);
     const js::Shape *methodShapeChange(JSContext *cx, const js::Shape &shape);
     bool methodShapeChange(JSContext *cx, uint32 slot);

js/src/jsobjinlines.h

@@ -59,6 +59,7 @@
 #include "jscntxt.h"
 #include "jsnum.h"
 #include "jsscopeinlines.h"
+#include "jsscriptinlines.h"
 #include "jsstr.h"
 
 #include "jsfuninlines.h"
@@ -141,6 +142,59 @@ JSObject::finalize(JSContext *cx)
     finish(cx);
 }
 
+/* 
+ * Initializer for Call objects for functions and eval frames. Set class,
+ * parent, map, and shape, and allocate slots.
+ */
+inline void
+JSObject::initCall(JSContext *cx, const js::Bindings *bindings, JSObject *parent)
+{
+    init(cx, &js_CallClass, NULL, parent, NULL, false);
+    map = bindings->lastShape();
+
+    /*
+     * If |bindings| is for a function that has extensible parents, that means
+     * its Call should have its own shape; see js::Bindings::extensibleParents.
+     */
+    if (bindings->extensibleParents())
+        setOwnShape(js_GenerateShape(cx));
+    else
+        objShape = map->shape;
+}
+
+/*
+ * Initializer for cloned block objects. Set class, prototype, frame, map, and
+ * shape.
+ */
+inline void
+JSObject::initClonedBlock(JSContext *cx, JSObject *proto, JSStackFrame *frame)
+{
+    init(cx, &js_BlockClass, proto, NULL, frame, false);
+
+    /* Cloned blocks copy their prototype's map; it had better be shareable. */
+    JS_ASSERT(!proto->inDictionaryMode() || proto->lastProp->frozen());
+    map = proto->map;
+
+    /*
+     * If the prototype has its own shape, that means the clone should, too; see
+     * js::Bindings::extensibleParents.
+     */
+    if (proto->hasOwnShape())
+        setOwnShape(js_GenerateShape(cx));
+    else
+        objShape = map->shape;
+}
+
+/* 
+ * Mark a compile-time block as OWN_SHAPE, indicating that its run-time clones
+ * also need unique shapes. See js::Bindings::extensibleParents.
+ */
+inline void
+JSObject::setBlockOwnShape(JSContext *cx) {
+    JS_ASSERT(isStaticBlock());
+    setOwnShape(js_GenerateShape(cx));
+}
+
 /*
  * Property read barrier for deferred cloning of compiler-created function
  * objects optimized as typically non-escaping, ad-hoc methods in obj.

js/src/jsparse.cpp

@@ -326,6 +326,12 @@ JSFunctionBox::inAnyDynamicScope() const
     return false;
 }
 
+bool
+JSFunctionBox::scopeIsExtensible() const
+{
+    return tcflags & TCF_FUN_EXTENSIBLE_SCOPE;
+}
+
 bool
 JSFunctionBox::shouldUnbrand(uintN methods, uintN slowMethods) const
 {
@@ -2047,6 +2053,7 @@ Parser::analyzeFunctions(JSTreeContext *tc)
         return true;
     if (!markFunArgs(tc->functionList))
         return false;
+    markExtensibleScopeDescendants(tc->functionList, false);
     setFunctionKinds(tc->functionList, &tc->flags);
     return true;
 }
@@ -2660,6 +2667,38 @@ Parser::setFunctionKinds(JSFunctionBox *funbox, uint32 *tcflags)
 #undef FUN_METER
 }
 
+/*
+ * Walk the JSFunctionBox tree looking for functions whose call objects may
+ * acquire new bindings as they execute: non-strict functions that call eval,
+ * and functions that contain function statements (definitions not appearing
+ * within the top statement list, which don't take effect unless they are
+ * evaluated). Such call objects may acquire bindings that shadow variables
+ * defined in enclosing scopes, so any enclosed functions must have their
+ * bindings' extensibleParents flags set, and enclosed compiler-created blocks
+ * must have their OWN_SHAPE flags set; the comments for
+ * js::Bindings::extensibleParents explain why.
+ */
+void
+Parser::markExtensibleScopeDescendants(JSFunctionBox *funbox, bool hasExtensibleParent) 
+{
+    for (; funbox; funbox = funbox->siblings) {
+        /*
+         * It would be nice to use FUN_KIND(fun) here to recognize functions
+         * that will never consult their parent chains, and thus don't need
+         * their 'extensible parents' flag set. Filed as bug 619750. 
+         */
+
+        JS_ASSERT(!funbox->bindings.extensibleParents());
+        if (hasExtensibleParent)
+            funbox->bindings.setExtensibleParents();
+
+        if (funbox->kids) {
+            markExtensibleScopeDescendants(funbox->kids,
+                                           hasExtensibleParent || funbox->scopeIsExtensible());
+        }
+    }
+}
+
 const char js_argument_str[] = "argument";
 const char js_variable_str[] = "variable";
 const char js_unknown_str[]  = "unknown";
@@ -3542,8 +3581,11 @@ Parser::statements()
              */
             if (tc->atBodyLevel())
                 pn->pn_xflags |= PNX_FUNCDEFS;
-            else
+            else {
                 tc->flags |= TCF_HAS_FUNCTION_STMT;
+                /* Function statements extend the Call object at runtime. */
+                tc->noteHasExtensibleScope();
+            }
         }
         pn->append(pn2);
     }
@@ -7689,6 +7731,12 @@ Parser::memberExpr(JSBool allowCallSyntax)
                     pn2->pn_op = JSOP_EVAL;
                     tc->noteCallsEval();
                     tc->flags |= TCF_FUN_HEAVYWEIGHT;
+                    /*
+                     * In non-strict mode code, direct calls to eval can add
+                     * variables to the call object.
+                     */
+                    if (!tc->inStrictMode())
+                        tc->noteHasExtensibleScope();
                 }
             } else if (pn->pn_op == JSOP_GETPROP) {
                 /* Select JSOP_FUNAPPLY given foo.apply(...). */

js/src/jsparse.h

@@ -984,6 +984,12 @@ struct JSFunctionBox : public JSObjectBox
      */
     bool inAnyDynamicScope() const;
 
+    /* 
+     * Must this function's descendants be marked as having an extensible
+     * ancestor?
+     */
+    bool scopeIsExtensible() const;
+
     /*
      * Unbrand an object being initialized or constructed if any method cannot
      * be joined to one compiler-created null closure shared among N different
@@ -1113,6 +1119,7 @@ struct Parser : private js::AutoGCRooter
     bool analyzeFunctions(JSTreeContext *tc);
     void cleanFunctionList(JSFunctionBox **funbox);
     bool markFunArgs(JSFunctionBox *funbox);
+    void markExtensibleScopeDescendants(JSFunctionBox *funbox, bool hasExtensibleParent);
     void setFunctionKinds(JSFunctionBox *funbox, uint32 *tcflags);
 
     void trace(JSTracer *trc);

js/src/jspropertycache.cpp

@@ -61,6 +61,7 @@ PropertyCache::fill(JSContext *cx, JSObject *obj, uintN scopeIndex, uintN protoI
 
     JS_ASSERT(this == &JS_PROPERTY_CACHE(cx));
     JS_ASSERT(!cx->runtime->gcRunning);
+    JS_ASSERT_IF(adding, !obj->isCall());
 
     if (js_IsPropertyCacheDisabled(cx)) {
         PCMETER(disfills++);

js/src/jsprvtd.h

@@ -172,6 +172,7 @@ struct PropertyCacheEntry;
 
 struct Shape;
 struct EmptyShape;
+class Bindings;
 
 } /* namespace js */
 

js/src/jsscript.h

@@ -173,6 +173,7 @@ class Bindings {
     uint16 nargs;
     uint16 nvars;
     uint16 nupvars;
+    bool hasExtensibleParents;
 
   public:
     inline Bindings(JSContext *cx, EmptyShape *emptyCallShape);
@@ -297,6 +298,54 @@ class Bindings {
      */
     void makeImmutable();
 
+    /*
+     * Sometimes call objects and run-time block objects need unique shapes, but
+     * sometimes they don't.
+     *
+     * Property cache entries only record the shapes of the first and last
+     * objects along the search path, so if the search traverses more than those
+     * two objects, then those first and last shapes must determine the shapes
+     * of everything else along the path. The js_PurgeScopeChain stuff takes
+     * care of making this work, but that suffices only because we require that
+     * start points with the same shape have the same successor object in the
+     * search path --- a cache hit means the starting shapes were equal, which
+     * means the seach path tail (everything but the first object in the path)
+     * was shared, which in turn means the effects of a purge will be seen by
+     * all affected starting search points.
+     *
+     * For call and run-time block objects, the "successor object" is the scope
+     * chain parent. Unlike prototype objects (of which there are usually few),
+     * scope chain parents are created frequently (possibly on every call), so
+     * following the shape-implies-parent rule blindly would lead one to give
+     * every call and block its own shape.
+     *
+     * In many cases, however, it's not actually necessary to give call and
+     * block objects their own shapes, and we can do better. If the code will
+     * always be used with the same global object, and none of the enclosing
+     * call objects could have bindings added to them at runtime (by direct eval
+     * calls or function statements), then we can use a fixed set of shapes for
+     * those objects. You could think of the shapes in the functions' bindings
+     * and compile-time blocks as uniquely identifying the global object(s) at
+     * the end of the scope chain.
+     *
+     * (In fact, some JSScripts we do use against multiple global objects (see
+     * bug 618497), and using the fixed shapes isn't sound there.)
+     * 
+     * In deciding whether a call or block has any extensible parents, we
+     * actually only need to consider enclosing calls; blocks are never
+     * extensible, and the other sorts of objects that appear in the scope
+     * chains ('with' blocks, say) are not CacheableNonGlobalScopes.
+     *
+     * If the hasExtensibleParents flag is set, then Call objects created for
+     * the function this Bindings describes need unique shapes. If the flag is
+     * clear, then we can use lastBinding's shape.
+     *
+     * For blocks, we set the the OWN_SHAPE flag on the compiler-generated
+     * blocksto indicate that their clones need unique shapes.
+     */
+    void setExtensibleParents() { hasExtensibleParents = true; }
+    bool extensibleParents() const { return hasExtensibleParents; }
+
     /*
      * These methods provide direct access to the shape path normally
      * encapsulated by js::Bindings. These methods may be used to make a

js/src/jsscriptinlines.h

@@ -52,7 +52,8 @@ namespace js {
 
 inline
 Bindings::Bindings(JSContext *cx, EmptyShape *emptyCallShape)
-  : lastBinding(emptyCallShape), nargs(0), nvars(0), nupvars(0)
+  : lastBinding(emptyCallShape), nargs(0), nvars(0), nupvars(0),
+    hasExtensibleParents(false)
 {
 }
 

js/src/tests/js1_8_5/regress/jstests.list

@@ -13,6 +13,12 @@ script regress-551763-1.js
 script regress-551763-2.js
 script regress-552432.js
 script regress-553778.js
+script regress-554955-1.js
+script regress-554955-2.js
+script regress-554955-3.js
+script regress-554955-4.js
+script regress-554955-5.js
+script regress-554955-6.js
 script regress-555246-0.js
 script regress-555246-1.js
 script regress-559402-1.js

js/src/tests/js1_8_5/regress/regress-554955-1.js

@@ -0,0 +1,32 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+function f(s) {
+  eval(s);
+  return function(a) {
+    var d;
+    let (c = 3) {
+      d = function() { a; }; // force Block object to be cloned
+      with({}) {}; // repel JägerMonkey
+      return b; // lookup occurs in scope of Block
+    }
+  };
+}
+
+var b = 1;
+var g1 = f("");
+var g2 = f("var b = 2;");
+
+/* Call the lambda once, caching a reference to the global b. */
+g1(0);
+
+/* 
+ * If this call sees the above cache entry, then it will erroneously use the
+ * global b.
+ */
+assertEq(g2(0), 2);
+
+reportCompare(true, true);

js/src/tests/js1_8_5/regress/regress-554955-2.js

@@ -0,0 +1,29 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+function f(s) {
+    eval(s);
+    return function(a) {
+        with({}) {}; // repel JägerMonkey
+        eval(a);
+        return b;
+    };
+}
+
+var b = 1;
+var g1 = f("");
+var g2 = f("var b = 2;");
+
+/* Call the lambda once, caching a reference to the global b. */
+g1('');
+
+/*
+ * If this call sees the above cache entry, then it will erroneously use
+ * the global b.
+ */
+assertEq(g2(''), 2);
+
+reportCompare(true, true);

js/src/tests/js1_8_5/regress/regress-554955-3.js

@@ -0,0 +1,31 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+function f(s) {
+    eval(s);
+    return function(a) {
+        with({}) {}; // repel JägerMonkey
+        eval(a);
+        let (c = 3) {
+            return b;
+        };
+    };
+}
+
+var b = 1;
+var g1 = f("");
+var g2 = f("var b = 2;");
+
+/* Call the lambda once, caching a reference to the global b. */
+g1('');
+
+/* 
+ * If this call sees the above cache entry, then it will erroneously use the
+ * global b.
+ */
+assertEq(g2(''), 2);
+
+reportCompare(true, true);

js/src/tests/js1_8_5/regress/regress-554955-4.js

@@ -0,0 +1,36 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+function f() {
+    return function(a) {
+        // This eval must take place before the block is cloned, when the
+        // call object is still not marked as a delegate. The scope chain
+        // purge for the JSOP_DEFVAR will not change the global's shape,
+        // and the property cache entry will remain valid.
+        eval(a);
+        let (c = 3) {
+            // This eval forces the block to be cloned, so its shape gets
+            // used as the property cache key shape.
+            eval('');
+            return b;
+        };
+    };
+}
+
+var b = 1;
+var g1 = f();
+var g2 = f();
+
+/* Call the lambda once, caching a reference to the global b. */
+g1('');
+
+/*
+ * If this call sees the above cache entry, then it will erroneously use the
+ * global b.
+ */
+assertEq(g2('var b=2'), 2);
+
+reportCompare(true, true);

js/src/tests/js1_8_5/regress/regress-554955-5.js

@@ -0,0 +1,30 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+function f(s) {
+    if (s) {
+        function b() { }
+    }
+    return function(a) {
+        eval(a);
+        return b;
+    };
+}
+
+var b = 1;
+var g1 = f(false);
+var g2 = f(true);
+
+/* Call the lambda once, caching a reference to the global b. */
+g1('');
+
+/*
+ * If this call sees the above cache entry, then it will erroneously use the
+ * global b.
+ */
+assertEq(typeof g2(''), "function");
+
+reportCompare(true, true);

js/src/tests/js1_8_5/regress/regress-554955-6.js

@@ -0,0 +1,47 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+var v="global";
+function f(a) {
+  // This eval could extend f's call object. However, the call object has
+  // not yet been marked as a delegate at this point, so no scope chain
+  // purge takes place when it is extended.
+  eval(a);
+  let (b=3) {
+    // This eval causes the cloned block object to be added to the
+    // scope chain. The block needs a unique shape: its parent call
+    // could acquire bindings for anything without affecting the global
+    // object's shape, so it's up to the block's shape to mismatch all
+    // property cache entries for prior blocks.
+    eval("");
+    return v;
+  };
+}
+
+// Call the function once, to cache a reference to the global v from within
+// f's lexical block.
+assertEq("global", f(""));
+
+// Call the function again, adding a binding to the call, and ensure that
+// we do not see any property cache entry created by the previous reference
+// that would direct us to the global definition.
+assertEq("local", f("var v='local'"));
+
+// Similarly,but with a doubly-nested block; make sure everyone gets marked.
+function f2(a) {
+  eval(a);
+  let (b=3) {
+    let (c=4) {
+      eval("");
+      return v;
+    };
+  };
+}
+
+assertEq("global", f2(""));
+assertEq("local",  f2("var v='local'"));
+
+reportCompare(true, true);