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

@@ -3666,9 +3666,30 @@ js_EmitFunctionScript(JSContext *cx, JSCodeGenerator *cg, JSParseNode *body)
         CG_SWITCH_TO_MAIN(cg);
     }
 
-    return js_EmitTree(cx, cg, body) &&
-           js_Emit1(cx, cg, JSOP_STOP) >= 0 &&
-           JSScript::NewScriptFromCG(cx, cg);
+    if (!js_EmitTree(cx, cg, body) ||
+        js_Emit1(cx, cg, JSOP_STOP) < 0 ||
+        !JSScript::NewScriptFromCG(cx, cg)) {
+        return false;
+    }
+
+    JSFunction *fun = cg->fun();
+    if (fun->script()->bindings.extensibleParents()) {
+        /*
+         * Since the function has extensible parents, its blocks need unique
+         * shapes. See the comments for js::Bindings::extensibleParents.
+         */
+        JSScript *script = FUN_SCRIPT(fun);
+        if (JSScript::isValidOffset(script->objectsOffset)) {
+            JSObjectArray *objects = FUN_SCRIPT(fun)->objects();
+            for (uint32 i = 0; i < objects->length; i++) {
+                JSObject *obj = objects->vector[i];
+                if (obj->isBlock())
+                    obj->setBlockOwnShape(cx);
+            }
+        }
+    }
+
+    return true;
 }
 
 /* A macro for inlining at the top of js_EmitTree (whence it came). */

js/src/jsfun.cpp

@@ -958,8 +958,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

@@ -2090,7 +2090,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

@@ -3264,9 +3264,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

@@ -484,6 +484,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

@@ -58,6 +58,7 @@
 #include "jscntxt.h"
 #include "jsnum.h"
 #include "jsscopeinlines.h"
+#include "jsscriptinlines.h"
 #include "jsstr.h"
 
 #include "jsgcinlines.h"
@@ -139,6 +140,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, false));
+    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, false));
+    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, false));
+}
+
 /*
  * 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

@@ -322,6 +322,23 @@ JSFunctionBox::inAnyDynamicScope() const
     return false;
 }
 
+bool
+JSFunctionBox::scopeIsExtensible() const
+{
+    /* Direct eval calls can add bindings, but not in strict mode functions. */
+    if ((tcflags & TCF_FUN_CALLS_EVAL) && !(tcflags & TCF_STRICT_MODE_CODE))
+        return true;
+
+    /* 
+     * Function statements also extend call objects. (We forbid function
+     * statements in strict mode code.)
+     */
+    if (tcflags & TCF_HAS_FUNCTION_STMT)
+        return true;
+
+    return false;
+}
+
 bool
 JSFunctionBox::shouldUnbrand(uintN methods, uintN slowMethods) const
 {
@@ -2027,6 +2044,7 @@ Parser::analyzeFunctions(JSTreeContext *tc)
         return true;
     if (!markFunArgs(tc->functionList))
         return false;
+    markExtensibleScopeDescendants(tc->functionList, false);
     setFunctionKinds(tc->functionList, &tc->flags);
     return true;
 }
@@ -2633,6 +2651,37 @@ 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). Enclosed functions that could refer to bindings these extensible
+ * call objects can shadow must have their bindings' extensibleParents flags
+ * set, and such 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";

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
@@ -1112,6 +1118,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

@@ -171,6 +171,7 @@ struct PropertyCacheEntry;
 
 struct Shape;
 struct EmptyShape;
+struct 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);
@@ -303,6 +304,43 @@ 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 --- if two start points have different successors, they must
+     * have different shapes.
+     *
+     * For call and run-time block objects, the "successor" 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 it's
+     * not too far from optimal 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, so 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.)
+     * 
+     * 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.
+     */
+    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)
-  : lastBinding(cx->runtime->emptyCallShape), nargs(0), nvars(0), nupvars(0)
+  : lastBinding(cx->runtime->emptyCallShape), nargs(0), nvars(0), nupvars(0),
+    hasExtensibleParents(false)
 {
 }
 

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

@@ -13,6 +13,11 @@ 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-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,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) {
+        eval(a);
+        let (c = 3) {
+            eval(s);
+            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);