Bug 93607 - Web Inspector: [JSC] Caught exception is treated as uncaught
Summary: Web Inspector: [JSC] Caught exception is treated as uncaught
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Nobody
URL:
Keywords: InRadar
: 117215 121010 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-08-09 05:21 PDT by Clark
Modified: 2013-09-10 14:00 PDT (History)
8 users (show)

See Also:


Attachments
[TEST] Test Case (659 bytes, text/html)
2013-09-09 16:09 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (22.55 KB, patch)
2013-09-09 19:10 PDT, Joseph Pecoraro
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Clark 2012-08-09 05:21:39 PDT
Webkit Version: r125153

With the flag "All Uncaught Exception" ON, show Error Console. Load the attached file.

Actual: It paused in the "throw" line.

Expected: Since the "throw" is within a try...catch, it should not be treated as an uncaught exception.

This case is ok.===========================
try {
    throw new Error("x");
} catch (e) {

}

This case is bad: ===========================
try {
    function f() {
      throw new Error("x");
    }
    f();
} catch (e) {

}
Comment 1 Deirdre Saoirse Moen 2012-08-14 22:31:53 PDT
rdar://problem/12101592
Comment 2 Timothy Hatcher 2013-06-05 10:59:23 PDT
*** Bug 117215 has been marked as a duplicate of this bug. ***
Comment 3 Joseph Pecoraro 2013-09-09 16:06:15 PDT
Yep, this looks like a JSC issue.

ScriptDebugServer::execution is getting called with "hasHandler" being false. We would expect this to be true, because there is an eventual exception handler up the stack. I suspect this is a regression that happened a while ago.


---

(lldb) b ScriptDebugServer::exception

