Bug 203560 – [[HasProperty]] result of Proxy in prototype chain is ignored

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

When [[HasProperty]] is called on ordinary object with Proxy in prototype chain that implements "has" trap,
falsy result of the trap is ignored and prototype chain gets inspected further.

Test case:
  "prop" in Object.create(
    new Proxy(
      Object.create({ prop: 42 }),
      { has: () => false }
    )
  )

Expected:
  false

Actual:
  true

ECMA262:
https://tc39.es/ecma262/#sec-ordinaryhasproperty (step 5.a)

Test262:
https://test262.report/browse/built-ins/Proxy/has/call-in-prototype.js (non-index)
https://test262.report/browse/built-ins/Array/prototype/indexOf/calls-only-has-on-prototype-after-length-zeroed.js (index)
https://test262.report/browse/built-ins/Array/prototype/lastIndexOf/calls-only-has-on-prototype-after-length-zeroed.js (index)

[Alexey Shvayka]

Created attachment 382177 [details]
Patch

[EWS Watchlist]

Comment on attachment 382177 [details]
Patch

Attachment 382177 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13188171

New failing tests:
stress/proxy-get-own-property.js.ftl-eager-no-cjit
stress/proxy-get-prototype-of.js.no-cjit-validate-phases
stress/for-in-side-effects.js.mini-mode
stress/for-in-side-effects.js.dfg-eager
stress/proxy-get-own-property.js.ftl-no-cjit-no-inline-validate
stress/proxy-get-prototype-of.js.ftl-eager
stress/proxy-get-own-property.js.no-cjit-validate-phases
stress/proxy-get-own-property.js.dfg-eager
stress/proxy-get-prototype-of.js.ftl-no-cjit-b3o0
stress/for-in-side-effects.js.bytecode-cache
stress/proxy-get-own-property.js.no-cjit-collect-continuously
stress/for-in-side-effects.js.dfg-eager-no-cjit-validate
stress/proxy-get-own-property.js.ftl-eager
stress/proxy-get-prototype-of.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-get-prototype-of.js.ftl-eager-no-cjit
stress/proxy-get-prototype-of.js.ftl-no-cjit-small-pool
stress/for-in-side-effects.js.ftl-no-cjit-b3o0
stress/proxy-get-own-property.js.ftl-no-cjit-no-put-stack-validate
stress/for-in-side-effects.js.no-ftl
stress/proxy-get-prototype-of.js.eager-jettison-no-cjit
stress/proxy-get-prototype-of.js.no-llint
stress/proxy-get-prototype-of.js.bytecode-cache
stress/proxy-get-own-property.js.no-ftl
stress/proxy-get-own-property.js.default
stress/proxy-get-own-property.js.no-llint
stress/proxy-get-prototype-of.js.ftl-no-cjit-no-put-stack-validate
stress/for-in-side-effects.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-side-effects.js.ftl-no-cjit-no-put-stack-validate
stress/proxy-get-prototype-of.js.ftl-no-cjit-no-inline-validate
stress/proxy-get-own-property.js.bytecode-cache
stress/for-in-side-effects.js.no-cjit-collect-continuously
stress/proxy-get-prototype-of.js.dfg-eager-no-cjit-validate
stress/for-in-side-effects.js.no-cjit-validate-phases
stress/proxy-get-own-property.js.ftl-eager-no-cjit-b3o1
stress/for-in-side-effects.js.ftl-eager-no-cjit
stress/proxy-get-prototype-of.js.ftl-eager-no-cjit-b3o1
stress/proxy-get-own-property.js.eager-jettison-no-cjit
stress/for-in-side-effects.js.ftl-eager-no-cjit-b3o1
stress/proxy-get-prototype-of.js.mini-mode
stress/proxy-get-prototype-of.js.no-ftl
stress/for-in-side-effects.js.default
ChakraCore.yaml/ChakraCore/test/es7/lookupgettersetter.js.default
stress/for-in-side-effects.js.ftl-no-cjit-small-pool
stress/proxy-get-own-property.js.dfg-eager-no-cjit-validate
stress/proxy-get-prototype-of.js.dfg-eager
stress/proxy-get-prototype-of.js.no-cjit-collect-continuously
stress/for-in-side-effects.js.eager-jettison-no-cjit
stress/for-in-side-effects.js.no-llint
stress/proxy-get-own-property.js.ftl-no-cjit-b3o0
stress/proxy-get-own-property.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-side-effects.js.ftl-eager
stress/proxy-get-own-property.js.ftl-no-cjit-small-pool
stress/proxy-get-own-property.js.mini-mode
stress/for-in-side-effects.js.ftl-no-cjit-no-inline-validate
stress/proxy-get-prototype-of.js.default

[Alexey Shvayka]

Created attachment 382191 [details]
Patch

Add internalMethodType() check and adjust tests.

[Ross Kirsling]

Comment on attachment 382191 [details]
Patch

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

r=me with a comment

> Source/JavaScriptCore/runtime/JSObjectInlines.h:130
> +            return hasSlot;

Here and below, it seems like we could just return false instead?

[Alexey Shvayka]

Created attachment 382491 [details]
Patch

Set reviewer and return false.

[Ross Kirsling]

Do you have commit access set up now, Alexey? Otherwise I can cq+ if it's still in progress.

[Alexey Shvayka]

(In reply to Ross Kirsling from comment #6)
> Do you have commit access set up now, Alexey? Otherwise I can cq+ if it's
> still in progress.

Please do that, Ross. EWS failures seem irrelevant.
I will be all set when Committer Agreement will be delivered to Apple and signed, which will take ~2 weeks.

[WebKit Commit Bot]

Comment on attachment 382491 [details]
Patch

Clearing flags on attachment: 382491

Committed r251940: <https://trac.webkit.org/changeset/251940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56828447>

[Ross Kirsling]

Hmm, looks like this patch caused 6 test262 failures:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/6860

But they're all existing failures with updated results:
  test/built-ins/Array/prototype/reverse/length-exceeding-integer-limit-with-proxy.js
  test/built-ins/Array/prototype/splice/create-species-length-exceeding-integer-limit.js
  test/built-ins/Proxy/has/trap-is-undefined.js

So I think we can commit an expectations.yaml update instead of rolling out.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #11)
> So I think we can commit an expectations.yaml update instead of rolling out.

Er wait, no, test/built-ins/Proxy/has/trap-is-undefined.js is a totally new failure. I guess rollout is unavoidable.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 203920

[Alexey Shvayka]

Created attachment 383041 [details]
Patch

Fix performDefaultHasProperty and adjust expectations.

[WebKit Commit Bot]

Comment on attachment 383041 [details]
Patch

Clearing flags on attachment: 383041

Committed r252191: <https://trac.webkit.org/changeset/252191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 383041 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObjectInlines.h:129
> +        if (object->type() == ProxyObjectType && slot.internalMethodType() == PropertySlot::InternalMethodType::HasProperty)

What about when it’s not in the prototype chain though? What if the proxy is the base?

[Alexey Shvayka]

(In reply to Saam Barati from comment #17)
> Comment on attachment 383041 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383041&action=review
> 
> > Source/JavaScriptCore/runtime/JSObjectInlines.h:129
> > +        if (object->type() == ProxyObjectType && slot.internalMethodType() == PropertySlot::InternalMethodType::HasProperty)
> 
> What about when it’s not in the prototype chain though? What if the proxy is
> the base?

This case is handled by `JSObject* object = this;` on lines 121 and 152 (before loops), isn't it?