Bug 125648 - jsCStack: Fix handling of uncaught exceptions
Summary: jsCStack: Fix handling of uncaught exceptions
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:
Depends on:
Blocks:
 
Reported: 2013-12-12 12:49 PST by Mark Lam
Modified: 2013-12-12 13:23 PST (History)
5 users (show)

See Also:


Attachments
the patch. (1.63 KB, patch)
2013-12-12 12:54 PST, Mark Lam
no flags Details | Formatted Diff | Diff
revised patch. (1.62 KB, patch)
2013-12-12 13:01 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-12-12 12:49:07 PST
The LLINT handleUncaughtException was not working with the correct CallFrame.  It should load the callFrame from VM::callFrameForThrow (which should more appropriately be named callFrameForCatch) instead of assuming that the current frame is the frame that will "catch" i.e. handle the exception.  In the case of the uncaught exception case, the "catch" frame should be the global frame which VM::callFrameForThrow provides.

The baseline JIT still does not handle uncaught exceptions correctly.  Will look at that separately.
Comment 1 Mark Lam 2013-12-12 12:54:09 PST
Created attachment 219110 [details]
the patch.
Comment 2 Mark Lam 2013-12-12 12:59:41 PST
Comment on attachment 219110 [details]
the patch.

I stand corrected.  This patch also fixes uncaught exceptions for the baseline JIT.  I previously thought it didn't because I had initially implemented the fix by making llint_slow_handle_exception set the returning ExecState.  I've since moved the loading of callFrameForThrow to the LLINT handleUncaughtException thunk which fixes the issue for both the LLINT and the baseline JIT.  Will fix ChangeLog and upload another patch.
Comment 3 Mark Lam 2013-12-12 13:01:58 PST
Created attachment 219111 [details]
revised patch.
Comment 4 Geoffrey Garen 2013-12-12 13:18:34 PST
Comment on attachment 219111 [details]
revised patch.

r=me

Do we need this "bpeq CodeBlock[cfr], 1, .calleeFramePopped / loadp CallerFrame[cfr], cfr" business? I don't think so. You should look into removing it. Since we know we're handling an uncaught exception, we know for certain that callFrameForThrow is the sentinel CallFrame, and we don't need to test it.
Comment 5 Mark Lam 2013-12-12 13:19:52 PST
(In reply to comment #4)
> (From update of attachment 219111 [details])
> r=me
> 
> Do we need this "bpeq CodeBlock[cfr], 1, .calleeFramePopped / loadp CallerFrame[cfr], cfr" business? I don't think so. You should look into removing it. Since we know we're handling an uncaught exception, we know for certain that callFrameForThrow is the sentinel CallFrame, and we don't need to test it.

I'll remove that in my next patch when I fix all exception handling.
Comment 6 Mark Lam 2013-12-12 13:23:26 PST
Thanks for the review.  Landed in r160505 on the jsCStack branch: <http://trac.webkit.org/r160505>.