(lldb) c
* thread #1: tid = 0x8da30a, 0x00000001124613f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007f948310b130, debuggerCallFrame=0x00007fff51742118, sourceID=1, lineNumber=14, columnNumber=0, hasHandler=false) + 39 at ScriptDebugServer.cpp:529, queue = 'com.apple.main-thread, stop reason = breakpoint 2.1
    frame #0: 0x00000001124613f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007f948310b130, debuggerCallFrame=0x00007fff51742118, sourceID=1, lineNumber=14, columnNumber=0, hasHandler=false) + 39 at ScriptDebugServer.cpp:529
   526 	
   527 	void ScriptDebugServer::exception(const DebuggerCallFrame& debuggerCallFrame, intptr_t sourceID, int lineNumber, int columnNumber, bool hasHandler)
   528 	{
-> 529 	    if (m_paused)
   530 	        return;
   531 	
   532 	    if (m_pauseOnExceptionsState == PauseOnAllExceptions || (m_pauseOnExceptionsState == PauseOnUncaughtExceptions && !hasHandler))

(lldb) p hasHandler
(bool) $8 = false

(lldb) bt
* thread #1: tid = 0x8da30a, 0x00000001124613f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007f948310b130, debuggerCallFrame=0x00007fff51742118, sourceID=1, lineNumber=14, columnNumber=0, hasHandler=false) + 39 at ScriptDebugServer.cpp:529, queue = 'com.apple.main-thread, stop reason = breakpoint 2.1
    frame #0: 0x00000001124613f7 WebCore`WebCore::ScriptDebugServer::exception(this=0x00007f948310b130, debuggerCallFrame=0x00007fff51742118, sourceID=1, lineNumber=14, columnNumber=0, hasHandler=false) + 39 at ScriptDebugServer.cpp:529
    frame #1: 0x000000010fe92b3f JavaScriptCore`JSC::Interpreter::unwind(this=0x00007f9483464d50, callFrame=0x00007fff517421f0, exceptionValue=0x00007fff517421e8, bytecodeOffset=29) + 847 at Interpreter.cpp:633
    frame #2: 0x000000010feb359e JavaScriptCore`JSC::genericUnwind(vm=0x00007f9482821400, callFrame=0x0000000118f600c8, exceptionValue=JSValue at 0x00007fff517421e8, vPCIndex=29) + 110 at JITExceptions.cpp:73
    frame #3: 0x000000010ffc0e08 JavaScriptCore`doThrow(exec=0x0000000118f600c8, pc=0x00007f948303a970) + 136 at LLIntExceptions.cpp:51
    frame #4: 0x000000010ffc0d6d JavaScriptCore`JSC::LLInt::returnToThrow(exec=0x0000000118f600c8, pc=0x00007f948303a970) + 29 at LLIntExceptions.cpp:60
    frame #5: 0x000000010ffc89c5 JavaScriptCore`llint_slow_path_get_from_scope(exec=0x0000000118f600c8, pc=0x00007f948303a970) + 469 at LLIntSlowPaths.cpp:1295
    frame #6: 0x000000010ffce851 JavaScriptCore`llint_op_get_from_scope + 494
Comment 4 Joseph Pecoraro 2013-09-09 16:09:29 PDT
Created attachment 211111 [details]
[TEST] Test Case

Here is a manual test case.
Comment 5 Joseph Pecoraro 2013-09-09 19:10:07 PDT
Created attachment 211132 [details]
[PATCH] Proposed Fix

Check the entire call stack for exception handlers, instead of just the local call frame.

Added tests that cover this and other Inspector pause on exception states.
Comment 6 Mark Lam 2013-09-09 20:09:52 PDT
Comment on attachment 211132 [details]
[PATCH] Proposed Fix

The Interpreter.cpp changes LGTM.
Comment 7 Geoffrey Garen 2013-09-09 21:23:53 PDT
Comment on attachment 211132 [details]
[PATCH] Proposed Fix

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

Thanks, Joe!

r=me, but please fix the logic error below, and add a test for it.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:582
> +        if (!codeBlock)
> +            return StackVisitor::Done;

I think you want StackVisitor::Continue here.

Consider this stack:
    js function with try/catch scope => Array.prototype.map => js function => throw
CodeBlock is null for host functions. Therefore, when you see Array.prototype.map, you'll return StackVisitor::Done and conclude that there's no handler. But there is a handler, in the function that called map.
Comment 8 Paul Miller 2013-09-09 21:26:19 PDT
*** Bug 121010 has been marked as a duplicate of this bug. ***
Comment 9 Joseph Pecoraro 2013-09-10 00:09:44 PDT
(In reply to comment #7)
> (From update of attachment 211132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211132&action=review
> 
> Thanks, Joe!
> 
> r=me, but please fix the logic error below, and add a test for it.
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:582
> > +        if (!codeBlock)
> > +            return StackVisitor::Done;
> 
> I think you want StackVisitor::Continue here.
> 
> Consider this stack:
>     js function with try/catch scope => Array.prototype.map => js function => throw
> CodeBlock is null for host functions. Therefore, when you see Array.prototype.map, you'll return StackVisitor::Done and conclude that there's no handler. But there is a handler, in the function that called map.

Good catch and thanks for the example! That indeed fixes a issue. It also uncovered a somewhat surprising double pause.

Given:

    function exceptionInHostFunction()
    {
        [1].map(function(x) {
            throw "exception in host function";
        });
    }

Setting the inspector to break on all exceptions, and evaluating: setTimeout(exceptionHostFunction, 0);

The inspector pauses twice:

  1. At the throw line. Continuing in Web Inspector goes to (2)
  2. At the [].map line. Continuing in Web Inspector no longer pauses.

I'll take a brief look at this. It exists separate from this issue/patch, but affects the tests I'm writing for this patch.
Comment 10 Joseph Pecoraro 2013-09-10 11:57:54 PDT
(In reply to comment #9)
> I'll take a brief look at this. It exists separate from this issue/patch, but affects the tests I'm writing for this patch.

Filed: <https://webkit.org/b/121108> Web Inspector gets paused twice when there is an exception in host function

I added a workaround in these tests to expect an extra pause.
Comment 11 Joseph Pecoraro 2013-09-10 14:00:08 PDT
Committed <http://trac.webkit.org/changeset/155471>.