Bug 214539 – Simplify DisallowScope, DisallowGC, and DisallowVMReentry implementations.

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

[Mark Lam]

Also do the following:
1. Change DisallowScope and DisallowGC to be based on ASSERT_ENABLED instead of NEBUG.
2. Make DisallowVMReentry always enforceable, not just when ASSERT_ENABLED.
3. Add DisallowVMReentry scopes whenever we create a PropertySlot for VMInquiry.

[Radar WebKit Bug Importer]

<rdar://problem/65795729>

[Mark Lam]

Created attachment 404683 [details]
proposed patch.

[Mark Lam]

Created attachment 404684 [details]
proposed patch.

[Mark Lam]

Created attachment 404686 [details]
proposed patch.

[Mark Lam]

Created attachment 404693 [details]
proposed patch.

Changed private name fix after consulting with Caitlin.

[Mark Lam]

Comment on attachment 404693 [details]
proposed patch.

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:295
> +    move a0, vm
> +    move a1, protoCallFrame

Found a bug: these arguments are reversed.  Will upload a fixed patch shortly.

[Mark Lam]

Created attachment 404694 [details]
proposed patch.

[Mark Lam]

Created attachment 404696 [details]
proposed patch.

[Mark Lam]

The mac-wk2 EWS appears to be unrelated to this patch.

[Mark Lam]

Comment on attachment 404696 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/PropertySlot.h:120
> +        ASSERT_UNUSED(disallowVMEntry, (internalMethodType == InternalMethodType::VMInquiry) == !!disallowVMEntry);

I should also assert that disallowVMEntry.isEngaged().  Will need to add the isEngaged() method.  Will upload another patch for this shortly.

[Mark Lam]

Created attachment 404779 [details]
proposed patch.

[Saam Barati]

Comment on attachment 404779 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:34
> +        2. Enforcement of DisallowVMReentry is now done in the LLInt's doVMEntry() instead

is this really needed? The assumption is we're already in C++ code if we're creating such an RAII, so we'd need to cross C++ to doVMEntry. Maybe just do it where we call vmEntryToJavaScript?

> Source/JavaScriptCore/ChangeLog:48
> +        5. Add DisallowVMEntry scopes whenever we create a PropertySlot for VMInquiry.
> +           The PropertySlot constructor will now take an optional DisallowVMEntry pointer.
> +           If the slot's specified internal method type is VMInquiry, then we'll
> +           RELEASE_ASSERT that the DisallowVMEntry pointer is non-null.

Why not just make the slot have an optional DisallowVMEntry that it creates on VMInquiry?

> Source/JavaScriptCore/ChangeLog:55
> +        6. Fixed a bug in llint_slow_path_get_private_name().  It should be passing
> +           GetOwnProperty to JSObject::getPrivateField() instead of VMInquiry.
> +           JSObject::getPrivateField() can throw an exception, which makes it not
> +           appropriate for a VMInquiry.  There is also no current client that needs it
> +           to handle a VMInquiry.  Added an assertion in JSObject::getPrivateField() to
> +           verify that the client will not be requesting a VMInquiry.

this seems fine, but just saying it's because it can't throw doesn't feel totally complete

[Saam Barati]

Comment on attachment 404779 [details]
proposed patch.

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

still reviewing, some initial comments

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:680
> +        disallowVMEntry.release();

not needed

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:654
> +        disallowVMEntry.release();

this shouldn't be needed

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:174
> +    loadi VM::disallowVMEntryCount[vm], t4
> +    btinz t4, .checkVMEntryPermission

Ignore prior comment on checking this in JITCode::execute. This seems fine

[Mark Lam]

Comment on attachment 404779 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:34
>> +        2. Enforcement of DisallowVMReentry is now done in the LLInt's doVMEntry() instead
> 
> is this really needed? The assumption is we're already in C++ code if we're creating such an RAII, so we'd need to cross C++ to doVMEntry. Maybe just do it where we call vmEntryToJavaScript?

vmEntryToJavaScript itself is a label in LLInt asm.  But yes, there are places in C++ code that we currently always go thru when entering the VM.  It just feels like a stronger guarantee to me that we choke it at the real choke point, rather than an associated one.

>> Source/JavaScriptCore/ChangeLog:48
>> +           RELEASE_ASSERT that the DisallowVMEntry pointer is non-null.
> 
> Why not just make the slot have an optional DisallowVMEntry that it creates on VMInquiry?

That was my original plan.  However, there are places where the lifecycle of the PropertySlot doesn't match the range of code where we want to disallow VM entry.  I also think it is good to call out that we're disabling VM entry explicitly and because it's declared explicitly, it's clear why we sometimes limit its scope.  Let me what you think after you're seen the places where it is used.

