Bug 121108 – Web Inspector gets paused twice when there is an exception in host function

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Joseph Pecoraro]

Created attachment 211217 [details]
[TEST] Manual Test Case

See attached test case. The Inspector gets paused twice, as a result of two JSC::Debugger::exception calls. Once as expected at the throw statement, then once again, unexpectedly at the Array.prototype.map call site.

* STEPS TO REPRODUCE
1. Inspect attached [TEST]
2. Open Debugger Navigation Sidebar
3. Enable Breakpoint on "All Exceptions"
4. Click <button> on the page => inspector will pause
5. Click Continue in Web Inspector's Debugger sidebar
  => unexpectedly pause again

* NOTES
(lldb) bt
* thread #1: tid = 0x96660e, 0x000000010f3bb3f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007ffdabc43e80, debuggerCallFrame=0x00007fff547edb38, sourceID=1, lineNumber=27, columnNumber=0, hasHandler=true) + 39 at ScriptDebugServer.cpp:529, queue = 'com.apple.main-thread, stop reason = breakpoint 1.1
    frame #0: 0x000000010f3bb3f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007ffdabc43e80, debuggerCallFrame=0x00007fff547edb38, sourceID=1, lineNumber=27, columnNumber=0, hasHandler=true) + 39 at ScriptDebugServer.cpp:529
    frame #1: 0x000000010cdec89d JavaScriptCore`JSC::Interpreter::unwind(this=0x00007ffdadb09ab0, callFrame=0x00007fff547edc10, exceptionValue=0x00007fff547edc08, bytecodeOffset=27) + 909 at Interpreter.cpp:668
    frame #2: 0x000000010ce0d4ae JavaScriptCore`JSC::genericUnwind(vm=0x00007ffdae005e00, callFrame=0x0000000115ea5190, exceptionValue=JSValue at 0x00007fff547edc08, vPCIndex=27) + 110 at JITExceptions.cpp:73
    frame #3: 0x000000010cf1ad18 JavaScriptCore`doThrow(exec=0x0000000115ea5190, pc=0x00007ffdb6eb0010) + 136 at LLIntExceptions.cpp:51
    frame #4: 0x000000010cf1ac7d JavaScriptCore`JSC::LLInt::returnToThrow(exec=0x0000000115ea5190, pc=0x00007ffdb6eb0010) + 29 at LLIntExceptions.cpp:60
    frame #5: 0x000000010cf21ddd JavaScriptCore`llint_slow_path_throw(exec=0x0000000115ea5190, pc=0x00007ffdb6eb0010) + 173 at LLIntSlowPaths.cpp:1222
    frame #6: 0x000000010cf29abb JavaScriptCore`llint_op_throw + 15

(lldb) bt
* thread #1: tid = 0x96660e, 0x000000010f3bb3f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007ffdabc43e80, debuggerCallFrame=0x00007fff547ee018, sourceID=1, lineNumber=26, columnNumber=0, hasHandler=true) + 39 at ScriptDebugServer.cpp:529, queue = 'com.apple.main-thread, stop reason = breakpoint 1.1
    frame #0: 0x000000010f3bb3f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007ffdabc43e80, debuggerCallFrame=0x00007fff547ee018, sourceID=1, lineNumber=26, columnNumber=0, hasHandler=true) + 39 at ScriptDebugServer.cpp:529
    frame #1: 0x000000010cdec89d JavaScriptCore`JSC::Interpreter::unwind(this=0x00007ffdadb09ab0, callFrame=0x00007fff547ee0f0, exceptionValue=0x00007fff547ee0e8, bytecodeOffset=46) + 909 at Interpreter.cpp:668
    frame #2: 0x000000010ce0d4ae JavaScriptCore`JSC::genericUnwind(vm=0x00007ffdae005e00, callFrame=0x0000000115ea50b8, exceptionValue=JSValue at 0x00007fff547ee0e8, vPCIndex=46) + 110 at JITExceptions.cpp:73
    frame #3: 0x000000010ce0d5ec JavaScriptCore`JSC::jitThrowNew(vm=0x00007ffdae005e00, callFrame=0x0000000115ea50b8, exceptionValue=JSValue at 0x00007fff547ee138) + 60 at JITExceptions.cpp:96
    frame #4: 0x000000010ce2d8e2 JavaScriptCore`cti_vm_handle_exception(callFrame=0x0000000115ea50b8) + 178 at JITStubs.cpp:2189
    frame #5: 0x000000010ce23d29 JavaScriptCore`ctiVMHandleException + 8

