Bug 214624 – Disallow VM entry when doing a VMInquiry.

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

[Mark Lam]

We can now use the DisallowVMEntry scope to enforce this.

[Radar WebKit Bug Importer]

<rdar://problem/65915314>

[Mark Lam]

Created attachment 404892 [details]
proposed patch.

[Mark Lam]

Created attachment 404901 [details]
proposed patch.

[Mark Lam]

Created attachment 404902 [details]
proposed patch.

[Saam Barati]

Comment on attachment 404902 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:27
> +           do entail entering the VM.  In such cases, we need to reset the PropertySlot's

do entail => entails

> Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.cpp:310
> +        PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry, &lexicalGlobalObject->vm() };
> +        bool found = JSObject::getOwnPropertySlot(thisObject, lexicalGlobalObject, propertyName, slot);
> +        slot.disallowVMEntry.reset();

for a lot of these that just require the bool result of calling getOwnPropertySlot, if you just passed the slot in as a constructed thing in the function call, you wouldn't need the reset dance, since it's destroyed post call. 

(I haven't vetted all call sites, but a lot could do this, and would be cleaner for it)

[Mark Lam]

Comment on attachment 404902 [details]
proposed patch.

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

>> Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.cpp:310
>> +        slot.disallowVMEntry.reset();
> 
> for a lot of these that just require the bool result of calling getOwnPropertySlot, if you just passed the slot in as a constructed thing in the function call, you wouldn't need the reset dance, since it's destroyed post call. 
> 
> (I haven't vetted all call sites, but a lot could do this, and would be cleaner for it)

I'll add a helper in JSObject to handle this.

[Mark Lam]

Created attachment 404984 [details]
patch for landing.

[Mark Lam]

Comment on attachment 404984 [details]
patch for landing.

After thinking about this more, I'm going to opt for not adding the helper functions.  The reason is because it is not possible to use the helpers in all places.  For example,
1. some places needs to use the PropertySlot after the call to "get".
2. some places want to call "get" via the methodTable.  Hence, the helpers will not be helpful there as they only dispatch to the JSObject methods.

Adopting the helpers will therefore result in having more than one way to do VMInquiries, and can make the code more confusing.  So, I'm going to forego the helpers in the interest of keeping the interface consistent.

Instantiating the PropertySlot directly as the argument for the "get" function is also not an option.  This is because this "get" functions require a PropertySlot&.  C++ does not let us pass a reference to this unnamed instantiated PropertySlot.

I'll land the original patch with the ChangeLog typo fix.  Thanks.

[Mark Lam]

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

[Michael Catanzaro]

GCC found a problem: bug #214809