>> Source/JavaScriptCore/ChangeLog:55
>> +           verify that the client will not be requesting a VMInquiry.
> 
> this seems fine, but just saying it's because it can't throw doesn't feel totally complete

I presume by this, you want me to point out that throwing is observable at the JS spec level and hence does not fit what we expect of behavior of VMInquiry?

[Saam Barati]

Comment on attachment 404779 [details]
proposed patch.

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

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1112
> +            bool found  = JSGlobalLexicalEnvironment::getOwnPropertySlot(globalLexicalEnvironment, globalObject, function->name(), slot);

style: extra space before =

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:641
> +    throwScope.assertNoException();

it seems possible we could OOM.

> Source/JavaScriptCore/runtime/DisallowVMEntry.h:37
> +template<class VMType = VM>

template <typename VMType = VM>

> Source/JavaScriptCore/runtime/JSFunction.cpp:571
> +            scope.assertNoException();

don't do this. Go back to the old code

> Source/WebCore/bindings/js/JSDOMAbstractOperations.h:63
> +    JSC::PropertySlot slot(&thisObject, JSC::PropertySlot::InternalMethodType::VMInquiry, &disallowVMEntry);

Why is this VMInquiry? Is this not defined in the spec?

> Source/WebCore/bindings/js/JSDOMAbstractOperations.h:104
> +    JSC::PropertySlot slot(&thisObject, JSC::PropertySlot::InternalMethodType::VMInquiry, &disallowVMEntry);

ditto

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:971
> +            push(@$outputArray, "            PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry, &disallowVMEntry);\n");

why is this VMInquiry?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1052
> +            push(@$outputArray, "        PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry, &disallowVMEntry);\n");

why is this VMInquiry?

[Mark Lam]

After talking with Saam offline about this patch, I think I will break it down into several patches (and bugs) and change some of the implementation as well.  I'll use this specific bug to only do the refactoring that the title says: "Simplify DisallowScope, DisallowGC, and DisallowVMReentry implementations".

[Mark Lam]

Created attachment 404864 [details]
proposed patch.

[Mark Lam]

Created attachment 404886 [details]
proposed patch.

[Mark Lam]

Created attachment 404887 [details]
proposed patch.

[Keith Miller]

Comment on attachment 404887 [details]
proposed patch.

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

r=me with nits.

> Source/JavaScriptCore/ChangeLog:28
> +           If Options::crashOnDisallowedVMEntry() is false, an attempt to call into the VM
> +           while disallowed will return immediately with an undefined result without
> +           invoking any script.

Is this better than throwing an Exception of some kind?

> Source/JavaScriptCore/runtime/DisallowScope.h:42
> +        auto count = T::scopeReentryCount();
> +        T::setScopeReentryCount(++count);

Why not make the API a ref and deref? Seems like you need two functions either way?

> Source/JavaScriptCore/runtime/DisallowVMEntry.h:35
> +    WTF_MAKE_NONCOPYABLE(DisallowVMReentry);

Nit: I don't think you need this because DisallowScope isn't copyable.

> Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:65
> +        m_disallowGC.reset();
> +        m_disallowVMEntry.reset();

Unrelated but its weird that this is called reset... I would have expected clear() but ok.

[Mark Lam]

Comment on attachment 404887 [details]
proposed patch.

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

Thanks for there review.

>> Source/JavaScriptCore/ChangeLog:28
>> +           invoking any script.
> 
> Is this better than throwing an Exception of some kind?

I'm planning to use this to disallow VM entry during VMInquiry later.  It's intentional for it to not throw.

>> Source/JavaScriptCore/runtime/DisallowScope.h:42
>> +        T::setScopeReentryCount(++count);
> 
> Why not make the API a ref and deref? Seems like you need two functions either way?

I supposed I could, but that would be one additional step beyond just refactoring this to be an RAII object and removing the enable/disable functionality.  I'll look into that in a separate patch.

>> Source/JavaScriptCore/runtime/DisallowVMEntry.h:35
>> +// need it.
> 
> Nit: I don't think you need this because DisallowScope isn't copyable.

I think this comment somehow got associated with the wrong line here, and I think you meant it to be about the WTF_MAKE_NONCOPYABLE below.  Anyway, DisallowVMEntryImpl no longer inherits from DisallowScope.  So, what DisallowScope does is irrelevant here.

[Mark Lam]

Landed in r264688: <http://trac.webkit.org/r264688>.

[Caio Lima]

Comment on attachment 404887 [details]
proposed patch.

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:331
> +    move 0, r1

We need to fix the order of value here. In current 32-bits little-endian, payload goes into `r0` and tag goes into `r1`. I'm not aware of any 32-bits architecture using LLInt and JIT that's big-endian.