Bug 793823 - Store an InternalHandle in BindingIter; r=billm

Currently, BindingIter stores a Bindings*: this is sometimes a pointer into a
JSScript, which causes the analysis to poison it.  This patch straightforwardly
replaces this Bindings* with an InternalBindingsHandle.

--HG--
extra : rebase_source : bc780da603be1ae24fbbdf2d304d3fdb53de130e

js/src/frontend/BytecodeEmitter.cpp

@@ -886,14 +886,14 @@ ClonedBlockDepth(BytecodeEmitter *bce)
 }
 
 static uint16_t
-AliasedNameToSlot(JSScript *script, PropertyName *name)
+AliasedNameToSlot(HandleScript script, PropertyName *name)
 {
     /*
      * Beware: BindingIter may contain more than one Binding for a given name
      * (in the case of |function f(x,x) {}|) but only one will be aliased.
      */
     unsigned slot = CallObject::RESERVED_SLOTS;
-    for (BindingIter bi(script->bindings); ; bi++) {
+    for (BindingIter bi(script); ; bi++) {
         if (bi->aliased()) {
             if (bi->name() == name)
                 return slot;
@@ -2621,7 +2621,8 @@ frontend::EmitFunctionScript(JSContext *cx, BytecodeEmitter *bce, ParseNode *bod
         bce->switchToProlog();
         if (Emit1(cx, bce, JSOP_ARGUMENTS) < 0)
             return false;
-        unsigned varIndex = bce->script->bindings.argumentsVarIndex(cx);
+        InternalBindingsHandle bindings(bce->script, &bce->script->bindings);
+        unsigned varIndex = Bindings::argumentsVarIndex(cx, bindings);
         if (bce->script->varIsAliased(varIndex)) {
             ScopeCoordinate sc;
             sc.hops = 0;
@@ -4925,7 +4926,7 @@ EmitFunc(JSContext *cx, BytecodeEmitter *bce, ParseNode *pn)
             return false;
     } else {
 #ifdef DEBUG
-        BindingIter bi(bce->script->bindings);
+        BindingIter bi(bce->script);
         while (bi->name() != fun->atom())
             bi++;
         JS_ASSERT(bi->kind() == VARIABLE || bi->kind() == CONSTANT || bi->kind() == ARGUMENT);

js/src/jsanalyze.cpp

@@ -141,7 +141,8 @@ ScriptAnalysis::analyzeBytecode(JSContext *cx)
     bool allVarsAliased = script_->compartment()->debugMode();
     bool allArgsAliased = allVarsAliased || script_->argumentsHasVarBinding();
 
-    for (BindingIter bi(script_->bindings); bi; bi++) {
+    RootedScript script(cx, script_);
+    for (BindingIter bi(script); bi; bi++) {
         if (bi->kind() == ARGUMENT)
             escapedSlots[ArgSlot(bi.frameIndex())] = allArgsAliased || bi->aliased();
         else

js/src/jsdbgapi.cpp

@@ -411,7 +411,7 @@ extern JS_PUBLIC_API(uintptr_t *)
 JS_GetFunctionLocalNameArray(JSContext *cx, JSFunction *fun, void **markp)
 {
     BindingVector bindings(cx);
-    if (!FillBindingVector(fun->script()->bindings, &bindings))
+    if (!FillBindingVector(fun->script(), &bindings))
         return NULL;
 
     /* Munge data into the API this method implements.  Avert your eyes! */

js/src/jsfun.cpp

@@ -679,7 +679,7 @@ js::FunctionToString(JSContext *cx, HandleFunction fun, bool bodyOnly, bool lamb
             // Fish out the argument names.
             BindingVector *localNames = cx->new_<BindingVector>(cx);
             js::ScopedDeletePtr<BindingVector> freeNames(localNames);
-            if (!FillBindingVector(script->bindings, localNames))
+            if (!FillBindingVector(script, localNames))
                 return NULL;
             for (unsigned i = 0; i < fun->nargs; i++) {
                 if ((i && !out.append(", ")) ||

js/src/jsopcode.cpp

@@ -1078,7 +1078,8 @@ SetPrinterLocalNames(JSContext *cx, JSScript *script, JSPrinter *jp)
     BindingVector *localNames = NULL;
     if (script->bindings.count() > 0) {
         localNames = cx->new_<BindingVector>(cx);
-        if (!localNames || !FillBindingVector(script->bindings, localNames))
+        RootedScript script_(cx, script);
+        if (!localNames || !FillBindingVector(script_, localNames))
             return false;
     }
     jp->localNames = localNames;
@@ -6047,7 +6048,8 @@ ExpressionDecompiler::init()
     localNames = cx->new_<BindingVector>(cx);
     if (!localNames)
         return false;
-    if (!FillBindingVector(script->bindings, localNames))
+    RootedScript script_(cx, script);
+    if (!FillBindingVector(script_, localNames))
         return false;
 
     return true;

js/src/jsscript.cpp

@@ -50,18 +50,18 @@ using namespace js;
 using namespace js::gc;
 using namespace js::frontend;
 
-unsigned
-Bindings::argumentsVarIndex(JSContext *cx) const
+/* static */ unsigned
+Bindings::argumentsVarIndex(JSContext *cx, InternalBindingsHandle bindings)
 {
     HandlePropertyName arguments = cx->names().arguments;
-    BindingIter bi(*this);
+    BindingIter bi(bindings);
     while (bi->name() != arguments)
         bi++;
     return bi.frameIndex();
 }
 
 bool
-Bindings::initWithTemporaryStorage(JSContext *cx, InternalHandle<Bindings*> self,
+Bindings::initWithTemporaryStorage(JSContext *cx, InternalBindingsHandle self,
                                    unsigned numArgs, unsigned numVars,
                                    Binding *bindingArray)
 {
@@ -106,7 +106,7 @@ Bindings::initWithTemporaryStorage(JSContext *cx, InternalHandle<Bindings*> self
         return false;
 #endif
 
-    BindingIter bi(*self);
+    BindingIter bi(self);
     unsigned slot = CallObject::RESERVED_SLOTS;
     for (unsigned i = 0, n = self->count(); i < n; i++, bi++) {
         if (!bi->aliased())
@@ -151,7 +151,7 @@ Bindings::switchToScriptStorage(Binding *newBindingArray)
 }
 
 bool
-Bindings::clone(JSContext *cx, InternalHandle<Bindings*> self,
+Bindings::clone(JSContext *cx, InternalBindingsHandle self,
                 uint8_t *dstScriptData,
                 HandleScript srcScript)
 {
@@ -186,13 +186,13 @@ XDRScriptBindings(XDRState<mode> *xdr, LifoAllocScope &las, unsigned numArgs, un
     JSContext *cx = xdr->cx();
 
     if (mode == XDR_ENCODE) {
-        for (BindingIter bi(script->bindings); bi; bi++) {
+        for (BindingIter bi(script); bi; bi++) {
             JSAtom *atom = bi->name();
             if (!XDRAtom(xdr, &atom))
                 return false;
         }
 
-        for (BindingIter bi(script->bindings); bi; bi++) {
+        for (BindingIter bi(script); bi; bi++) {
             uint8_t u8 = (uint8_t(bi->kind()) << 1) | uint8_t(bi->aliased());
             if (!xdr->codeUint8(&u8))
                 return false;
@@ -225,7 +225,7 @@ XDRScriptBindings(XDRState<mode> *xdr, LifoAllocScope &las, unsigned numArgs, un
             bindingArray[i] = Binding(name, kind, aliased);
         }
 
-        InternalHandle<Bindings*> bindings(script, &script->bindings);
+        InternalBindingsHandle bindings(script, &script->bindings);
         if (!Bindings::initWithTemporaryStorage(cx, bindings, numArgs, numVars, bindingArray))
             return false;
     }
@@ -261,8 +261,9 @@ Bindings::trace(JSTracer *trc)
 }
 
 bool
-js::FillBindingVector(Bindings &bindings, BindingVector *vec)
+js::FillBindingVector(HandleScript fromScript, BindingVector *vec)
 {
+    InternalBindingsHandle bindings(fromScript, &fromScript->bindings);
     for (BindingIter bi(bindings); bi; bi++) {
         if (!vec->append(*bi))
             return false;
@@ -2552,7 +2553,8 @@ JSScript::argumentsOptimizationFailed(JSContext *cx, JSScript *script_)
 
     script->needsArgsObj_ = true;
 
-    const unsigned var = script->bindings.argumentsVarIndex(cx);
+    InternalBindingsHandle bindings(script, &script->bindings);
+    const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
 
     /*
      * By design, the apply-arguments optimization is only made when there

js/src/jsscript.h

@@ -126,6 +126,9 @@ class Binding
 
 JS_STATIC_ASSERT(sizeof(Binding) == sizeof(uintptr_t));
 
+class Bindings;
+typedef InternalHandle<Bindings *> InternalBindingsHandle;
+
 /*
  * Formal parameters and local variables are stored in a shape tree
  * path encapsulated within this class.  This class represents bindings for
@@ -168,7 +171,7 @@ class Bindings
      * storage is release, switchToScriptStorage must be called, providing a
      * pointer into the Binding array stored in script->data.
      */
-    static bool initWithTemporaryStorage(JSContext *cx, InternalHandle<Bindings*> self,
+    static bool initWithTemporaryStorage(JSContext *cx, InternalBindingsHandle self,
                                          unsigned numArgs, unsigned numVars,
                                          Binding *bindingArray);
 
@@ -178,7 +181,8 @@ class Bindings
      * Clone srcScript's bindings (as part of js::CloneScript). dstScriptData
      * is the pointer to what will eventually be dstScript->data.
      */
-    static bool clone(JSContext *cx, InternalHandle<Bindings*> self, uint8_t *dstScriptData, HandleScript srcScript);
+    static bool clone(JSContext *cx, InternalBindingsHandle self, uint8_t *dstScriptData,
+                      HandleScript srcScript);
 
     unsigned numArgs() const { return numArgs_; }
     unsigned numVars() const { return numVars_; }
@@ -188,7 +192,7 @@ class Bindings
     Shape *callObjShape() const { return callObjShape_; }
 
     /* Convenience method to get the var index of 'arguments'. */
-    unsigned argumentsVarIndex(JSContext *cx) const;
+    static unsigned argumentsVarIndex(JSContext *cx, InternalBindingsHandle);
 
     /* Return whether the binding at bindingIndex is aliased. */
     bool bindingIsAliased(unsigned bindingIndex);
@@ -900,14 +904,14 @@ namespace js {
  */
 class BindingIter
 {
-    const Bindings *bindings_;
+    const InternalBindingsHandle bindings_;
     unsigned i_;
 
     friend class Bindings;
-    BindingIter(const Bindings &bindings, unsigned i) : bindings_(&bindings), i_(i) {}
 
   public:
-    explicit BindingIter(const Bindings &bindings) : bindings_(&bindings), i_(0) {}
+    explicit BindingIter(const InternalBindingsHandle &bindings) : bindings_(bindings), i_(0) {}
+    explicit BindingIter(const HandleScript &script) : bindings_(script, &script->bindings), i_(0) {}
 
     bool done() const { return i_ == bindings_->count(); }
     operator bool() const { return !done(); }
@@ -931,7 +935,7 @@ class BindingIter
 typedef Vector<Binding, 32> BindingVector;
 
 extern bool
-FillBindingVector(Bindings &bindings, BindingVector *vec);
+FillBindingVector(HandleScript fromScript, BindingVector *vec);
 
 /*
  * Iterator over the aliased formal bindings in ascending index order. This can

js/src/vm/Debugger.cpp

@@ -3741,7 +3741,8 @@ DebuggerObject_getParameterNames(JSContext *cx, unsigned argc, Value *vp)
 
         if (fun->nargs > 0) {
             BindingVector bindings(cx);
-            if (!FillBindingVector(fun->script()->bindings, &bindings))
+            RootedScript script(cx, fun->script());
+            if (!FillBindingVector(script, &bindings))
                 return false;
             for (size_t i = 0; i < fun->nargs; i++) {
                 Value v;

js/src/vm/ScopeObject.cpp

@@ -1124,7 +1124,7 @@ class DebugScopeProxy : public BaseProxyHandler
                 return false;
 
             Bindings &bindings = script->bindings;
-            BindingIter bi(script->bindings);
+            BindingIter bi(script);
             while (bi && NameToId(bi->name()) != id)
                 bi++;
             if (!bi)
@@ -1395,7 +1395,8 @@ class DebugScopeProxy : public BaseProxyHandler
          * they must be manually appended here.
          */
         if (scope.isCall() && !scope.asCall().isForEval()) {
-            for (BindingIter bi(scope.asCall().callee().script()->bindings); bi; bi++) {
+            RootedScript script(cx, scope.asCall().callee().script());
+            for (BindingIter bi(script); bi; bi++) {
                 if (!bi->aliased() && !props.append(NameToId(bi->name())))
                     return false;
             }
@@ -1433,7 +1434,8 @@ class DebugScopeProxy : public BaseProxyHandler
          * a manual search is necessary.
          */
         if (!found && scope.isCall() && !scope.asCall().isForEval()) {
-            for (BindingIter bi(scope.asCall().callee().script()->bindings); bi; bi++) {
+            RootedScript script(cx, scope.asCall().callee().script());
+            for (BindingIter bi(script); bi; bi++) {
                 if (!bi->aliased() && NameToId(bi->name()) == id) {
                     found = true;
                     break;