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.
<rdar://problem/16520770>
Created attachment 231902 [details] the patch.
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?
(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 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.
Thanks for the review. Landed in r169221: <http://trac.webkit.org/r169221>.