Bug 1100623 - Fix expression decompiler reporting the wrong local, get rid of FillBindingVector. r=luke
authorJan de Mooij <jdemooij@mozilla.com>
Thu, 20 Nov 2014 11:21:50 +0100
changeset 216626 beb6d3e078d0eb1802922431af4e63e0fc85d0b5
parent 216625 03c6a758c9e8da0dfcad48788a8eabb6f9d899d2
child 216627 d90b4907a287547c46e18a2c10b1a5d24c142fb2
push idunknown
push userunknown
push dateunknown
reviewersluke
bugs1100623
milestone36.0a1
Bug 1100623 - Fix expression decompiler reporting the wrong local, get rid of FillBindingVector. r=luke
js/src/jit-test/tests/basic/bug1100623.js
js/src/jsfriendapi.cpp
js/src/jsfun.cpp
js/src/jsopcode.cpp
js/src/jsscript.cpp
js/src/jsscript.h
js/src/vm/Debugger.cpp
js/src/vm/ForkJoin.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug1100623.js
@@ -0,0 +1,12 @@
+// |jit-test| error: baz is null
+var document = {getElementById: () => null};
+
+(function() {
+    const one = 1;
+
+    function foo() { return one; }
+    function bar() { return foo(); }
+
+    var baz = document.getElementById("baz");
+    baz.value;
+})();
--- a/js/src/jsfriendapi.cpp
+++ b/js/src/jsfriendapi.cpp
@@ -784,23 +784,17 @@ FormatFrame(JSContext *cx, const ScriptF
         buf = JS_sprintf_append(buf, "%d anonymous(", num);
     } else {
         buf = JS_sprintf_append(buf, "%d <TOP LEVEL>", num);
     }
     if (!buf)
         return buf;
 
     if (showArgs && iter.hasArgs()) {
-        BindingVector bindings(cx);
-        if (fun && fun->isInterpreted()) {
-            if (!FillBindingVector(script, &bindings))
-                return buf;
-        }
-
-
+        BindingIter bi(script);
         bool first = true;
         for (unsigned i = 0; i < iter.numActualArgs(); i++) {
             RootedValue arg(cx);
             if (i < iter.numFormalArgs() && script->formalIsAliased(i)) {
                 for (AliasedFormalIter fi(script); ; fi++) {
                     if (fi.frameIndex() == i) {
                         arg = iter.callObj().aliasedVar(fi);
                         break;
@@ -813,20 +807,22 @@ FormatFrame(JSContext *cx, const ScriptF
             }
 
             JSAutoByteString valueBytes;
             const char *value = FormatValue(cx, arg, valueBytes);
 
             JSAutoByteString nameBytes;
             const char *name = nullptr;
 
-            if (i < bindings.length()) {
-                name = nameBytes.encodeLatin1(cx, bindings[i].name());
+            if (i < iter.numFormalArgs()) {
+                MOZ_ASSERT(i == bi.argIndex());
+                name = nameBytes.encodeLatin1(cx, bi->name());
                 if (!buf)
                     return nullptr;
+                bi++;
             }
 
             if (value) {
                 buf = JS_sprintf_append(buf, "%s%s%s%s%s%s",
                                         !first ? ", " : "",
                                         name ? name :"",
                                         name ? " = " : "",
                                         arg.isString() ? "\"" : "",
--- a/js/src/jsfun.cpp
+++ b/js/src/jsfun.cpp
@@ -1064,26 +1064,27 @@ js::FunctionToString(JSContext *cx, Hand
             // This function was created with the Function constructor. We don't
             // have source for the arguments, so we have to generate that. Part
             // of bug 755821 should be cobbling the arguments passed into the
             // Function constructor into the source string.
             if (!out.append("("))
                 return nullptr;
 
             // Fish out the argument names.
-            BindingVector *localNames = cx->new_<BindingVector>(cx);
-            ScopedJSDeletePtr<BindingVector> freeNames(localNames);
-            if (!FillBindingVector(script, localNames))
-                return nullptr;
-            for (unsigned i = 0; i < fun->nargs(); i++) {
-                if ((i && !out.append(", ")) ||
-                    (i == unsigned(fun->nargs() - 1) && fun->hasRest() && !out.append("...")) ||
-                    !out.append((*localNames)[i].name())) {
+            MOZ_ASSERT(script->bindings.numArgs() == fun->nargs());
+
+            BindingIter bi(script);
+            for (unsigned i = 0; i < fun->nargs(); i++, bi++) {
+                MOZ_ASSERT(bi.argIndex() == i);
+                if (i && !out.append(", "))
                     return nullptr;
-                }
+                if (i == unsigned(fun->nargs() - 1) && fun->hasRest() && !out.append("..."))
+                    return nullptr;
+                if (!out.append(bi->name()))
+                    return nullptr;
             }
             if (!out.append(") {\n"))
                 return nullptr;
         }
         if ((bodyOnly && !funCon) || addUseStrict) {
             // We need to get at the body either because we're only supposed to
             // return the body or we need to insert "use strict" into the body.
             size_t bodyStart = 0, bodyEnd;
--- a/js/src/jsopcode.cpp
+++ b/js/src/jsopcode.cpp
@@ -1461,32 +1461,28 @@ namespace {
  * 5". If we need to decompile a subexpression, we call decompilePC (step 2)
  * recursively on the operands' pcs. The result is a depth-first traversal of
  * the expression tree.
  *
  */
 struct ExpressionDecompiler
 {
     JSContext *cx;
-    InterpreterFrame *fp;
     RootedScript script;
     RootedFunction fun;
-    BindingVector *localNames;
     BytecodeParser parser;
     Sprinter sprinter;
 
     ExpressionDecompiler(JSContext *cx, JSScript *script, JSFunction *fun)
         : cx(cx),
           script(cx, script),
           fun(cx, fun),
-          localNames(nullptr),
           parser(cx, script),
           sprinter(cx)
     {}
-    ~ExpressionDecompiler();
     bool init();
     bool decompilePCForStackOperand(jsbytecode *pc, int i);
     bool decompilePC(jsbytecode *pc);
     JSAtom *getLocal(uint32_t local, jsbytecode *pc);
     JSAtom *getArg(unsigned slot);
     JSAtom *loadAtom(jsbytecode *pc);
     bool quote(JSString *s, uint32_t quote);
     bool write(const char *s);
@@ -1626,36 +1622,24 @@ ExpressionDecompiler::decompilePC(jsbyte
         return write(str);
       }
       default:
         break;
     }
     return write("(intermediate value)");
 }
 
-ExpressionDecompiler::~ExpressionDecompiler()
-{
-    js_delete<BindingVector>(localNames);
-}
-
 bool
 ExpressionDecompiler::init()
 {
     assertSameCompartment(cx, script);
 
     if (!sprinter.init())
         return false;
 
-    localNames = cx->new_<BindingVector>(cx);
-    if (!localNames)
-        return false;
-    RootedScript script_(cx, script);
-    if (!FillBindingVector(script_, localNames))
-        return false;
-
     if (!parser.parse())
         return false;
 
     return true;
 }
 
 bool
 ExpressionDecompiler::write(const char *s)
@@ -1680,33 +1664,43 @@ ExpressionDecompiler::loadAtom(jsbytecod
 {
     return script->getAtom(GET_UINT32_INDEX(pc));
 }
 
 JSAtom *
 ExpressionDecompiler::getArg(unsigned slot)
 {
     MOZ_ASSERT(fun);
-    MOZ_ASSERT(slot < script->bindings.count());
-    return (*localNames)[slot].name();
+    MOZ_ASSERT(slot < script->bindings.numArgs());
+
+    for (BindingIter bi(script); bi; bi++) {
+        MOZ_ASSERT(bi->kind() == Binding::ARGUMENT);
+        if (bi.argIndex() == slot)
+            return bi->name();
+    }
+
+    MOZ_CRASH("No binding");
 }
 
 JSAtom *
 ExpressionDecompiler::getLocal(uint32_t local, jsbytecode *pc)
 {
     MOZ_ASSERT(local < script->nfixed());
     if (local < script->nbodyfixed()) {
-        MOZ_ASSERT(fun);
-        uint32_t slot = local + fun->nargs();
-        MOZ_ASSERT(slot < script->bindings.count());
-        return (*localNames)[slot].name();
+        for (BindingIter bi(script); bi; bi++) {
+            if (bi->kind() != Binding::ARGUMENT && !bi->aliased() && bi.frameIndex() == local)
+                return bi->name();
+        }
+
+        MOZ_CRASH("No binding");
     }
     for (NestedScopeObject *chain = script->getStaticScope(pc);
          chain;
-         chain = chain->enclosingNestedScope()) {
+         chain = chain->enclosingNestedScope())
+    {
         if (!chain->is<StaticBlockObject>())
             continue;
         StaticBlockObject &block = chain->as<StaticBlockObject>();
         if (local < block.localOffset())
             continue;
         local -= block.localOffset();
         if (local >= block.numVariables())
             return nullptr;
--- a/js/src/jsscript.cpp
+++ b/js/src/jsscript.cpp
@@ -329,27 +329,16 @@ Bindings::trace(JSTracer *trc)
         return;
 
     for (Binding *b = bindingArray(), *end = b + count(); b != end; b++) {
         PropertyName *name = b->name();
         MarkStringUnbarriered(trc, &name, "bindingArray");
     }
 }
 
-bool
-js::FillBindingVector(HandleScript fromScript, BindingVector *vec)
-{
-    for (BindingIter bi(fromScript); bi; bi++) {
-        if (!vec->append(*bi))
-            return false;
-    }
-
-    return true;
-}
-
 template<XDRMode mode>
 bool
 js::XDRScriptConst(XDRState<mode> *xdr, MutableHandleValue vp)
 {
     JSContext *cx = xdr->cx();
 
     /*
      * A script constant can be an arbitrary primitive value as they are used
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -1749,26 +1749,31 @@ class BindingIter
     void operator++(int) {
         MOZ_ASSERT(!done());
         const Binding &binding = **this;
         if (binding.kind() != Binding::ARGUMENT && !binding.aliased())
             unaliasedLocal_++;
         i_++;
     }
 
-    // Stack slots are assigned to arguments and unaliased locals. frameIndex()
-    // returns the slot index. It's invalid to call this method when the
-    // iterator is stopped on an aliased local, as it has no stack slot.
+    // Stack slots are assigned to arguments (aliased and unaliased) and
+    // unaliased locals. frameIndex() returns the slot index. It's invalid to
+    // call this method when the iterator is stopped on an aliased local, as it
+    // has no stack slot.
     uint32_t frameIndex() const {
         MOZ_ASSERT(!done());
         if (i_ < bindings_->numArgs())
             return i_;
         MOZ_ASSERT(!(*this)->aliased());
         return unaliasedLocal_;
     }
+
+    // If the current binding is an argument, argIndex() returns its index.
+    // It returns the same value as frameIndex(), as slots are allocated for
+    // both unaliased and aliased arguments.
     uint32_t argIndex() const {
         MOZ_ASSERT(!done());
         MOZ_ASSERT(i_ < bindings_->numArgs());
         return i_;
     }
     uint32_t argOrLocalIndex() const {
         MOZ_ASSERT(!done());
         return i_ < bindings_->numArgs() ? i_ : i_ - bindings_->numArgs();
@@ -1784,26 +1789,16 @@ class BindingIter
         return binding.kind() != Binding::ARGUMENT;
     }
 
     const Binding &operator*() const { MOZ_ASSERT(!done()); return bindings_->bindingArray()[i_]; }
     const Binding *operator->() const { MOZ_ASSERT(!done()); return &bindings_->bindingArray()[i_]; }
 };
 
 /*
- * This helper function fills the given BindingVector with the sequential
- * values of BindingIter.
- */
-
-typedef Vector<Binding, 32> BindingVector;
-
-extern bool
-FillBindingVector(HandleScript fromScript, BindingVector *vec);
-
-/*
  * Iterator over the aliased formal bindings in ascending index order. This can
  * be veiwed as a filtering of BindingIter with predicate
  *   bi->aliased() && bi->kind() == Binding::ARGUMENT
  */
 class AliasedFormalIter
 {
     const Binding *begin_, *p_, *end_;
     unsigned slot_;
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -6030,25 +6030,24 @@ DebuggerObject_getParameterNames(JSConte
     if (fun->isInterpreted()) {
         RootedScript script(cx, GetOrCreateFunctionScript(cx, fun));
         if (!script)
             return false;
 
         MOZ_ASSERT(fun->nargs() == script->bindings.numArgs());
 
         if (fun->nargs() > 0) {
-            BindingVector bindings(cx);
-            if (!FillBindingVector(script, &bindings))
-                return false;
-            for (size_t i = 0; i < fun->nargs(); i++) {
+            BindingIter bi(script);
+            for (size_t i = 0; i < fun->nargs(); i++, bi++) {
+                MOZ_ASSERT(bi.argIndex() == i);
                 Value v;
-                if (bindings[i].name()->length() == 0)
+                if (bi->name()->length() == 0)
                     v = UndefinedValue();
                 else
-                    v = StringValue(bindings[i].name());
+                    v = StringValue(bi->name());
                 result->setDenseElement(i, v);
             }
         }
     } else {
         for (size_t i = 0; i < fun->nargs(); i++)
             result->setDenseElement(i, UndefinedValue());
     }
 
--- a/js/src/vm/ForkJoin.cpp
+++ b/js/src/vm/ForkJoin.cpp
@@ -1102,49 +1102,45 @@ ForkJoinOperation::reportBailoutWarnings
                         sp.printf("<anonymous>");
                     }
                 } else {
                     sp.printf("<global>");
                 }
                 sp.printf(" (%s:%u)", script->filename(), PCToLineNumber(script, frame->pc()));
 
                 // Format bindings.
-                BindingVector bindings(cx_);
-                if (!FillBindingVector(script, &bindings))
-                    return false;
-
                 unsigned scopeSlot = 0;
-                for (unsigned i = 0; i < bindings.length(); i++) {
+                for (BindingIter bi(script); bi; bi++) {
                     JSAutoByteString nameBytes;
                     const char *nameChars = nullptr;
-                    RootedPropertyName bindingName(cx_, bindings[i].name());
+                    RootedPropertyName bindingName(cx_, bi->name());
                     nameChars = nameBytes.encodeUtf8(cx_, bindingName);
                     if (!nameChars)
                         return false;
 
                     RootedValue arg(cx_);
-                    if (bindings[i].aliased()) {
+                    if (bi->aliased()) {
                         arg = frame->callObj().getSlot(scopeSlot);
                         scopeSlot++;
-                    } else if (i < frame->numFormalArgs()) {
+                    } else if (bi->kind() == Binding::ARGUMENT) {
                         if (script->argsObjAliasesFormals() && frame->hasArgsObj())
-                            arg = frame->argsObj().arg(i);
+                            arg = frame->argsObj().arg(bi.argIndex());
                         else
-                            arg = frame->unaliasedActual(i, DONT_CHECK_ALIASING);
+                            arg = frame->unaliasedActual(bi.argIndex(), DONT_CHECK_ALIASING);
                     } else {
-                        arg = frame->unaliasedLocal(i - frame->numFormalArgs());
+                        arg = frame->unaliasedLocal(bi.frameIndex());
                     }
 
                     JSAutoByteString valueBytes;
                     const char *valueChars = ValueToChar(cx_, arg, valueBytes);
                     if (!valueChars)
                         return false;
 
                     sp.printf("\n      %s %s = %s",
-                              bindings[i].kind() == Binding::ARGUMENT ? "arg" : "var",
+                              bi->kind() == Binding::ARGUMENT ? "arg" : "var",
                               nameChars, valueChars);
                 }
             }
         }
     }
 
     if (SpewEnabled(SpewBailouts))
         JS_ReportWarning(cx_, "%s", sp.string());