Bug 159448 - StackVisitor::unwindToMachineCodeBlockFrame() may unwind past a VM entry frame when catching an exception and the frame has inlined tail calls
Summary: StackVisitor::unwindToMachineCodeBlockFrame() may unwind past a VM entry fram...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-05 18:36 PDT by Saam Barati
Modified: 2016-07-05 22:25 PDT (History)
12 users (show)

See Also:


Attachments
WIP (1.78 KB, patch)
2016-07-05 19:16 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (4.41 KB, patch)
2016-07-05 19:45 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-07-05 18:36:28 PDT
If we see that we have a sequence of tail calls that were inlined into a single machine frame, gotoNextFrame()
will step over the machine callee and go to its caller (because it made a tail call, we treat the callee
as logically not on the stack). This is a huge bug if we're unwinding from an exception where it's illegal
to skip over VM entry frames. The machine caller of this tail called machine frame might be the VM entry frame.
I think because unwindToMachineCodeBlockFrame() has sufficiently specific behavior, it should just do its own special unwinding.
Comment 1 Saam Barati 2016-07-05 18:37:37 PDT
<rdar://problem/27084459>
Comment 2 Saam Barati 2016-07-05 19:16:26 PDT
Created attachment 282838 [details]
WIP

Pretty sure this fixes the bug. Need to write up a changelog.
Comment 3 Mark Lam 2016-07-05 19:36:57 PDT
Comment on attachment 282838 [details]
WIP

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

> Source/JavaScriptCore/tests/stress/dont-unwind-past-vm-entry-frame.js:35
> +    for (i = 0; i < 10000; i++) {
> +        try {
> +            p();
> +        } catch(e) { }
> +    }

I presume the expectation here is that this test should not crash.  Can you add a comment indicating that at the top of the file?
Comment 4 Saam Barati 2016-07-05 19:45:52 PDT
Created attachment 282844 [details]
patch
Comment 5 Mark Lam 2016-07-05 19:48:57 PDT
Comment on attachment 282844 [details]
patch

r=me
Comment 6 WebKit Commit Bot 2016-07-05 22:25:41 PDT
Comment on attachment 282844 [details]
patch

Clearing flags on attachment: 282844

Committed r202847: <http://trac.webkit.org/changeset/202847>
Comment 7 WebKit Commit Bot 2016-07-05 22:25:46 PDT
All reviewed patches have been landed.  Closing bug.