Comment 3 for bug 1471949

The crash occurs because js::ScopeCoordinateName returns nullptr. The only way it can do that is if |id| is zero at http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/vm/ScopeObject.cpp#l96.

Unfortunately, it works fine in a debug build.

I added a "MOZ_RELEASE_ASSERT(!JSID_IS_ZERO(id))" after http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/vm/ScopeObject.cpp#l89 so I could crash Firefox when this happens and catch it in gdb.

At the point we hit this assert in gdb, |sc| has already been optimised out. However, by examining the incoming bytecode:

(gdb) p *pc
$2 = 136 '\210'
(gdb) x pc+1
0xb68b2e2: 0x00000000

... we see that this is a JSOP_GETALIASEDVAR instruction and the |slot| argument is 0x000000. After adding some printf's to http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/vm/ScopeObject.cpp#l72, I verified that no properties with slot=0 ever exist.

I added further asserts in js::frontend::BytecodeEmitter::emitScopeCoordOp() and verified that this instruction sequence was not being emitted here. After creating a small helper function to scan the bytecode at various stages in BytecodeEmitter, I managed to narrow the point at which this instruction sequence is emitted to js::frontend::BytecodeEmitter::emitNameOp().

Here, at http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/frontend/BytecodeEmitter.cpp#l2272:

        if (!pn->pn_cookie.isFree()) {
             MOZ_ASSERT(JOF_OPTYPE(op) != JOF_ATOM);
             if (!emitVarOp(pn, op))
                 return false;
         } else {
             if (!emitAtomOp(pn, op))
                 return false;
         }

... despite |op| being equal to JSOP_GETALIASEDVAR, this code calls emitAtomOp() because the "if (!pn->pn_cookie.isFree()) {" check evaluates false. This is wrong - it should be calling emitVarOp().

Changing the MOZ_ASSERT at the top of js::frontend::BytecodeEmitter::emitAtomOp() to a MOZ_RELEASE_ASSERT verifies that we hit this. In gdb:

(gdb) f 1
#1 0xf58099bf in emitAtomOp (op=<optimised out>, pn=0x825bc60, this=0xffffa408) at /home/chr1s/src/firefox/build-area/firefox-39.0+build5/js/src/frontend/BytecodeEmitter.cpp:1071
1071 return emitAtomOp(pn->pn_atom, op);
(gdb) p pn->pn_u.name.cookie
$2 = {level_ = 255, slot_ = 4, static FREE_LEVEL = 255}

So, the correct branch is taken. Which means that |pn| is invalid.

In fact, it goes wrong in js::frontend::BytecodeEmitter::tryConvertFreeName(), just here at http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/frontend/BytecodeEmitter.cpp#l1567:

                    JSOp op;
                    switch (pn->getOp()) {
                      case JSOP_GETNAME: op = JSOP_GETALIASEDVAR; break;
                      case JSOP_SETNAME: op = JSOP_SETALIASEDVAR; break;
                      default: return false;
                    }

                    pn->setOp(op);
                    JS_ALWAYS_TRUE(pn->pn_cookie.set(parser->tokenStream, hops, slot));
                    return true;

If I add a "MOZ_RELEASE_ASSERT(!pn->pn_cookie.isFree());" just before the "return true", then we hit this in our broken build. The line above calls js::frontend::UpvarCookie::set(), which is inlined from http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/frontend/ParseNode.h#l51:

    bool set(TokenStream& ts, unsigned newLevel, uint32_t newSlot) {
        if (newLevel >= FREE_LEVEL)
            return ts.reportError(JSMSG_TOO_DEEP, js_function_str);

        if (newSlot >= SCOPECOORD_SLOT_LIMIT)
            return ts.reportError(JSMSG_TOO_MANY_LOCALS);

        level_ = newLevel;
        slot_ = newSlot;
        return true;
    }

... which seems simple enough. Note that |level_| and |slot_| both fit in to 32-bits, with |slot_| taking the least-significant byte and |level_| taking the other 3 bytes.

If I disassemble this bit of code in js::frontend::BytecodeEmitter::tryConvertFreeName() in a *good" build, starting at http://hg.mozilla.org/releases/mozilla-release/file/7665b8d4d51f/js/src/frontend/BytecodeEmitter.cpp#l1572:

