[Webkit-unassigned] [Bug 121108] Web Inspector gets paused twice when there is an exception in host function

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121108

Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #249100|review?                     |review-
              Flags|                            |

--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 249100
  --> https://bugs.webkit.org/attachment.cgi?id=249100
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150324/97fcc7e0/attachment-0002.html>