Finally blocks are implemented as a Catch block with an op_throw at its end to re-throw the exception. Currently, this throw in the finally block is no different than the throw that originated the exception. As a result, the exception stack trace is now set to the re-throw location in the finally block. This results in the Inspector reporting the wrong throw point for the exception.
<rdar://problem/18780129>
*** Bug 145522 has been marked as a duplicate of this bug. ***
Created attachment 254092 [details] work in progress: for EWS testing and personal review.
Created attachment 254160 [details] patch 2 for EWS testing. Still need to write tests.
Created attachment 254161 [details] patch 3: fixed broken builds for another round of EWS testing
Created attachment 254163 [details] patch 4: another try to fix those gtk, efl build failures.
Created attachment 254192 [details] patch 5: +Windows fix, +test Still not ready for review. This patch contains hacks to work around a potential bug in the Inspector code. Will need to address that Inspector issue before I can finalize this patch.
Attachment 254192 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorTimelineAgent.cpp:322: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebCore/inspector/InspectorTimelineAgent.cpp:707: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 100 files If any of these errors are false positives, please file a bug against check-webkit-style.
*** Bug 121108 has been marked as a duplicate of this bug. ***
Created attachment 254201 [details] patch 6: windows build fix again for EWS.
Comment on attachment 254192 [details] patch 5: +Windows fix, +test View in context: https://bugs.webkit.org/attachment.cgi?id=254192&action=review Some testing tips. > LayoutTests/inspector/debugger/break-on-exceptions-expected.txt:38 > +PAUSE #1 > + REASON: exception > +Paused at foo:12:34 > +RESUMED Output might look nicer if everything within the pause/resumed section was indented. PAUSE #1 REASON: exception LOCATION: foo:12:34 RESUMED You could even slim this down more, and make the reason an assertion (InspectorTest.assert(foo.bar.pauseReason === "exception")) and just output the pause locations. Pass 1: testExceptions with break on all exceptions PAUSED AT: foo:12:34 PAUSED AT: foo:17:16 Pass 2: ... PAUSED AT: ... > LayoutTests/inspector/debugger/break-on-exceptions.html:29 > + InspectorTest.evaluateInPage("setTimeout(function() { console.log('" + str + "'); }, 0)"); Seems this would work better as an InspectorTest.log. So you would know in the bottom part of the expected results, which PAUSEs correspond to the different test scenarios. It is non-obvious for example that the Pause 1 and 2 correspond to "Pass 1", and Pause 6 corresponded to "Pass 3". > LayoutTests/inspector/debugger/break-on-exceptions.html:73 > + { expression: // resumed from Throw Point 1. > + "nothingToDo();" > + }, Are these nothingToDo's actually necessary? I think the test might make more sense like: var scenarios = [ { title: "Pass 1: testExceptions with break on all exceptions", setup: "enableBreakOnAllExceptions(); disableBreakOnUncaughtExceptions()", expression: "runTestCase('testExceptions')" }, title: "Pass 2: ...", setup: "...", expression: "..." } ] Where each scenario has a single expression, and you just log every pause that happens. When you've resumed, move onto the next. But I may be overlooking something. > LayoutTests/inspector/debugger/break-on-exceptions.html:156 > +// WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange, function(event) { Paused / Resumed are "the first time you paused" and "we resumed and didn't pause again for 50ms". So these events won't trigger during stepping / pausing immediately after continuing. They try to be higher level. CallFramesDidChange and related events will get dispatched each and every pause, so during stepping / pausing immediately after continuing. > LayoutTests/inspector/debugger/break-on-exceptions.html:173 > + <p>Test that pausing in different ways triggers different pause reasons.</p> You should update the description of this test.
> CallFramesDidChange and related events will get dispatched each and every > pause, so during stepping / pausing immediately after continuing. I should clarify. Every individual pause and resume! So the first pause, steps, and resume.
Created attachment 254233 [details] patch 7: hopefully for real this time. I'll wait for the EWS bots to clear before marking it for review.
Created attachment 254257 [details] patch 8: windows build fixed The only remaining issue is that the new test break-on-exceptions.html triggers an assertion failure in the InspectorTimelineAgent. I think that this is a latent bug in the Inspector that the test just uncovers. I'll check with the Inspector folks tomorrow and update the patch to skip the test on debug builds if needed.
Comment on attachment 254257 [details] patch 8: windows build fixed Bots are green. Let’s get this reviewed while I look into the InspectorTimelineAgent issue.
Comment on attachment 254257 [details] patch 8: windows build fixed View in context: https://bugs.webkit.org/attachment.cgi?id=254257&action=review r=me if you perform the fixups below: > Source/JavaScriptCore/ChangeLog:14 > + Whatâs wrong with how it presently works: Please use ASCII here and elsewhere. > Source/JavaScriptCore/ChangeLog:40 > + object or not. If it is not an Exception object, then we must be thrown a new thrown=>throwing > Source/JavaScriptCore/API/JSObjectRef.cpp:65 > +static bool handleExceptionIfNeeded(ExecState* exec, JSValueRef* returnedExceptionRef) bool is opaque. Let's use an enum class { DidThrowException, DidNotThrowException }. > Source/JavaScriptCore/API/JSValueRef.cpp:56 > +static bool handleExceptionIfNeeded(ExecState* exec, JSValueRef* returnedExceptionRef) Ditto. > Source/JavaScriptCore/bytecode/CodeBlock.h:188 > + HandlerInfo* handlerForBytecodeOffset(unsigned bytecodeOffset, bool requireCatchHandler = false); Please use enum class { CatchHandler, AnyHandler }. > Source/JavaScriptCore/runtime/Exception.cpp:45 > + exception->Exception::~Exception(); No need for Exception:: here. > Source/JavaScriptCore/runtime/Exception.cpp:63 > + if (!thisObject->m_value) > + return; > + if (!thisObject->m_value.get().isCell()) > + return; > + visitor.append(&thisObject->m_value); You should just append without checking. append() will check for you. > Source/JavaScriptCore/runtime/Exception.cpp:74 > +Exception::Exception(VM& vm, JSValue thrownValue) > + : Base(vm, vm.exceptionStructure.get()) > +{ > + m_value.set(vm, this, thrownValue); > + > + Vector<StackFrame> stackTrace; > + vm.interpreter->getStackTrace(stackTrace); > + m_stack = RefCountedArray<StackFrame>(stackTrace); > +} This is wrong. GC objects should never do interesting work in their constructors. Interesting work must be delayed to the finishCreation function. > Source/JavaScriptCore/runtime/Exception.h:34 > +class Exception : public JSObject { JSNonFinalObject is better. > Source/JavaScriptCore/runtime/VM.cpp:564 > + if (thrownValue.isCell() && thrownValue.asCell()->structure() == exceptionStructure.get()) Please use jsDynamicCast instead. > LayoutTests/inspector/debugger/resources/break-on-exceptions.js:1 > +window.onerror = function(e){ Let's break the different test cases into different file: onerror, for-of, catch, finally, forEach, promises.
Created attachment 254367 [details] patch for landing with fixes applied.
Comment on attachment 254367 [details] patch for landing with fixes applied. View in context: https://bugs.webkit.org/attachment.cgi?id=254367&action=review > LayoutTests/http/tests/inspector/inspector-test.js:146 > +InspectorTestProxy.needToSanitizeUncaughtExceptionUrls = false; URLs should be capitalized. > LayoutTests/http/tests/inspector/inspector-test.js:153 > + var lastSlash = url.lastIndexOf('/'); > + var lastBackSlash = url.lastIndexOf('\\'); We use double quotes exclusively.
I've updated the patch with Tim's feedback. Geoff's feedback has already been applied. The elf WES bot failure is due to a pre-existing condition in ToT. The gtk merge failure is simply due to a merge conflict. Landed in r185259: <http://trac.webkit.org/r185259>.
This broke many inspector tests on Windows: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r185269%20(52268)/results.html
(In reply to comment #21) > This broke many inspector tests on Windows: > > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/ > r185269%20(52268)/results.html The breakage appears to be unrelated to exception breakpoints, and only manifests on Windows. I suspect that the issue might be that the Windows bot needs a clean build.
(In reply to comment #22) > (In reply to comment #21) > > This broke many inspector tests on Windows: > > > > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/ > > r185269%20(52268)/results.html > > The breakage appears to be unrelated to exception breakpoints, and only > manifests on Windows. I suspect that the issue might be that the Windows > bot needs a clean build. I filed https://bugs.webkit.org/show_bug.cgi?id=145720 to investigate the issue. Turns out that those tests have been skipped for Mac and Debug Win tests bots.
Looks like this was 1.5% progression on Speedometer.