Bug 161171

Summary: Introduce the ThrowScope and force every throw site to instantiate a ThrowScope.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, jfbastien, keith_miller, msaboff, sbarati, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
work in progress
none
x86_64 benchmark result run 1
none
x86_64 benchmark result run 2
none
proposed patch.
mark.lam: review-
proposed patch.
mark.lam: review-, buildbot: commit-queue-
Archive of layout-test-results from ews117 for mac-yosemite
none
proposed patch.
none
proposed patch.
none
revised patch w/ Fil's feedback + old header guard applied. fpizlo: review+

Description Mark Lam 2016-08-24 16:11:38 PDT
This is the first step towards simulating potential JS exceptions throws, and verifying that they are checked for appropriately in C++ code.
Comment 1 Mark Lam 2016-08-25 11:33:20 PDT
Created attachment 286989 [details]
work in progress
Comment 2 Geoffrey Garen 2016-08-25 12:17:42 PDT
Comment on attachment 286989 [details]
work in progress

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

> Source/JavaScriptCore/runtime/ThrowScope.h:39
> +
> +#ifndef ENABLE_EXCEPTION_CHECK_VERIFICATION
> +#define ENABLE_EXCEPTION_CHECK_VERIFICATION (!defined(NDEBUG))
> +#endif

Let's call this ENABLE_THROW_SCOPE_VERIFICATION.

Let's put this setting in Platform.h with other settings.

> Source/JavaScriptCore/runtime/ThrowScope.h:76
> +#define MAKE_THROW_SCOPE(name__, vm__) \

Let's call this DECLARE_THROW_SCOPE.

Instead of taking the local variable name as a parameter, let's do this:

    auto scope = DECLARE_THROW_SCOPE(vm).

That way, even though the declaration is a macro mystery, the nature of the local variable is not.
Comment 3 Mark Lam 2016-08-29 15:18:23 PDT
Created attachment 287336 [details]
x86_64 benchmark result run 1
Comment 4 Mark Lam 2016-08-29 15:18:56 PDT
Created attachment 287337 [details]
x86_64 benchmark result run 2
Comment 5 Mark Lam 2016-08-29 15:22:33 PDT
There's some regressions and progressions in the microbenchmarks.  All other so-called "definitely"s are not consistently reproducible on different runs.

A inspection of disassembled code for regressing benchmarks did not seem to show any bad code that can account for the regressions.  It looks to me like the regressions are just be due to cache effects.

I'll upload the updated patch shortly for review.
Comment 6 Mark Lam 2016-08-29 15:43:38 PDT
Created attachment 287341 [details]
proposed patch.
Comment 7 Mark Lam 2016-08-29 16:21:35 PDT
Comment on attachment 287341 [details]
proposed patch.

I ran the API tests, but forgot to run the bindings test.  I need to rebase the results there.  Will upload a new patch shortly.
Comment 8 Mark Lam 2016-08-29 16:36:38 PDT
Created attachment 287349 [details]
proposed patch.
Comment 9 Build Bot 2016-08-29 17:46:27 PDT
Comment on attachment 287349 [details]
proposed patch.

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

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2016-08-29 17:46:32 PDT
Created attachment 287359 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Mark Lam 2016-08-29 21:42:55 PDT
Comment on attachment 287349 [details]
proposed patch.

I had some silly bugs that resulted in the failures.  Will fix first.
Comment 12 Mark Lam 2016-08-30 00:09:54 PDT
Created attachment 287375 [details]
proposed patch.
Comment 13 Mark Lam 2016-08-30 01:26:08 PDT
Created attachment 287378 [details]
proposed patch.
Comment 14 Filip Pizlo 2016-08-30 09:40:27 PDT
Comment on attachment 287378 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ThrowScope.cpp:297
> +    if (topCallFrame && previousScope > topCallFrame)
> +        willBeHandleByLLIntOrJIT = true;

Is it clear that it's better to have this here, than to have logic in vmEntry?

