Proxies of an object which has a programmatic getter call the getter erroneously
Reported by
thunderc...@theflux.co.uk,
Oct 19 2017
|
|||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.62 Safari/537.36 Steps to reproduce the problem: See test case at https://jsbin.com/toluhoj/2/edit?js,console What is the expected behavior? The proxy should trap the call to get `value` and return the value 'yep' without calling the target's underlying getter for `value`. What went wrong? The value 'yep' is returned as expected, however the target's getter for 'value' is also called, which makes it a leaky proxy as the value was not completely trapped. Did this work before? Yes 61 Does this work in other browsers? Yes Chrome version: 62.0.3202.62 Channel: stable OS Version: OS X 10.12.6 Flash Version: This works as expected on other tested browsers (Safari and Firefox)
,
Oct 19 2017
Bisect info: 492151 (good) - 492167 (bad) https://chromium.googlesource.com/chromium/src/+log/4e90ca7e..c69cf190?pretty=fuller Suspecting r492162 "Update V8 to version 6.2.146" Landed in 62.0.3177.0 In V8 log suspecting 15ef03cbf3a439a99966a6e718e99f0617a9f604 "Reland "[builtins] Port getting property from Proxy to CSA"
,
Oct 20 2017
Able to reproduce this issue on reported version 62.0.3202.62 and Latest Canary 64.0.3242.0 using Mac 10.12.6, Ubuntu 14.04 and Win 10 and as per the bisect in C#1, assigning to jgruber @mslekova: Please confirm the bug and help in re-assigning if it is not related to your change. note: unable to assign to mslekova@, hence assigning to jgruber@ as one of the reviewers of the CL. Thanks!
,
Oct 20 2017
This is still on ToT, I'll have a look. The repro from #0 is:
const obj = {};
Object.defineProperty(obj, 'value', {
enumerable: true,
configurable: true,
get: function() { console.log("Shouldn't be seeing this"); }
});
const get = function(target, prop) {
if (prop === 'value') {
return 'yep';
}
return Reflect.get(target, prop)
}
const proxy = new Proxy(obj, { get });
console.log("Value is", proxy.value);
Removing ReleaseBlock-Stable since this is limited to Proxies. +cc hablich@, bmeurer@ fyi.
,
Oct 20 2017
Fix in-flight: https://chromium-review.googlesource.com/c/v8/v8/+/731123
,
Oct 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/14165a47d474fe2981bdb8e44ed1c9da4dd6937f commit 14165a47d474fe2981bdb8e44ed1c9da4dd6937f Author: jgruber <jgruber@chromium.org> Date: Mon Oct 23 07:48:22 2017 [proxy] Fix invalid call to getter in [[Get/Set]] Fixes the implementation of step 9 in the Proxy's internal [[Get]] method: Let targetDesc be ? target.[[GetOwnProperty]](P) If P is an accessor, this should not result in a call to the getter. Likewise in [[Set]]. https://tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver Bug: chromium:776338 Change-Id: Ic06b7eeac6a1ef9606ddda6fa9d6d58b709702fb Reviewed-on: https://chromium-review.googlesource.com/731123 Reviewed-by: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#48813} [modify] https://crrev.com/14165a47d474fe2981bdb8e44ed1c9da4dd6937f/src/builtins/builtins-proxy-gen.cc [add] https://crrev.com/14165a47d474fe2981bdb8e44ed1c9da4dd6937f/test/mjsunit/regress/regress-776338.js
,
Oct 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b4c832eba08104d58c47d695793e395fb1f51810 commit b4c832eba08104d58c47d695793e395fb1f51810 Author: Jakob Gruber <jgruber@chromium.org> Date: Mon Oct 23 08:19:17 2017 Revert "[proxy] Fix invalid call to getter in [[Get/Set]]" This reverts commit 14165a47d474fe2981bdb8e44ed1c9da4dd6937f. Reason for revert: Fix is incomplete, will reland soon. Original change's description: > [proxy] Fix invalid call to getter in [[Get/Set]] > > Fixes the implementation of step 9 in the Proxy's internal [[Get]] > method: > > Let targetDesc be ? target.[[GetOwnProperty]](P) > > If P is an accessor, this should not result in a call to the getter. > Likewise in [[Set]]. > > https://tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver > > Bug: chromium:776338 > Change-Id: Ic06b7eeac6a1ef9606ddda6fa9d6d58b709702fb > Reviewed-on: https://chromium-review.googlesource.com/731123 > Reviewed-by: Georg Neis <neis@chromium.org> > Commit-Queue: Jakob Gruber <jgruber@chromium.org> > Cr-Commit-Position: refs/heads/master@{#48813} TBR=neis@chromium.org,jgruber@chromium.org Change-Id: I92a11791b3c6a73ada1f72fe4193c25e7a054746 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:776338 Reviewed-on: https://chromium-review.googlesource.com/732877 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#48815} [modify] https://crrev.com/b4c832eba08104d58c47d695793e395fb1f51810/src/builtins/builtins-proxy-gen.cc [delete] https://crrev.com/432b2ddcced7f749248dca564144116d74578a86/test/mjsunit/regress/regress-776338.js
,
Oct 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/db09c2a60bb4a60300926c5acd6c1310cd9302e5 commit db09c2a60bb4a60300926c5acd6c1310cd9302e5 Author: jgruber <jgruber@chromium.org> Date: Mon Oct 23 11:21:26 2017 [proxy] Fix invalid call to getter in [[Get/Set/Has]] Fixes the implementation of step 9 in the Proxy's internal [[Get]] method: Let targetDesc be ? target.[[GetOwnProperty]](P) If P is an accessor, this should not result in a call to the getter. Likewise in [[Set]] and [[Has]]. https://tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver Bug: chromium:776338 Change-Id: I2652ffab2b3e4c38de00a82b8419192fdc768951 Reviewed-on: https://chromium-review.googlesource.com/732897 Reviewed-by: Georg Neis <neis@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#48825} [modify] https://crrev.com/db09c2a60bb4a60300926c5acd6c1310cd9302e5/src/builtins/builtins-proxy-gen.cc [modify] https://crrev.com/db09c2a60bb4a60300926c5acd6c1310cd9302e5/src/code-stub-assembler.cc [modify] https://crrev.com/db09c2a60bb4a60300926c5acd6c1310cd9302e5/src/code-stub-assembler.h [add] https://crrev.com/db09c2a60bb4a60300926c5acd6c1310cd9302e5/test/mjsunit/regress/regress-776338.js
,
Oct 23 2017
Fixed in #8. The bug in [[Has]] was introduced in 62. Bugs in [[Get]] and [[Set]] were introduced in 63.
,
Oct 23 2017
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 23 2017
Thanks for the fix jgruber@! We're already at M62 Stable rollout. Can you please confirm why this bug is critical and should be considered for our next respin? What are the consequences if we wait until M63? Which CL needs to be merged for M62?
,
Oct 23 2017
#8 fixes a correctness bug in JS Proxy methods, first introduced in M62. My intuition is that impact of the bug should be limited; I'm not sure what stable backmerge guidelines are in this case. For M62 the CL in #8 would to be manually adapted. For M63, #8 can hopefully be applied with minimal changes.
,
Oct 24 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 24 2017
+hablich@ for M63 merge review.
,
Oct 24 2017
+hablich@ for M62 review as well. My recommendation is to wait until M63, if the impact is limited.
,
Oct 25 2017
,
Oct 25 2017
As this is a correctness regression, this should be merged to both.
,
Oct 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2bba392a5b252a1b37f4bbe065c754e6613837ba commit 2bba392a5b252a1b37f4bbe065c754e6613837ba Author: jgruber <jgruber@chromium.org> Date: Wed Oct 25 09:12:10 2017 Merged: [proxy] Fix invalid call to getter in [[Get/Set/Has]] Fixes the implementation of step 9 in the Proxy's internal [[Get]] method: Let targetDesc be ? target.[[GetOwnProperty]](P) If P is an accessor, this should not result in a call to the getter. Likewise in [[Set]] and [[Has]]. https://tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: chromium:776338 Change-Id: If265572fe727fa0fd431a3dadc1bbad497cef921 Reviewed-on: https://chromium-review.googlesource.com/737792 Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/branch-heads/6.2@{#74} Cr-Branched-From: efa2ac4129d30c7c72e84c16af3d20b44829f990-refs/heads/6.2.414@{#1} Cr-Branched-From: a861ebb762a60bf5cc2a274faee3620abfb06311-refs/heads/master@{#47693} [modify] https://crrev.com/2bba392a5b252a1b37f4bbe065c754e6613837ba/src/builtins/builtins-proxy-gen.cc [modify] https://crrev.com/2bba392a5b252a1b37f4bbe065c754e6613837ba/src/builtins/builtins-proxy-helpers-gen.cc [modify] https://crrev.com/2bba392a5b252a1b37f4bbe065c754e6613837ba/src/code-stub-assembler.cc [modify] https://crrev.com/2bba392a5b252a1b37f4bbe065c754e6613837ba/src/code-stub-assembler.h [add] https://crrev.com/2bba392a5b252a1b37f4bbe065c754e6613837ba/test/mjsunit/regress/regress-776338.js
,
Oct 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c676d2cd1d9587e460733125445af5957ca50da0 commit c676d2cd1d9587e460733125445af5957ca50da0 Author: jgruber <jgruber@chromium.org> Date: Wed Oct 25 09:13:16 2017 Merged: [proxy] Fix invalid call to getter in [[Get/Set/Has]] Fixes the implementation of step 9 in the Proxy's internal [[Get]] method: Let targetDesc be ? target.[[GetOwnProperty]](P) If P is an accessor, this should not result in a call to the getter. Likewise in [[Set]] and [[Has]]. https://tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: chromium:776338 Change-Id: I7a6a8e8ec25c2340f1a1878de8092555ae90598e Reviewed-on: https://chromium-review.googlesource.com/737872 Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Michael Hablich <hablich@chromium.org> Cr-Commit-Position: refs/branch-heads/6.3@{#38} Cr-Branched-From: 094a7c93dcdcd921de3883ba4674b7e1a0feffbe-refs/heads/6.3.292@{#1} Cr-Branched-From: 18b8fbb528a8021e04a029e06eafee50b918bce0-refs/heads/master@{#48432} [modify] https://crrev.com/c676d2cd1d9587e460733125445af5957ca50da0/src/builtins/builtins-proxy-gen.cc [modify] https://crrev.com/c676d2cd1d9587e460733125445af5957ca50da0/src/code-stub-assembler.cc [modify] https://crrev.com/c676d2cd1d9587e460733125445af5957ca50da0/src/code-stub-assembler.h [add] https://crrev.com/c676d2cd1d9587e460733125445af5957ca50da0/test/mjsunit/regress/regress-776338.js
,
Oct 25 2017
This was merged. Removing Merge-Approved label.
,
Oct 25 2017
Also merged to 63.
,
Nov 27 2017
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by manoranj...@chromium.org
, Oct 19 2017