Bug 125648 – jsCStack: Fix handling of uncaught exceptions

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 219110 [details]
the patch.

[Mark Lam]

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.

[Mark Lam]

Created attachment 219111 [details]
revised patch.

[Geoffrey Garen]

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.

[Mark Lam]

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

[Mark Lam]

Thanks for the review.  Landed in r160505 on the jsCStack branch: <http://trac.webkit.org/r160505>.