Summary: | Disallow VM entry when doing a VMInquiry. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, ews-watchlist, hi, joepeck, keith_miller, mcatanzaro, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 214539 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Mark Lam
2020-07-21 19:14:35 PDT
Created attachment 404892 [details]
proposed patch.
Created attachment 404901 [details]
proposed patch.
Created attachment 404902 [details]
proposed patch.
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) 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. Created attachment 404984 [details]
patch for landing.
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.
Thanks for the review. Landed in r264736: <http://trac.webkit.org/r264736>. GCC found a problem: bug #214809 |