Bug 161498 – Add CatchScope and force all exception checks to be via ThrowScope or CatchScope.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Details coming.

[Mark Lam]

Created attachment 287783 [details]
work in progress: still need ChangeLogs, perf tests, and comments.

Let's try this on the EWS.

[Mark Lam]

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.

[Mark Lam]

Created attachment 287838 [details]
work in progress: rebased.

[Mark Lam]

Created attachment 287839 [details]
work in progress: rebased again.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 287902 [details]
work in progress: rebased again.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 287903 [details]
work in progress.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 287904 [details]
work in progress

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Created attachment 288034 [details]
proposed patch.

[Mark Lam]

Created attachment 288036 [details]
proposed patch.

[Geoffrey Garen]

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?

[Mark Lam]

Comment on attachment 288036 [details]
proposed patch.

Taking out of review.  I'm revising with feedback from Geoff.

[Mark Lam]

Created attachment 288184 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 288184 [details]
proposed patch.

r=me

[Mark Lam]

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.

[Mark Lam]

Thanks for the review.  Landed in r205569: <http://trac.webkit.org/r205569>.