Bug 161171 – Introduce the ThrowScope and force every throw site to instantiate a ThrowScope.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This is the first step towards simulating potential JS exceptions throws, and verifying that they are checked for appropriately in C++ code.

[Mark Lam]

Created attachment 286989 [details]
work in progress

[Geoffrey Garen]

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.

[Mark Lam]

Created attachment 287336 [details]
x86_64 benchmark result run 1

[Mark Lam]

Created attachment 287337 [details]
x86_64 benchmark result run 2

[Mark Lam]

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.

[Mark Lam]

Created attachment 287341 [details]
proposed patch.

[Mark Lam]

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.

[Mark Lam]

Created attachment 287349 [details]
proposed patch.

[Build Bot]

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.

[Build Bot]

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

[Mark Lam]

Comment on attachment 287349 [details]
proposed patch.

I had some silly bugs that resulted in the failures.  Will fix first.

[Mark Lam]

Created attachment 287375 [details]
proposed patch.

[Mark Lam]

Created attachment 287378 [details]
proposed patch.

[Filip Pizlo]

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__.

[Mark Lam]

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.

[Mark Lam]

I'm now trying to figure out why the Win EWS is having a bad time.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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?

[Mark Lam]

Created attachment 287410 [details]
revised patch w/ Fil's feedback + old header guard applied.

[Mark Lam]

Thanks for the reviews.  Landed in r205198: <http://trac.webkit.org/r205198>.