Bug 164969 - Fix exception scope verification failures in LLIntSlowPaths.cpp
Summary: Fix exception scope verification failures in LLIntSlowPaths.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-11-18 16:47 PST by Mark Lam
Modified: 2016-11-28 12:45 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (3.07 KB, patch)
2016-11-18 16:50 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 2016-11-18 16:47:15 PST
Patch coming.
Comment 1 Mark Lam 2016-11-18 16:50:38 PST
Created attachment 295223 [details]
proposed patch.
Comment 2 Geoffrey Garen 2016-11-28 12:12:06 PST
Comment on attachment 295223 [details]
proposed patch.

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

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1452
> +    throwScope.release();
>      return setUpCall(execCallee, pc, kind, calleeAsValue);

I think the throwScope.release() function should assert that there is no pending exception. Otherwise, this release point is an opportunity to make an error when calling into setUpCall.
Comment 3 Mark Lam 2016-11-28 12:32:26 PST
(In reply to comment #2)
> Comment on attachment 295223 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295223&action=review
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1452
> > +    throwScope.release();
> >      return setUpCall(execCallee, pc, kind, calleeAsValue);
> 
> I think the throwScope.release() function should assert that there is no
> pending exception. Otherwise, this release point is an opportunity to make
> an error when calling into setUpCall.

I agree. but I'll do this in a separate patch because I think this will result in some failures due to some old scope.release() being added this way (for various reasons):

    result = callSomethingThatThrows();
    scope.release();
    return result;
 
These need to be fixed like so:

    scope.release();
    return callSomethingThatThrows();

I'll do that fix up along with adding the assertion to scope.release() in one patch.
Comment 4 Mark Lam 2016-11-28 12:37:40 PST
(In reply to comment #3)
> I'll do that fix up along with adding the assertion to scope.release() in
> one patch.

Bug for making scope.release() assert no pending exceptions is https://bugs.webkit.org/show_bug.cgi?id=165105.
Comment 5 Mark Lam 2016-11-28 12:45:12 PST
Thanks for the review.  Landed in r209007: <http://trac.webkit.org/r209007>.