Bug 140811 – Immediate crash when setting JS breakpoint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

Sometimes when setting a breakpoint in JavaScript code using the inspector you'll get an immediate crash.  The reason for the crash is because the codeBlock says its scopeRegister is at offset 0 (which should never be).   The crash trace from a debug build is below:

(lldb) bt 15
* thread #1: tid = 0xcd1d7, 0x00000001125e121d JavaScriptCore`JSC::StructureIDTable::get(this=0x0000206000000338, structureID=1462580048) + 45 at StructureIDTable.h:85, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x206000000360)
    frame #0: 0x00000001125e121d JavaScriptCore`JSC::StructureIDTable::get(this=0x0000206000000338, structureID=1462580048) + 45 at StructureIDTable.h:85
    frame #1: 0x00000001125e7b76 JavaScriptCore`JSC::JSCell::structure(this=0x00007fff572d3270, vm=0x0000206000000040) const + 54 at JSCellInlines.h:106
    frame #2: 0x00000001125ef4f1 JavaScriptCore`JSC::JSCell::classInfo(this=0x00007fff572d3270) const + 97 at JSCellInlines.h:245
    frame #3: 0x00000001125e3969 JavaScriptCore`JSC::JSCell::inherits(this=0x00007fff572d3270, info=0x0000000113289b20) const + 25 at JSCellInlines.h:221
    frame #4: 0x000000011272a153 JavaScriptCore`JSC::JSScope* JSC::jsCast<JSC::JSScope*>(from=JSValue at 0x00007fff572d2b78) + 67 at JSCell.h:249
    frame #5: 0x000000011272a102 JavaScriptCore`JSC::Register::scope(this=0x00007fff572d3160) const + 34 at JSScope.h:239
    frame #6: 0x000000011272960d JavaScriptCore`JSC::ExecState::scope(this=0x00007fff572d3160, scopeRegisterOffset=0) const + 45 at CallFrame.h:50
  * frame #7: 0x0000000112728cb8 JavaScriptCore`JSC::DebuggerCallFrame::scope(this=0x0000000137e95e60) + 264 at DebuggerCallFrame.cpp:149
    frame #8: 0x0000000112e83737 JavaScriptCore`Inspector::ScriptDebugServer::exceptionOrCaughtValue(this=0x000000011d7f0138, state=0x00000001205df4b0) + 167 at ScriptDebugServer.cpp:340
    frame #9: 0x0000000112e83638 JavaScriptCore`Inspector::ScriptDebugServer::dispatchDidPause(this=0x000000011d7f0138, listener=0x000000011d7f0010) + 328 at ScriptDebugServer.cpp:138
    frame #10: 0x0000000112e83e35 JavaScriptCore`Inspector::ScriptDebugServer::dispatchFunctionToListeners(this=0x000000011d7f0138, listeners=0x000000011d7f0248, callback=0x0000000112e834f0)(Inspector::ScriptDebugListener*)) + 229 at ScriptDebugServer.cpp:279
    frame #11: 0x0000000112e83d3c JavaScriptCore`Inspector::ScriptDebugServer::dispatchFunctionToListeners(this=0x000000011d7f0138, callback=0x0000000112e834f0)(Inspector::ScriptDebugListener*)) + 156 at ScriptDebugServer.cpp:271
    frame #12: 0x0000000112e84129 JavaScriptCore`Inspector::ScriptDebugServer::handlePause(this=0x000000011d7f0138, vmEntryGlobalObject=0x00000001205df470, (null)=PausedForBreakpoint) + 73 at ScriptDebugServer.cpp:312
    frame #13: 0x0000000112714f4c JavaScriptCore`JSC::Debugger::pauseIfNeeded(this=0x000000011d7f0138, callFrame=0x00007fff572d30e0) + 604 at Debugger.cpp:679
    frame #14: 0x00000001127151ec JavaScriptCore`JSC::Debugger::updateCallFrameAndPauseIfNeeded(this=0x000000011d7f0138, callFrame=0x00007fff572d30e0) + 60 at Debugger.cpp:634


[Michael Saboff]

rdar://problem/19544274

[Michael Saboff]

Created attachment 245203 [details]
Patch

[Mark Lam]

Comment on attachment 245203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245203&action=review

I think this solution is fine for ensuring that we don’t get an incorrect local for a non-existant scope register in a DFG frame.  However, this will not entirely satisfy the debugger’s need / concept for a scope.  But that’s a different issue that we can address separately later.

Can you add a regression test please (one that sets the breakpoint in a DFG compiled function that does not have a scope register)?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1666
> +    ASSERT(m_scopeRegister.isLocal() || !m_scopeRegister.isValid());

We will only clone llint or baseline CodeBlocks.  Hence, shouldn’t the scope always be valid and is a local here?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1723
> +    ASSERT(m_scopeRegister.isLocal() || !m_scopeRegister.isValid());

In this case, the scope comes from an UnlinkedCodeBlock.  Should that always be valid and is a local?

[Michael Saboff]

(In reply to comment #3)
> Comment on attachment 245203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245203&action=review
> 
> I think this solution is fine for ensuring that we don’t get an incorrect
> local for a non-existant scope register in a DFG frame.  However, this will
> not entirely satisfy the debugger’s need / concept for a scope.  But that’s
> a different issue that we can address separately later.
> 
> Can you add a regression test please (one that sets the breakpoint in a DFG
> compiled function that does not have a scope register)?

I considered adding a test, but with the ASSERT in setScopeRegister(), we crash in testapi without the fix to DFGStackLayoutPhase.cpp for the exact reason of this bug.  I didn't run any of the JS regression tests, but I expect that multiple tests would crash for the same reason.  Therefore this fix is covered by existing tests.

> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1666
> > +    ASSERT(m_scopeRegister.isLocal() || !m_scopeRegister.isValid());
> 
> We will only clone llint or baseline CodeBlocks.  Hence, shouldn’t the scope
> always be valid and is a local here?

No.  If we could be copying a code block with an invalid scope register, see below.

> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1723
> > +    ASSERT(m_scopeRegister.isLocal() || !m_scopeRegister.isValid());
> 
> In this case, the scope comes from an UnlinkedCodeBlock.  Should that always
> be valid and is a local?

No.  If we don't need a scope register, it will be an invalid virtual register.

[Michael Saboff]

Created attachment 245241 [details]
Patch with updates after talking with mlam.

[Mark Lam]

Comment on attachment 245241 [details]
Patch with updates after talking with mlam.

r=me.  Thanks for the fix.

[Michael Saboff]

Committed r179015: <http://trac.webkit.org/changeset/179015>