[Joseph Pecoraro]

This affects some LayoutTests landed in: <http://trac.webkit.org/changeset/155471>

When committing a fix for this, you should remove the workarounds added to these tests, and update their expected results:
LayoutTests/inspector-protocol/debugger/setPauseOnExceptions-all.html
LayoutTests/inspector-protocol/debugger/setPauseOnExceptions-uncaught.html

That also means you don't necessarily have to add a specific test for this fix, since it would be covered by these existing tests!

[Peter Wang]

Created attachment 248504 [details]
Patch

[Peter Wang]

By my investigation, if the exception is not truly cleared in VM, it means that exception is thrown to the caller. So, as long as the 'Debugger::m_currentException" is saving the right status of the exception in VM, we can use it to check if it's the same exception sent to debugger.

[Peter Wang]

Excuse me, is here anybody can help to review?

[Joseph Pecoraro]

Comment on attachment 248504 [details]
Patch

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

This seems fine to me. Mark Lam would be a better reviewer.

> Source/JavaScriptCore/debugger/Debugger.cpp:158
> +    , m_currentException(JSValue())

I don't think this is necessary, since JSValue is a class, the default constructor would get called, and be exactly this.

> Source/JavaScriptCore/debugger/Debugger.cpp:633
> +    if (m_currentException != JSValue() && m_vm->exception() == JSValue()
> +        && !m_hasHandlerForExceptionCallback)

Likewise these comparison to JSValue(). Can they just be "!foo" since JSCJSValue has operator UnspecifiedBoolType?

  if (m_currentException && !m_vm->exception() && !m_hasHandlerForExceptionCallback)
      m_currentException = JSValue();

> Source/JavaScriptCore/debugger/Debugger.cpp:698
> +    if (m_isPaused && m_currentException != JSValue())

Likewise, could this just be "&& m_currentException".

[Mark Lam]

Comment on attachment 248504 [details]
Patch

I agree with Joe's comments in terms of code styling.  That said, I'm not convinced that this fix is correct.  To get a feel for why this fix is needed, I tried running the tests (with the test changes applied) without the Debugger.cpp changes, and I don't see any failures.  Hence, it appears  that the original issue that necessitated those FIXMEs in the tests no longer exists.

Peter, are you seeing an issue that still manifests without your fix?  If so, can you please provide a test that demonstrates that issue?

[Peter Wang]

Thank you for your comments, Joseph and Mark. Let me check it again and update the patch.

[Peter Wang]

Created attachment 248922 [details]
A new test case.

The patch r164139 (SHA 4cbc701b) has re-implemented "Array.prototype.map" using JS in file "JavaScriptCore/builtins/Array.prototype.js", so, it's not "host function" now.
So we have to change the case to use "Map.prototype.forEach" instead.

[Peter Wang]

Created attachment 249100 [details]
Patch

[Peter Wang]

Hi Joseph and Mark, the new patch has been uploaded. 

There 3 modifications included:
(1) Change the Layout test case, since the new implementation (SHA 4cbc701b) of "Array.prototype.map" can workaround this bug.
(2) Correct the coding problem of the old patch.
(3) Take a missed case into consideration: when VM is unwinding CallFrames it will clear the exception temporarily.

The root cause of this bug is that the exception, thrown form the JS callback of host function, will be handled twice. So if the 'Debugger::m_currentException' can save the real state of exception of VM, we can use it to justify if it's a same exception triggering the debugger in second time.

Please help to review, thank you very much.

[Build Bot]

Comment on attachment 249100 [details]
Patch

Attachment 249100 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/4629479943045120

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 249126 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Joseph Pecoraro]

Comment on attachment 249100 [details]
Patch

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

Definitely needs Mark to review, since you're changing the Interpreter.

> LayoutTests/ChangeLog:8
> +        Remove the workarounds in accroding test cases.

Typo: "accroding" => "associated"?

[Mark Lam]

