Bug 200560 - getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
Summary: getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL: https://rawgit.com/tvcutsem/es-lab/ma...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-08 21:33 PDT by Yusuke Suzuki
Modified: 2020-04-09 08:14 PDT (History)
12 users (show)

See Also:


Attachments
WIP Patch (6.77 KB, patch)
2020-04-07 10:34 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (13.50 KB, patch)
2020-04-08 16:32 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 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
Comment 1 Alexey Shvayka 2020-04-07 10:34:03 PDT
Created attachment 395703 [details]
WIP Patch
Comment 2 Yusuke Suzuki 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?
Comment 3 Alexey Shvayka 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.
Comment 4 Alexey Shvayka 2020-04-08 16:32:31 PDT
Created attachment 395885 [details]
Patch
Comment 5 EWS 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].
Comment 6 Radar WebKit Bug Importer 2020-04-09 08:14:13 PDT
<rdar://problem/61518448>