Bug 145525 – finally blocks should not set the exception stack trace when re-throwing the exception.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

<rdar://problem/18780129>

[Mark Lam]

<rdar://problem/18780129>

[Mark Lam]

*** Bug 145522 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 254092 [details]
work in progress: for EWS testing and personal review.

[Mark Lam]

Created attachment 254160 [details]
patch 2 for EWS testing.  Still need to write tests.

[Mark Lam]

Created attachment 254161 [details]
patch 3: fixed broken builds for another round of EWS testing

[Mark Lam]

Created attachment 254163 [details]
patch 4: another try to fix those gtk, efl build failures.

[Mark Lam]

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.

[WebKit Commit Bot]

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.

[Mark Lam]

*** Bug 121108 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 254201 [details]
patch 6: windows build fix again for EWS.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

> 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.

[Mark Lam]

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.

[Mark Lam]

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.

[Mark Lam]

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.

[Geoffrey Garen]

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.

[Mark Lam]

Created attachment 254367 [details]
patch for landing with fixes applied.

[Timothy Hatcher]

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.

[Mark Lam]

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>.

[Alexey Proskuryakov]

This broke many inspector tests on Windows:

https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r185269%20(52268)/results.html

[Mark Lam]

(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.

[Mark Lam]

(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.

[Ryosuke Niwa]

Looks like this was 1.5% progression on Speedometer.