Bug 164849 - Proxy's [[Get]] passes incorrect receiver
Summary: Proxy's [[Get]] passes incorrect receiver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 171915 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-11-16 17:55 PST by Alexey Shvayka
Modified: 2017-05-30 12:18 PDT (History)
12 users (show)

See Also:


Attachments
patch (5.70 KB, patch)
2017-05-18 17:35 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (5.61 KB, patch)
2017-05-18 17:36 PDT, Saam Barati
ysuzuki: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for landing (9.05 KB, patch)
2017-05-18 18:23 PDT, Saam Barati
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 2016-11-16 17:55:30 PST
Please consider the following code:

```
var target =
{
    get prop()
    {
        console.log(this == proxy) // => `false`, should be `true`
    }
}

var proxy = new Proxy(target, {})

proxy.prop
```

`proxy.prop` calls `[[Get]]` on `proxy` with `"prop"` as key and `proxy` as receiver.
Proxy's `[[Get]]` method checks for `get` trap, it is missing, so it should call `[[Get]]` on `target` with **the same** parameters.
However, JSC does not pass receiver, thus `prop` getter is called with context of `target`, not `proxy`.

Both V8 and SpiderMonkey implement this correctly.
tc39/test-262 PR: https://github.com/tc39/test262/pull/792
chai/chaijs issue: https://github.com/chaijs/chai/issues/855
Comment 1 Radar WebKit Bug Importer 2017-04-21 15:07:13 PDT
<rdar://problem/31767058>
Comment 2 Saam Barati 2017-05-18 17:35:04 PDT
Created attachment 310579 [details]
patch
Comment 3 Saam Barati 2017-05-18 17:36:16 PDT
Comment on attachment 310579 [details]
patch

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

> JSTests/stress/proxy-get-set-correct-receiver.js:21
> +            assert(this === proxy) // => `false`, should be `true`

oops, let me remove these comments.
Comment 4 Saam Barati 2017-05-18 17:36:49 PDT
Created attachment 310580 [details]
patch
Comment 5 Yusuke Suzuki 2017-05-18 17:41:07 PDT
Comment on attachment 310580 [details]
patch

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

r=me

> JSTests/stress/proxy-get-set-correct-receiver.js:36
> +            assert(this === proxy)

OK, receiver is proxy.

> JSTests/stress/proxy-get-set-correct-receiver.js:50
> +            assert(this === proxy)

OK, receiver is neither p1 nor target.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:135
> +        return jsUndefined();

OK, previously, we ignored receiver.
Comment 6 Build Bot 2017-05-18 18:15:47 PDT
Comment on attachment 310580 [details]
patch

Attachment 310580 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3772712

New failing tests:
stress/proxy-set.js.ftl-no-cjit-small-pool
stress/reflect-set-proxy-set.js.ftl-no-cjit-no-put-stack-validate
stress/reflect-set-receiver-proxy-set.js.default
stress/reflect-set-receiver-proxy-set.js.no-llint
stress/reflect-set-receiver-proxy-set.js.ftl-no-cjit-validate-sampling-profiler
stress/reflect-set-proxy-set.js.ftl-no-cjit-b3o1
stress/reflect-set-receiver-proxy-set.js.ftl-eager
stress/reflect-set-proxy-set.js.ftl-no-cjit-small-pool
stress/reflect-set-receiver-proxy-set.js.ftl-eager-no-cjit-b3o1
stress/reflect-set-proxy-set.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-set.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-set.js.ftl-eager-no-cjit-b3o1
stress/reflect-set-proxy-set.js.ftl-eager-no-cjit
stress/reflect-set-receiver-proxy-set.js.ftl-eager-no-cjit
stress/proxy-set.js.ftl-eager
stress/reflect-set-proxy-set.js.ftl-no-cjit-no-inline-validate
stress/proxy-set.js.dfg-eager
stress/reflect-set-proxy-set.js.dfg-eager
stress/proxy-set.js.ftl-no-cjit-no-inline-validate
stress/proxy-set.js.ftl-no-cjit-b3o1
stress/proxy-set.js.no-llint
stress/reflect-set-proxy-set.js.ftl-eager
stress/reflect-set-proxy-set.js.dfg-eager-no-cjit-validate
stress/proxy-set.js.default
stress/proxy-set.js.no-cjit-validate-phases
stress/reflect-set-proxy-set.js.no-cjit-validate-phases
stress/reflect-set-receiver-proxy-set.js.ftl-no-cjit-no-put-stack-validate
stress/reflect-set-receiver-proxy-set.js.dfg-eager-no-cjit-validate
stress/reflect-set-receiver-proxy-set.js.dfg-eager
stress/reflect-set-receiver-proxy-set.js.ftl-no-cjit-no-inline-validate
stress/proxy-set.js.ftl-eager-no-cjit
stress/reflect-set-proxy-set.js.default
stress/proxy-set.js.dfg-eager-no-cjit-validate
stress/reflect-set-proxy-set.js.no-llint
stress/proxy-set.js.ftl-no-cjit-no-put-stack-validate
stress/reflect-set-receiver-proxy-set.js.no-cjit-validate-phases
stress/reflect-set-proxy-set.js.no-cjit-collect-continuously
stress/proxy-set.js.no-ftl
stress/reflect-set-proxy-set.js.dfg-maximal-flush-validate-no-cjit
stress/reflect-set-proxy-set.js.no-ftl
stress/reflect-set-receiver-proxy-set.js.no-ftl
stress/reflect-set-receiver-proxy-set.js.ftl-no-cjit-b3o1
stress/proxy-set.js.dfg-maximal-flush-validate-no-cjit
stress/reflect-set-receiver-proxy-set.js.ftl-no-cjit-small-pool
stress/reflect-set-receiver-proxy-set.js.dfg-maximal-flush-validate-no-cjit
stress/reflect-set-proxy-set.js.ftl-eager-no-cjit-b3o1
stress/proxy-set.js.no-cjit-collect-continuously
stress/reflect-set-receiver-proxy-set.js.no-cjit-collect-continuously
Comment 7 Saam Barati 2017-05-18 18:23:45 PDT
Created attachment 310587 [details]
patch for landing
Comment 8 WebKit Commit Bot 2017-05-18 20:51:17 PDT
Comment on attachment 310587 [details]
patch for landing

Clearing flags on attachment: 310587

Committed r217093: <http://trac.webkit.org/changeset/217093>
Comment 9 WebKit Commit Bot 2017-05-18 20:51:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 GSkachkov 2017-05-30 12:16:26 PDT
*** Bug 169040 has been marked as a duplicate of this bug. ***
Comment 11 GSkachkov 2017-05-30 12:18:10 PDT
*** Bug 171915 has been marked as a duplicate of this bug. ***