Bug 164971 - Fix exception scope verification failures in JSC profiler files.
Summary: Fix exception scope verification failures in JSC profiler files.
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:57 PST by Mark Lam
Modified: 2016-11-22 12:52 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (9.72 KB, patch)
2016-11-18 17:02 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch: updated to use return { } where possible. (10.52 KB, patch)
2016-11-21 12:05 PST, Mark Lam
sbarati: 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:57:16 PST
Patch coming.
Comment 1 Mark Lam 2016-11-18 17:02:11 PST
Created attachment 295225 [details]
proposed patch.
Comment 2 Mark Lam 2016-11-21 12:05:23 PST
Created attachment 295310 [details]
proposed patch: updated to use return { } where possible.
Comment 3 Saam Barati 2016-11-21 15:22:23 PST
Comment on attachment 295310 [details]
proposed patch: updated to use return { } where possible.

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

> Source/JavaScriptCore/profiler/ProfilerCompilation.cpp:120
> +    RETURN_IF_EXCEPTION(scope, { });

Why not make these assertions in this file since this is only used for debugging?
Comment 4 Mark Lam 2016-11-22 12:48:21 PST
Comment on attachment 295310 [details]
proposed patch: updated to use return { } where possible.

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

>> Source/JavaScriptCore/profiler/ProfilerCompilation.cpp:120
>> +    RETURN_IF_EXCEPTION(scope, { });
> 
> Why not make these assertions in this file since this is only used for debugging?

We were already checking for some exceptions in this code before.  In this patch, I'm filling out more exception checks.  It might be appropriate to replace all these with assertions, but that would be a change in behavior.  I'll stick with the exception checks for now.  We can consider a change of behavior to use assertions later if desired.
Comment 5 Mark Lam 2016-11-22 12:52:51 PST
Thanks for the review.  Landed in r208968: <http://trac.webkit.org/r208968>.