Bug 783315 - Don't touch script->bindings.bindingArray if the script isn't fully compiled (r=billm)

--HG--
extra : rebase_source : fb4e163eaf9f3f7d84ad632d5f940b9d343c8ffc

js/src/frontend/BytecodeCompiler.cpp

@@ -102,7 +102,7 @@ frontend::CompileScript(JSContext *cx, HandleObject scopeChain, StackFrame *call
 
     // Global/eval script bindings are always empty (all names are added to the
     // scope dynamically via JSOP_DEFFUN/VAR).
-    if (!script->bindings.init(cx, 0, 0, NULL))
+    if (!script->bindings.initWithTemporaryStorage(cx, 0, 0, NULL))
         return NULL;
 
     // We can specialize a bit for the given scope chain if that scope chain is the global object.

js/src/frontend/TreeContext.cpp

@@ -223,7 +223,7 @@ TreeContext::generateFunctionBindings(JSContext *cx, Bindings *bindings) const
     AppendPackedBindings(this, args_, packedBindings);
     AppendPackedBindings(this, vars_, packedBindings + args_.length());
 
-    if (!bindings->init(cx, args_.length(), vars_.length(), packedBindings))
+    if (!bindings->initWithTemporaryStorage(cx, args_.length(), vars_.length(), packedBindings))
         return false;
 
     if (bindings->hasAnyAliasedBindings() || sc->funHasExtensibleScope())

js/src/jsscript.cpp

@@ -62,10 +62,10 @@ Bindings::argumentsVarIndex(JSContext *cx) const
 }
 
 bool
-Bindings::init(JSContext *cx, unsigned numArgs, unsigned numVars, Binding *bindingArray)
+Bindings::initWithTemporaryStorage(JSContext *cx, unsigned numArgs, unsigned numVars, Binding *bindingArray)
 {
     JS_ASSERT(!callObjShape_);
-    JS_ASSERT(!bindingArray_);
+    JS_ASSERT(bindingArrayAndFlag_ == TEMPORARY_STORAGE_BIT);
 
     if (numArgs > UINT16_MAX || numVars > UINT16_MAX) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
@@ -75,11 +75,11 @@ Bindings::init(JSContext *cx, unsigned numArgs, unsigned numVars, Binding *bindi
         return false;
     }
 
-    bindingArray_ = bindingArray;
+    JS_ASSERT(!(uintptr_t(bindingArray) & TEMPORARY_STORAGE_BIT));
+    bindingArrayAndFlag_ = uintptr_t(bindingArray) | TEMPORARY_STORAGE_BIT;
     numArgs_ = numArgs;
     numVars_ = numVars;
 
-
     /*
      * Get the initial shape to use when creating CallObjects for this script.
      * Since unaliased variables are, by definition, only accessed by local
@@ -135,10 +135,13 @@ Bindings::init(JSContext *cx, unsigned numArgs, unsigned numVars, Binding *bindi
 }
 
 uint8_t *
-Bindings::switchStorageTo(Binding *newBindingArray)
+Bindings::switchToScriptStorage(Binding *newBindingArray)
 {
-    PodCopy(newBindingArray, bindingArray_, count());
+    JS_ASSERT(bindingArrayUsingTemporaryStorage());
-    bindingArray_ = newBindingArray;
+    JS_ASSERT(!(uintptr_t(newBindingArray) & TEMPORARY_STORAGE_BIT));
+
+    PodCopy(newBindingArray, bindingArray(), count());
+    bindingArrayAndFlag_ = uintptr_t(newBindingArray);
     return reinterpret_cast<uint8_t *>(newBindingArray + count());
 }
 
@@ -147,7 +150,7 @@ Bindings::clone(JSContext *cx, uint8_t *dstScriptData, HandleScript srcScript)
 {
     /* The clone has the same bindingArray_ offset as 'src'. */
     Bindings &src = srcScript->bindings;
-    ptrdiff_t off = (uint8_t *)src.bindingArray_ - srcScript->data;
+    ptrdiff_t off = (uint8_t *)src.bindingArray() - srcScript->data;
     JS_ASSERT(off >= 0);
     JS_ASSERT(off <= (srcScript->code - srcScript->data));
     Binding *dstPackedBindings = (Binding *)(dstScriptData + off);
@@ -156,8 +159,10 @@ Bindings::clone(JSContext *cx, uint8_t *dstScriptData, HandleScript srcScript)
      * Since atoms are shareable throughout the runtime, we can simply copy
      * the source's bindingArray directly.
      */
-    PodCopy(dstPackedBindings, src.bindingArray_, src.count());
+    if (!initWithTemporaryStorage(cx, src.numArgs(), src.numVars(), src.bindingArray()))
-    return init(cx, src.numArgs(), src.numVars(), dstPackedBindings);
+        return false;
+    switchToScriptStorage(dstPackedBindings);
+    return true;
 }
 
 template<XDRMode mode>