(In reply to comment #10)
> Hi Joseph and Mark, the new patch has been uploaded. 
> 
> There 3 modifications included:
> (1) Change the Layout test case, since the new implementation (SHA 4cbc701b)
> of "Array.prototype.map" can workaround this bug.
> (2) Correct the coding problem of the old patch.
> (3) Take a missed case into consideration: when VM is unwinding CallFrames
> it will clear the exception temporarily.
> 
> The root cause of this bug is that the exception, thrown form the JS
> callback of host function, will be handled twice. So if the
> 'Debugger::m_currentException' can save the real state of exception of VM,
> we can use it to justify if it's a same exception triggering the debugger in
> second time.

This information about the root cause is helpful in understanding what is being fixed.  It should be included in the ChangeLog.

> Please help to review, thank you very much.

That said, the change in Debugger::exception() that removes the clearing of m_currentException still doesn’t feel right to me.  Let me think about the patch a bit more, and I’ll get back to you with some feedback soon.

[Mark Lam]

Comment on attachment 249100 [details]
Patch

Thank you for your effort on this patch, but after analyzing the issue, I've concluded that this patch is not appropriate for the following reasons:

1. The patch is complicated and makes the code difficult to understand and reason about.  Specifically, I'm referring to all the interaction between Debugger::m_currentException, m_vm->exception(), m_unwindingCallFrame, and m_hasHandlerForExceptionCallback.

2. The patch results in a bug.  I wrote a simple test file:

<script>
function foo(x) {
    throw "Stuff";
}
function goo() {
    foo();
}
goo();
</script>

With this patch, the WebInspector with "Break on Uncaught Exceptions" enabled, will only break the first time you reload the webpage.  On all subsequent reloads of the webpage, the Inspector will no longer break on the uncaught exception.  This is a regression from how ToT works.

3. The ChangeLog is also not clear about what the issue is.  I finally understood only after I wrote some more test code and tested ToT against other browsers.

The real issue here is that the existing implementation will break once (when "Break on Uncaught Exceptions" is enabled) for every entry / re-entry into the VM.  Consider the following stack of calls (where this stack grows down):

      frame 0: JS function j0()
      frame 1: JS function j1()
      frame 2: native C++ function n2()
      frame 3: JS function j3()
      frame 4: JS function j4()         <==== Uncaught exception thrown here 

... and there are no catch clauses in any of those functions.  With the existing implementation, the Inspector will break in frame 4 and frame 1.  The desired behavior is to only break in frame 4.

Now knowing this, this is how I think the fix should be implemented:

1. Change Debugger::exception() to Debugger::beginExceptionHandling().  It should set m_hasHandlerForExceptionCallback and m_currentException but not clear them.

2. Implement a Debugger::endExceptionHandling() which clears m_hasHandlerForExceptionCallback and m_currentException.  endExceptionHandling() should be called in 2 places:
    a. The place where the catch clause is handled.
        This is the case where the exception is caught and handled, and the Inspector should break on any exceptions thrown after this.
    b. When the Interpreter exits the outer most VMEntryScope.
        This is the case where the exception is unhandled, and we've unwound out of all JS code.  The Inspector should also break on any exceptions thrown in new JS code executed after this.

3. Implement some way to prevent the Interpreter from calling beginExceptionHandling() again if the debugger is already in the process of handling an exception.  One way is to add a isHandlingException() query to Debugger like so:

    bool isHandlingException() const { return !!m_currentException; }

... and add that in Interpreter::unwind() as one of the conditions for allowing debugger->beginExceptionHandling() to be called.

Because we're changing how long m_currentException is set, there might be assertions in Debugger that needs to be updated.  I would do some more testing with a debug build.  At minimum:
1. run all inspector layout tests with a debug build.
2. manually use the debug build Inspector to debug some JS code (which throw exceptions, uncaught and caught) to make sure everything is working as expected and nothing broke.

Part 2 above may be a little more challenging for someone who is not familiar with the VM, but some grepping and debugging should get you there.  If not, I can take a look into implementing the fix myself some time this weekend.

[Mark Lam]

This issue turns out to be only 1 symptom of deeper issue.  I’m working on a fix for the general issue.  Fix coming soon.

[Mark Lam]

The exceptions re-work in https://bugs.webkit.org/show_bug.cgi?id=145525 includes the fix for this issue (among a few other things).  Hence, I'll close this one as a dup.

*** This bug has been marked as a duplicate of bug 145525 ***