Bug 161498

Summary: Add CatchScope and force all exception checks to be via ThrowScope or CatchScope.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=161756
Bug Depends on: 161499    
Bug Blocks:    
Attachments:
Description Flags
work in progress: still need ChangeLogs, perf tests, and comments.
none
x86_64 benchmark result run 1
none
work in progress: rebased.
none
work in progress: rebased again.
none
work in progress: rebased again.
none
work in progress.
none
work in progress
buildbot: commit-queue-
Archive of layout-test-results from ews114 for mac-yosemite
none
proposed patch.
none
proposed patch.
mark.lam: review-
proposed patch. ggaren: review+

Description Mark Lam 2016-09-01 11:38:46 PDT
Details coming.
Comment 1 Mark Lam 2016-09-02 11:08:33 PDT
Created attachment 287783 [details]
work in progress: still need ChangeLogs, perf tests, and comments.

Let's try this on the EWS.
Comment 2 Mark Lam 2016-09-02 17:29:58 PDT
Created attachment 287837 [details]
x86_64 benchmark result run 1

This is the benchmark results from the last patch.  The only reproducible diffs there are "definitely" regressions in longspider:hash-map and microbenchmarks:string-sub.  However, after I rebased my workspaces to ToT r205382 with no code changes of my own, these regressions disappeared.  I think it's fair to say that they are noise due to cache alignment or something, and call it a wash.

Will upload the newly rebased patch for EWS testing shortly.  I still need to do a manual review of the patch and write ChangeLog and documentation before it's ready for review.
Comment 3 Mark Lam 2016-09-02 17:30:49 PDT
Created attachment 287838 [details]
work in progress: rebased.
Comment 4 Mark Lam 2016-09-02 17:45:00 PDT
Created attachment 287839 [details]
work in progress: rebased again.
Comment 5 WebKit Commit Bot 2016-09-02 17:48:19 PDT
Attachment 287839 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 222 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Lam 2016-09-04 07:57:00 PDT
Created attachment 287902 [details]
work in progress: rebased again.
Comment 7 WebKit Commit Bot 2016-09-04 08:00:03 PDT
Attachment 287902 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 222 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Mark Lam 2016-09-04 08:03:57 PDT
Created attachment 287903 [details]
work in progress.
Comment 9 WebKit Commit Bot 2016-09-04 08:06:38 PDT
Attachment 287903 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/WorkerScriptController.cpp:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 222 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Mark Lam 2016-09-04 08:22:52 PDT
Created attachment 287904 [details]
work in progress
Comment 11 Build Bot 2016-09-04 09:37:31 PDT
Comment on attachment 287904 [details]
work in progress

Attachment 287904 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2006784

New failing tests:
fast/dom/Window/window-postmessage-clone.html
Comment 12 Build Bot 2016-09-04 09:37:34 PDT
Created attachment 287911 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 13 Mark Lam 2016-09-06 11:14:22 PDT
Created attachment 288034 [details]
proposed patch.
Comment 14 Mark Lam 2016-09-06 12:21:04 PDT
Created attachment 288036 [details]
proposed patch.
Comment 15 Geoffrey Garen 2016-09-06 12:58:58 PDT
Comment on attachment 288036 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:14
> +        3. ThrowScope and CatchScope are now sub-classes of ExceptionScope.  This is needed
> +           because ThrowScope also needs to be aware of CatchScopes that enclose it, so
> +           as to be able to determine whether it needs to simulate a re-throw or not.

I don't understand this. Under what conditions is it beneficial for a ThrowScope not to simulate a throw?

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:235
> +        auto scope = DECLARE_CATCH_SCOPE(vm);

I thought we liked putting these at the top of the function?

> Source/JavaScriptCore/runtime/CatchScope.cpp:33
> +// See comments at the top of ThrowScope.cpp for details on how CatchScopes work.

I don't like hyperlinks in comments. Can we put the description of CatchScope into CatchScope.h?

> Source/JavaScriptCore/runtime/CatchScope.h:45
> +        m_vm.m_needExceptionCheck = false;

This line should move into VM::clearException().

> Source/JavaScriptCore/runtime/ExceptionScope.cpp:43
> +    if (m_vm.m_traceExceptionEvents) {

I don't think we want this in-tree. We already have extremely verbose reporting for exception checks.

> Source/JavaScriptCore/runtime/ThrowScope.cpp:102
> +       Note: it is wrong to assert that there is no exception on entry into a CatchScope.
> +       This is because a ThrowScope may enclose a CatchScope, and use the CatchScope
> +       to clear the exception only in some cases. Hence, we need to be able to enter
> +       the CatchScope with a pending exception. The only invariant is that we must
> +       never exit it with an exception still pending.

This requirement decreases our error checking ability.

Do we need to keep things this way? Why can't we declare our catch scope at a high enough level so that it encloses any possible exception throw?
Comment 16 Mark Lam 2016-09-06 14:52:10 PDT
Comment on attachment 288036 [details]
proposed patch.

Taking out of review.  I'm revising with feedback from Geoff.
Comment 17 Mark Lam 2016-09-07 14:20:55 PDT
Created attachment 288184 [details]
proposed patch.
Comment 18 Geoffrey Garen 2016-09-07 14:45:26 PDT
Comment on attachment 288184 [details]
proposed patch.

r=me
Comment 19 Mark Lam 2016-09-07 15:08:01 PDT
I forgot that we ended up not needing CatchScope::release() so far.  As a result, the patch does not have it in the code.  I will also remove mentions of it in the ChangeLog before landing.
Comment 20 Mark Lam 2016-09-07 15:14:02 PDT
Thanks for the review.  Landed in r205569: <http://trac.webkit.org/r205569>.