Bug 203560 - [[HasProperty]] result of Proxy in prototype chain is ignored
Summary: [[HasProperty]] result of Proxy in prototype chain is ignored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 203920
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-29 05:06 PDT by Alexey Shvayka
Modified: 2019-12-02 13:53 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2019-10-29 05:24 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2019-10-29 10:11 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2019-10-31 13:09 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2019-11-07 05:49 PST, 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 Alexey Shvayka 2019-10-29 05:06:13 PDT
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)
Comment 1 Alexey Shvayka 2019-10-29 05:24:51 PDT
Created attachment 382177 [details]
Patch
Comment 2 EWS Watchlist 2019-10-29 07:49:19 PDT
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
Comment 3 Alexey Shvayka 2019-10-29 10:11:14 PDT
Created attachment 382191 [details]
Patch

Add internalMethodType() check and adjust tests.
Comment 4 Ross Kirsling 2019-10-30 16:27:13 PDT
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?
Comment 5 Alexey Shvayka 2019-10-31 13:09:40 PDT
Created attachment 382491 [details]
Patch

Set reviewer and return false.
Comment 6 Ross Kirsling 2019-10-31 15:00:28 PDT
Do you have commit access set up now, Alexey? Otherwise I can cq+ if it's still in progress.
Comment 7 Alexey Shvayka 2019-11-01 13:17:20 PDT
(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.
Comment 8 WebKit Commit Bot 2019-11-01 14:23:49 PDT
Comment on attachment 382491 [details]
Patch

Clearing flags on attachment: 382491

Committed r251940: <https://trac.webkit.org/changeset/251940>
Comment 9 WebKit Commit Bot 2019-11-01 14:23:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-11-01 14:24:16 PDT
<rdar://problem/56828447>
Comment 11 Ross Kirsling 2019-11-06 14:59:27 PST
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.
Comment 12 Ross Kirsling 2019-11-06 15:06:18 PST
(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.
Comment 13 WebKit Commit Bot 2019-11-06 15:12:57 PST
Re-opened since this is blocked by bug 203920
Comment 14 Alexey Shvayka 2019-11-07 05:49:49 PST
Created attachment 383041 [details]
Patch

Fix performDefaultHasProperty and adjust expectations.
Comment 15 WebKit Commit Bot 2019-11-07 10:31:38 PST
Comment on attachment 383041 [details]
Patch

Clearing flags on attachment: 383041

Committed r252191: <https://trac.webkit.org/changeset/252191>
Comment 16 WebKit Commit Bot 2019-11-07 10:31:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Saam Barati 2019-11-30 17:00:20 PST
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?
Comment 18 Alexey Shvayka 2019-12-02 13:53:28 PST
(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?