desktop-packages team mailing list archive
-
desktop-packages team
-
Mailing list archive
-
Message #127966
[Bug 1471949] Re: Firefox 39 crashes on startup or within a few seconds on Precise/x86
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().
--
You received this bug notification because you are a member of Desktop
Packages, 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
Status in firefox package in Ubuntu:
Invalid
Status in firefox source package in Precise:
Triaged
Bug description:
This is blocking publication of Firefox 39.
The build for x86 on 12.04 currently crashes on startup, or within a
few seconds of startup. It's basically unusable. An example crash
report is: https://crash-stats.mozilla.com/report/index/d0d97dbb-f6bc-
4e4d-88ff-e5fff2150702.
Unfortunately, despite the warning in the PPA description for
https://launchpad.net/~ubuntu-mozilla-security/+archive/ubuntu/ppa,
ordinary users are still installing pre-release packages from it.
It works on all other releases and on Precise/x86-64. I did test
Firefox 39 with this toolchain when it was still the nightly version
whilst preparing the switch to GCC 4.8, and it worked fine.
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1471949/+subscriptions
References
