Bug 145525 - finally blocks should not set the exception stack trace when re-throwing the exception.
Summary: finally blocks should not set the exception stack trace when re-throwing the ...
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
: 121108 145522 (view as bug list)
Depends on: 145522 145524
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-01 14:01 PDT by Mark Lam
Modified: 2015-06-09 10:35 PDT (History)
11 users (show)

See Also:


Attachments
work in progress: for EWS testing and personal review. (128.37 KB, patch)
2015-06-02 13:27 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 2 for EWS testing. Still need to write tests. (133.21 KB, patch)
2015-06-03 00:13 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 3: fixed broken builds for another round of EWS testing (133.48 KB, patch)
2015-06-03 01:51 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 4: another try to fix those gtk, efl build failures. (135.05 KB, patch)
2015-06-03 02:55 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 5: +Windows fix, +test (148.33 KB, patch)
2015-06-03 12:21 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 6: windows build fix again for EWS. (148.33 KB, patch)
2015-06-03 13:19 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 7: hopefully for real this time. (165.85 KB, patch)
2015-06-03 20:19 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 8: windows build fixed (166.12 KB, patch)
2015-06-04 00:37 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
patch for landing with fixes applied. (245.49 KB, patch)
2015-06-05 11:15 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-06-01 14:01:07 PDT
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.
Comment 1 Mark Lam 2015-06-01 14:03:26 PDT
<rdar://problem/18780129>
Comment 2 Mark Lam 2015-06-01 14:03:27 PDT
<rdar://problem/18780129>
Comment 3 Mark Lam 2015-06-02 13:26:05 PDT
*** Bug 145522 has been marked as a duplicate of this bug. ***
Comment 4 Mark Lam 2015-06-02 13:27:30 PDT
Created attachment 254092 [details]
work in progress: for EWS testing and personal review.
Comment 5 Mark Lam 2015-06-03 00:13:59 PDT
Created attachment 254160 [details]
patch 2 for EWS testing.  Still need to write tests.
Comment 6 Mark Lam 2015-06-03 01:51:50 PDT
Created attachment 254161 [details]
patch 3: fixed broken builds for another round of EWS testing
Comment 7 Mark Lam 2015-06-03 02:55:30 PDT
Created attachment 254163 [details]
patch 4: another try to fix those gtk, efl build failures.
Comment 8 Mark Lam 2015-06-03 12:21:04 PDT
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.
Comment 9 WebKit Commit Bot 2015-06-03 12:23:36 PDT
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.
Comment 10 Mark Lam 2015-06-03 13:14:06 PDT
*** Bug 121108 has been marked as a duplicate of this bug. ***
Comment 11 Mark Lam 2015-06-03 13:19:24 PDT
Created attachment 254201 [details]
patch 6: windows build fix again for EWS.
Comment 12 Joseph Pecoraro 2015-06-03 13:21:06 PDT
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.
Comment 13 Joseph Pecoraro 2015-06-03 13:22:46 PDT
> 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.
Comment 14 Mark Lam 2015-06-03 20:19:26 PDT
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.
Comment 15 Mark Lam 2015-06-04 00:37:13 PDT
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 16 Mark Lam 2015-06-04 05:54:19 PDT
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 17 Geoffrey Garen 2015-06-04 15:12:03 PDT
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.
Comment 18 Mark Lam 2015-06-05 11:15:10 PDT
Created attachment 254367 [details]
patch for landing with fixes applied.
Comment 19 Timothy Hatcher 2015-06-05 11:20:04 PDT
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.
Comment 20 Mark Lam 2015-06-05 11:55:40 PDT
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>.
Comment 21 Alexey Proskuryakov 2015-06-05 16:35:33 PDT
This broke many inspector tests on Windows:

https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r185269%20(52268)/results.html
Comment 22 Mark Lam 2015-06-05 16:51:45 PDT
(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.
Comment 23 Mark Lam 2015-06-05 18:31:37 PDT
(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.
Comment 24 Ryosuke Niwa 2015-06-09 10:35:56 PDT
Looks like this was 1.5% progression on Speedometer.