[Bug 1471949] Re: Firefox 39 crashes on startup or within a few seconds on Precise/x86

Fri Jul 10 21:11:38 UTC 2015

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;
    }

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().

-- 
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to firefox in Ubuntu.
https://bugs.launchpad.net/bugs/1471949

Title:
  Firefox 39 crashes on startup or within a few seconds on Precise/x86

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1471949/+subscriptions