@@ -207,7 +212,7 @@ XDRScriptBindings(XDRState<mode> *xdr, LifoAllocScope &las, unsigned numArgs, un
             bindingArray[i] = Binding(name, kind, aliased);
         }
 
-        if (!script->bindings.init(cx, numArgs, numVars, bindingArray))
+        if (!script->bindings.initWithTemporaryStorage(cx, numArgs, numVars, bindingArray))
             return false;
     }
 
@@ -218,7 +223,7 @@ bool
 Bindings::bindingIsAliased(unsigned bindingIndex)
 {
     JS_ASSERT(bindingIndex < count());
-    return bindingArray_[bindingIndex].aliased();
+    return bindingArray()[bindingIndex].aliased();
 }
 
 void
@@ -227,7 +232,15 @@ Bindings::trace(JSTracer *trc)
     if (callObjShape_)
         MarkShape(trc, &callObjShape_, "callObjShape");
 
-    for (Binding *b = bindingArray_, *end = b + count(); b != end; b++) {
+    /*
+     * As the comment in Bindings explains, bindingsArray may point into freed
+     * storage when bindingArrayUsingTemporaryStorage so we don't mark it.
+     * Note: during compilation, atoms are already kept alive by gcKeepAtoms.
+     */
+    if (bindingArrayUsingTemporaryStorage())
+        return;
+
+    for (Binding *b = bindingArray(), *end = b + count(); b != end; b++) {
         PropertyName *name = b->name();
         MarkStringUnbarriered(trc, &name, "bindingArray");
     }
@@ -1573,7 +1586,7 @@ JSScript::partiallyInit(JSContext *cx, Handle<JSScript*> script,
         cursor += vectorSize;
     }
 
-    cursor = script->bindings.switchStorageTo(reinterpret_cast<Binding *>(cursor));
+    cursor = script->bindings.switchToScriptStorage(reinterpret_cast<Binding *>(cursor));
 
     script->code = (jsbytecode *)cursor;
     JS_ASSERT(cursor + length * sizeof(jsbytecode) + nsrcnotes * sizeof(jssrcnote) == script->data + size);

js/src/jsscript.h

@@ -87,20 +87,20 @@ class Binding
      * One JSScript stores one Binding per formal/variable so we use a
      * packed-word representation.
      */
-    size_t bits_;
+    uintptr_t bits_;
 
-    static const size_t KIND_MASK = 0x3;
+    static const uintptr_t KIND_MASK = 0x3;
-    static const size_t ALIASED_BIT = 0x4;
+    static const uintptr_t ALIASED_BIT = 0x4;
-    static const size_t NAME_MASK = ~(KIND_MASK | ALIASED_BIT);
+    static const uintptr_t NAME_MASK = ~(KIND_MASK | ALIASED_BIT);
 
   public:
     explicit Binding() : bits_(0) {}
 
     Binding(PropertyName *name, BindingKind kind, bool aliased) {
         JS_STATIC_ASSERT(CONSTANT <= KIND_MASK);
-        JS_ASSERT((size_t(name) & ~NAME_MASK) == 0);
+        JS_ASSERT((uintptr_t(name) & ~NAME_MASK) == 0);
-        JS_ASSERT((size_t(kind) & ~KIND_MASK) == 0);
+        JS_ASSERT((uintptr_t(kind) & ~KIND_MASK) == 0);
-        bits_ = size_t(name) | size_t(kind) | (aliased ? ALIASED_BIT : 0);
+        bits_ = uintptr_t(name) | uintptr_t(kind) | (aliased ? ALIASED_BIT : 0);
     }
 
     PropertyName *name() const {
@@ -116,7 +116,7 @@ class Binding
     }
 };
 
