Bug 133182 - REGRESSION(r154797): Debugger crashes when stepping over an uncaught exception
Summary: REGRESSION(r154797): Debugger crashes when stepping over an uncaught exception
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-22 08:49 PDT by Mark Lam
Modified: 2014-05-22 14:41 PDT (History)
4 users (show)

See Also:


Attachments
the patch. (10.28 KB, patch)
2014-05-22 12:11 PDT, Mark Lam
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-05-22 08:49:06 PDT
Before r154797, we used to clear the VM exception before calling into the debugger.  After r154797, we don’t.  This patch will restore this clearing of the exception.
Comment 1 Mark Lam 2014-05-22 08:49:50 PDT
<rdar://problem/16520770>
Comment 2 Mark Lam 2014-05-22 12:11:23 PDT
Created attachment 231902 [details]
the patch.
Comment 3 Oliver Hunt 2014-05-22 14:30:13 PDT
Comment on attachment 231902 [details]
the patch.

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

> Source/JavaScriptCore/interpreter/Interpreter.cpp:455
> +        ASSERT(!callFrame->hadException());

Why will this assertion not fire?  Can't the debugger legitimately cause an unhanded exception?
Comment 4 Mark Lam 2014-05-22 14:32:32 PDT
(In reply to comment #3)
> (From update of attachment 231902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231902&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:455
> > +        ASSERT(!callFrame->hadException());
> 
> Why will this assertion not fire?  Can't the debugger legitimately cause an unhanded exception?

The inspector is supposed to catch and handle any exceptions thrown in the debugging session.  I don’t think there’s currently a way to introduce an exception where there is none.  If this changes, we can change this assert in the future.
Comment 5 Joseph Pecoraro 2014-05-22 14:37:17 PDT
Comment on attachment 231902 [details]
the patch.

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

>>> Source/JavaScriptCore/interpreter/Interpreter.cpp:455
>>> +        ASSERT(!callFrame->hadException());
>> 
>> Why will this assertion not fire?  Can't the debugger legitimately cause an unhanded exception?
> 
> The inspector is supposed to catch and handle any exceptions thrown in the debugging session.  I don’t think there’s currently a way to introduce an exception where there is none.  If this changes, we can change this assert in the future.

If the Inspector executes anything on its own behalf (internal implementation details, breakpoint actions, probes, etc) we should be catching and clearing exception information. This assert sounds reasonable to me, and might bring to light a situation in the Inspector where we might not have been cleaning up after ourselves and probably should be.
Comment 6 Mark Lam 2014-05-22 14:41:37 PDT
Thanks for the review.  Landed in r169221: <http://trac.webkit.org/r169221>.