The starting context looks like this:
%eax = |op|
0x1c(%esp) = |hops|
0x24(%esp) = |slot|
0x74(%esp) = |pn|

   0xf4eb978b <+1739>: mov 0xa0(%ebp),%ecx
   0xf4eb9791 <+1745>: mov 0x74(%esp),%edi // %edi now points to |pn|
   0xf4eb9795 <+1749>: add $0x18,%ecx
   0xf4eb9798 <+1752>: cmpl $0xfe,0x1c(%esp) // Compare |hops| with 254 (FREE_LEVEL - 1)
   0xf4eb97a0 <+1760>: mov %al,0x2(%edi) // Calls pn->SetOp(op)
   0xf4eb97a3 <+1763>: mov 0x24(%esp),%eax // %eax now contains |slot|
// Jump if |hops| > 254
   0xf4eb97a7 <+1767>: ja 0xf4eb97e9 <js::frontend::BytecodeEmitter::tryConvertFreeName(js::frontend::ParseNode*)+1833>
   0xf4eb97a9 <+1769>: cmp $0xffffff,%eax // Compare |slot| with 0xffffff
// Jump if |slot| > 0xffffff
   0xf4eb97ae <+1774>: ja 0xf4eb980b <js::frontend::BytecodeEmitter::tryConvertFreeName(js::frontend::ParseNode*)+1867>
   0xf4eb97b0 <+1776>: movzbl 0x1c(%esp),%ecx // %ecx now contains |hops|
   0xf4eb97b5 <+1781>: mov 0x74(%esp),%edi // %edi now points to |pn|
   0xf4eb97b9 <+1785>: shl $0x8,%eax // Left shift new |slot| value by 8-bits
   0xf4eb97bc <+1788>: or %ecx,%eax // %eax now contains the bitwise-OR of |hops| and |slot|
   0xf4eb97be <+1790>: mov %eax,0x20(%edi) // Save the new values to |level_| and |slot_| in pn->pn_u.name.cookie

Now, looking at a broken build:

The starting context looks like this:
%eax = |op|
0x38(%esp) = |hops|
0x44(%esp) = |slot|
0x94(%esp) = |pn|

   0xf5803ba1 <+1729>: mov 0xa0(%ebp),%edx
   0xf5803ba7 <+1735>: mov 0x94(%esp),%esi // %esi now points to |pn|
   0xf5803bae <+1742>: add $0x18,%edx
   0xf5803bb1 <+1745>: cmpl $0xfe,0x38(%esp) // Compare |hops| with 254 (FREE_LEVEL - 1)
   0xf5803bb9 <+1753>: mov %al,0x2(%esi) // Calls pn->SetOp(op)
   0xf5803bbc <+1756>: mov 0x44(%esp),%eax // %eax now contains |slot|
// Jump if |hops| > 254
   0xf5803bc0 <+1760>: ja 0xf5803c22 <js::frontend::BytecodeEmitter::tryConvertFreeName(js::frontend::ParseNode*)+1858>
   0xf5803bc2 <+1762>: cmp $0xffffff,%eax // Compare |slot| with 0xffffff
// Jump if |slot| > 0xffffff
   0xf5803bc7 <+1767>: ja 0xf5803c09 <js::frontend::BytecodeEmitter::tryConvertFreeName(js::frontend::ParseNode*)+1833>
   0xf5803bc9 <+1769>: mov 0x94(%esp),%esi // %esi now points to |pn|
   0xf5803bd0 <+1776>: shl $0x8,%eax // Left shift new |slot| value by 8-bits
   0xf5803bd3 <+1779>: mov %eax,%edx // ...and copy it in to %edx

// Now, this is where it goes wrong. The next instruction loads the current |level_| from pn->pn_u.name.cookie (which is 255) in to %eax
   0xf5803bd5 <+1781>: movzbl 0x20(%esi),%eax
   0xf5803bd9 <+1785>: or %edx,%eax // %eax now contains the bitwise-OR of the old |level_| and new |slot| value
   0xf5803bdb <+1787>: mov %eax,0x20(%esi) // Save the new |slot_|

So, this appears to be a miscompile in js::frontend::BytecodeEmitter::tryConvertFreeName - the inlined call to js::frontend::UpvarCookie::set() correctly sets |slot_| but always preserves the previous value of |level_|, causing the call to js::frontend::UpvarCookie::isFree() to return the wrong value in BytecodeEmitter::emitNameOp().