-JS_STATIC_ASSERT(sizeof(Binding) == sizeof(size_t));
+JS_STATIC_ASSERT(sizeof(Binding) == sizeof(uintptr_t));
 
 /*
  * Formal parameters and local variables are stored in a shape tree
@@ -130,20 +130,38 @@ class Bindings
     friend class AliasedFormalIter;
 
     HeapPtr<Shape> callObjShape_;
-    Binding *bindingArray_;
+    uintptr_t bindingArrayAndFlag_;
     uint16_t numArgs_;
     uint16_t numVars_;
 
+    /*
+     * During parsing, bindings are allocated out of a temporary LifoAlloc.
+     * After parsing, a JSScript object is created and the bindings are
+     * permanently transferred to it. On error paths, the JSScript object may
+     * end up with bindings that still point to the (new released) LifoAlloc
+     * memory. To avoid tracing these bindings during GC, we keep track of
+     * whether the bindings are temporary or permanent in the low bit of
+     * bindingArrayAndFlag_.
+     */
+    static const uintptr_t TEMPORARY_STORAGE_BIT = 0x1;
+    bool bindingArrayUsingTemporaryStorage() const {
+        return bindingArrayAndFlag_ & TEMPORARY_STORAGE_BIT;
+    }
+    Binding *bindingArray() const {
+        return reinterpret_cast<Binding *>(bindingArrayAndFlag_ & ~TEMPORARY_STORAGE_BIT);
+    }
+
   public:
     inline Bindings();
 
     /*
-     * Initialize a Bindings. bindingArray must have length numArgs+numVars and
+     * Initialize a Bindings with a pointer into temporary storage.
-     * must outlive Bindings. To use a temporary bindingArray, the caller may
+     * bindingArray must have length numArgs+numVars. Before the temporary
-     * call switchStorageTo, providing new storage for Bindings to use.
+     * storage is release, switchToScriptStorage must be called, providing a
+     * pointer into the Binding array stored in script->data.
      */
-    bool init(JSContext *cx, unsigned numArgs, unsigned numVars, Binding *bindingArray);
+    bool initWithTemporaryStorage(JSContext *cx, unsigned numArgs, unsigned numVars, Binding *bindingArray);
-    uint8_t *switchStorageTo(Binding *newStorage);
+    uint8_t *switchToScriptStorage(Binding *newStorage);
 
     /*
      * Clone srcScript's bindings (as part of js::CloneScript). dstScriptData
@@ -858,8 +876,8 @@ class BindingIter
         return i_ < bindings_->numArgs() ? i_ : i_ - bindings_->numArgs();
     }
 
-    const Binding &operator*() const { JS_ASSERT(!done()); return bindings_->bindingArray_[i_]; }
+    const Binding &operator*() const { JS_ASSERT(!done()); return bindings_->bindingArray()[i_]; }
-    const Binding *operator->() const { JS_ASSERT(!done()); return &bindings_->bindingArray_[i_]; }
+    const Binding *operator->() const { JS_ASSERT(!done()); return &bindings_->bindingArray()[i_]; }
 };
 
 /*

js/src/jsscriptinlines.h

@@ -24,7 +24,7 @@ namespace js {
 
 inline
 Bindings::Bindings()
-    : callObjShape_(NULL), bindingArray_(NULL), numArgs_(0), numVars_(0)
+    : callObjShape_(NULL), bindingArrayAndFlag_(TEMPORARY_STORAGE_BIT), numArgs_(0), numVars_(0)
 {}
 
 bool
@@ -35,7 +35,7 @@ Bindings::extensibleParents()
 
 inline
 AliasedFormalIter::AliasedFormalIter(JSScript *script)
-  : begin_(script->bindings.bindingArray_),
+  : begin_(script->bindings.bindingArray()),
     p_(begin_),
     end_(begin_ + (script->funHasAnyAliasedFormal ? script->bindings.numArgs() : 0)),
     slot_(CallObject::RESERVED_SLOTS)