Bug 200560

Summary: getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Minor CC: ashvayka, barraclough, cdumez, ews-watchlist, joepeck, keith_miller, ljharb, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://rawgit.com/tvcutsem/es-lab/master/src/ses/contract.html
See Also: https://bugs.webkit.org/show_bug.cgi?id=134420
https://bugs.webkit.org/show_bug.cgi?id=154378
https://bugs.webkit.org/show_bug.cgi?id=154314
Attachments:
Description Flags
WIP Patch
none
Patch none

Yusuke Suzuki
Reported 2019-08-08 21:33:14 PDT
We have a hack in JSObject::getOwnPropertyDescriptor for DebuggerScope and ProxyObject. But this is wrong since, 1. This hack breaks the assumption that JSObject::getOwnPropertySlot should return own property 2. This hack is only added for JSObject::getOwnPropertyDescriptor. So, the other clients using JSObject::getOwnPropertySlot see the different results
Attachments
WIP Patch (6.77 KB, patch)
2020-04-07 10:34 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (13.50 KB, patch)
2020-04-08 16:32 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Shvayka
Comment 1 2020-04-07 10:34:03 PDT
Created attachment 395703 [details] WIP Patch
Yusuke Suzuki
Comment 2 2020-04-07 11:49:34 PDT
Comment on attachment 395703 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395703&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:-3511 > - Can you explain, 1. Why this hack is originally added? 2. Why is it OK to remove this hack without changing any other part?
Alexey Shvayka
Comment 3 2020-04-08 16:32:15 PDT
(In reply to Yusuke Suzuki from comment #2) > Can you explain, > > 1. Why this hack is originally added? > 2. Why is it OK to remove this hack without changing any other part? 1. This hack was added so that CustomValue path, which must perform getDirect(), worked with JSProxy and didn't crash on ProxyObject and DebuggerScope instances. In fact, there are two hacks: for JSProxy and for ProxyObject. JSProxy hack was removed in r196676, yet brought back in r196760 since Web Inspector was failing ASSERT(maybeGetterSetter) in CustomValue path. Steps to reproduce: 1. Tools/Scripts/run-minibrowser --debug 2. Open Web Inspector 3. Navigate to https://rawgit.com/tvcutsem/es-lab/master/src/ses/contract.html 4. Wait for breakpoint 5. Expand "Global Variables" on the right, in "Scope Chain" panel As of r259682 and with this hack removed, these steps still lead to a crash. ProxyObject hack was introduced in r196775, along with another fix, but I didn't manage to find a test case nor an explanation. It also lacks `thisObject` changing bit like JSProxy counterpart has: if (auto* proxy = jsDynamicCast<JSProxy*>(vm, this)) thisObject = proxy->target(); making it fail that very ASSERT if called on Proxy with host object target (except for Window): Object.getOwnPropertyDescriptor(new Proxy(document, {}), "location"); 2. Given that Web Inspector crashes, and for a good reason, we can't just remove the hack. To accomodate JSProxy and other objects that override getOwnPropertySlot(), I am using slotBase() as `thisObject` for getDirect() calls. getDirect() can be safely called on slotBase(): if getOwnPropertySlot() result is returned from JS code of ProxyObject's trap, it will never be a PropertyAttribute::CustomValue.
Alexey Shvayka
Comment 4 2020-04-08 16:32:31 PDT
Created attachment 395885 [details] Patch
EWS
Comment 5 2020-04-09 08:13:12 PDT
Committed r259800: <https://trac.webkit.org/changeset/259800> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395885 [details].
Radar WebKit Bug Importer
Comment 6 2020-04-09 08:14:13 PDT
<rdar://problem/61518448>
Alexey Shvayka
Comment 7 2020-08-30 02:16:35 PDT
*** Bug 170537 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.