getOwnPropertyDescriptor() is incorrect with Proxy of exotic object

[Yusuke Suzuki]

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

[Alexey Shvayka]

Created attachment 395703 [details]
WIP Patch

[Yusuke Suzuki]

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]

(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]

Created attachment 395885 [details]
Patch

[EWS]

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]

<rdar://problem/61518448>

[Alexey Shvayka]

*** Bug 170537 has been marked as a duplicate of this bug. ***