> Source/JavaScriptCore/runtime/ThrowScope.h:42
> +class ThrowScope {
> +public:
> +    JS_EXPORT_PRIVATE ThrowScope(VM&, ThrowScopeLocation);
> +    JS_EXPORT_PRIVATE ~ThrowScope();
> +

Seems like you could delete the copy constructor/operator but enable the move constructor/operator.  I think you would do for example:

ThrowScope(const ThrowScope&) = delete;
ThrowScope(ThrowScope&&) = default;

Clearly, ThrowScope is not meant to be copied, so this protects against that.  But it is meant to be created by moving, via that macro.  I believe this should disallow the bad thing (copying) but allow the good thing (your auto scope = DECLARE_blah idiom).

> Source/JavaScriptCore/runtime/ThrowScope.h:77
> +    JSC::ThrowScope(vm__, JSC::ThrowScopeLocation(__FUNCTION__, __FILE__, __LINE__))

Put extra () around vm__, like "(vm__)".  This makes this macro behave more like a function, in case there are for example commas in vm__.
Comment 15 Mark Lam 2016-08-30 10:01:47 PDT
Comment on attachment 287378 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/ThrowScope.cpp:297
>> +        willBeHandleByLLIntOrJIT = true;
> 
> Is it clear that it's better to have this here, than to have logic in vmEntry?

VMEntry (in the comment above) is a misnomer on my part.  I meant to communicate first ThrowScope after a JS frame on the stack.  "VMEntry" is the wrong idea to communicate this.  Rereading the comment, I think just removing the "VMEntry" would do the trick.

The scenario we need to guard against is where a JS function calls a C++ operation helper, and the helper has a ThrowScope.  We need to clear the needExceptionCheck flag before returning to the JS frame because the JS code may invoke another C++ helper (and fail verification on entry to that new ThrowScope).  There is no VMEntryScope involved here.

>> Source/JavaScriptCore/runtime/ThrowScope.h:42
>> +
> 
> Seems like you could delete the copy constructor/operator but enable the move constructor/operator.  I think you would do for example:
> 
> ThrowScope(const ThrowScope&) = delete;
> ThrowScope(ThrowScope&&) = default;
> 
> Clearly, ThrowScope is not meant to be copied, so this protects against that.  But it is meant to be created by moving, via that macro.  I believe this should disallow the bad thing (copying) but allow the good thing (your auto scope = DECLARE_blah idiom).

Great.  I will apply and test.

>> Source/JavaScriptCore/runtime/ThrowScope.h:77
>> +    JSC::ThrowScope(vm__, JSC::ThrowScopeLocation(__FUNCTION__, __FILE__, __LINE__))
> 
> Put extra () around vm__, like "(vm__)".  This makes this macro behave more like a function, in case there are for example commas in vm__.

Will do.
Comment 16 Mark Lam 2016-08-30 10:26:52 PDT
I'm now trying to figure out why the Win EWS is having a bad time.
Comment 17 Geoffrey Garen 2016-08-30 10:40:51 PDT
Comment on attachment 287378 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ThrowScope.cpp:296
> +    if (topCallFrame && previousScope > topCallFrame)

Since we don't clear VM::m_topCallFrame most of the time, topCallFrame will almost always be non-null, and we will get away with a lot of mistakes.
Comment 18 Geoffrey Garen 2016-08-30 10:48:53 PDT
Comment on attachment 287378 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/ThrowScope.cpp:296
>> +    if (topCallFrame && previousScope > topCallFrame)
> 
> Since we don't clear VM::m_topCallFrame most of the time, topCallFrame will almost always be non-null, and we will get away with a lot of mistakes.

...Actually, even though we leave stale values for VM::topCallFrame during LLInt and JIT execution, once we exit, we restore null.
Comment 19 Geoffrey Garen 2016-08-30 11:06:19 PDT
I think you can fix the Windows build failure by using old-school header guards instead of #pragma once. Later, we should get DumpRenderTree.cpp not to include JavaScriptCore/runtime headers. It doesn't need to, since we have typed array API now.

Phil, any further comments?
Comment 20 Mark Lam 2016-08-30 11:12:25 PDT
Created attachment 287410 [details]
revised patch w/ Fil's feedback + old header guard applied.
Comment 21 Mark Lam 2016-08-30 13:57:33 PDT
Thanks for the reviews.  Landed in r205198: <http://trac.webkit.org/r205198>.