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().
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.
... 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.
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().
The crash occurs because js::ScopeCoordi nateName returns nullptr. The only way it can do that is if |id| is zero at http:// hg.mozilla. org/releases/ mozilla- release/ file/7665b8d4d5 1f/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/7665b8d4d5 1f/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/7665b8d4d5 1f/js/src/ vm/ScopeObject. cpp#l72, I verified that no properties with slot=0 ever exist.
I added further asserts in js::frontend: :BytecodeEmitte r::emitScopeCoo rdOp() 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: :BytecodeEmitte r::emitNameOp( ).
Here, at http:// hg.mozilla. org/releases/ mozilla- release/ file/7665b8d4d5 1f/js/src/ frontend/ BytecodeEmitter .cpp#l2272:
if (!pn->pn_ cookie. isFree( )) {
MOZ_ ASSERT( JOF_OPTYPE( op) != JOF_ATOM);
return false;
return false;
if (!emitVarOp(pn, op))
} else {
if (!emitAtomOp(pn, op))
}
... 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: :BytecodeEmitte r::emitAtomOp( ) to a MOZ_RELEASE_ASSERT verifies that we hit this. In gdb:
(gdb) f 1 src/firefox/ build-area/ firefox- 39.0+build5/ js/src/ frontend/ BytecodeEmitter .cpp:1071 pn->pn_ atom, op); u.name. cookie
#1 0xf58099bf in emitAtomOp (op=<optimised out>, pn=0x825bc60, this=0xffffa408) at /home/chr1s/
1071 return emitAtomOp(
(gdb) p pn->pn_
$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: :BytecodeEmitte r::tryConvertFr eeName( ), just here at http:// hg.mozilla. org/releases/ mozilla- release/ file/7665b8d4d5 1f/js/src/ frontend/ BytecodeEmitter .cpp#l1567:
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/7665b8d4d5 1f/js/src/ frontend/ ParseNode. h#l51:
bool set(TokenStream& ts, unsigned newLevel, uint32_t newSlot) { JSMSG_TOO_ DEEP, js_function_str);
if (newLevel >= FREE_LEVEL)
return ts.reportError(
if (newSlot >= SCOPECOORD_ SLOT_LIMIT) JSMSG_TOO_ MANY_LOCALS) ;
return ts.reportError(
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: :BytecodeEmitte r::tryConvertFr eeName( ) in a *good" build, starting at http:// hg.mozilla. org/releases/ mozilla- release/ file/7665b8d4d5 1f/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 :BytecodeEmitte r::tryConvertFr eeName( js::frontend: :ParseNode* )+1833> :BytecodeEmitte r::tryConvertFr eeName( js::frontend: :ParseNode* )+1867> u.name. cookie
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:
0xf4eb97a9 <+1769>: cmp $0xffffff,%eax // Compare |slot| with 0xffffff
// Jump if |slot| > 0xffffff
0xf4eb97ae <+1774>: ja 0xf4eb980b <js::frontend:
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_
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 :BytecodeEmitte r::tryConvertFr eeName( js::frontend: :ParseNode* )+1858> :BytecodeEmitte r::tryConvertFr eeName( js::frontend: :ParseNode* )+1833>
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:
0xf5803bc2 <+1762>: cmp $0xffffff,%eax // Compare |slot| with 0xffffff
// Jump if |slot| > 0xffffff
0xf5803bc7 <+1767>: ja 0xf5803c09 <js::frontend:
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: :BytecodeEmitte r::tryConvertFr eeName - 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( ).