Deoptimizing a derived constructor leaks the hole |
|||||||||||||
Issue description
Test case -
class A extends Object {
constructor(arg) {
super();
superclass_counter++;
if (superclass_counter === 3) {
return 1;
}
}
}
class B extends A {
constructor() {
let x = super(123);
if (superclass_counter === 3) {
print(x);
}
}
}
var superclass_counter = 0;
var observer = new Proxy(A, {
get(target, property, receiver) {
if (property === 'prototype') {
%DeoptimizeFunction(B);
}
return Reflect.get(target, property, receiver);
}
});
Reflect.construct(B, [], observer);
Reflect.construct(B, [], observer);
%OptimizeFunctionOnNextCall(B);
Reflect.construct(B, [], observer);
Output --
→ ./out.gn/x64.release/d8 --allow-natives-syntax leak.js
hole
leak.js:18: ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor
print(x);
^
ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor
at B (leak.js:18:7)
at leak.js:36:9
In the above test --
We inline the A's constructor into B. The inlining logic doesn't account for the fact that the derived constructor (A) could return a primitive, thus leaking the implicit receiver (which is the hole).
There's also another correctness bug where the incorrect ReferenceError is thrown. The correct error is "TypeError: Derived constructors may only return object or undefined". Since we've inlined A into B, the return from B is throwing the error, instead of the return from A.
,
Mar 30 2017
,
Mar 30 2017
Ups, leaving the restriction.
,
Mar 30 2017
We should really fix the contract for derived super constructor calls.
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c019e53cbb0e3249d28c2d88eca64bf73df8de56 commit c019e53cbb0e3249d28c2d88eca64bf73df8de56 Author: bmeurer <bmeurer@chromium.org> Date: Thu Mar 30 10:17:10 2017 [turbofan] Disable inlining of derived class constructors. The inlining logic doesn't account for the fact that the derived constructor could return a primitive, thus leaking the implicit receiver (which is the hole). R=jarin@chromium.org BUG= chromium:706642 Review-Url: https://codereview.chromium.org/2788603002 Cr-Commit-Position: refs/heads/master@{#44264} [modify] https://crrev.com/c019e53cbb0e3249d28c2d88eca64bf73df8de56/src/compiler/js-inlining.cc [add] https://crrev.com/c019e53cbb0e3249d28c2d88eca64bf73df8de56/test/mjsunit/regress/regress-crbug-706642.js
,
Mar 30 2017
,
Mar 30 2017
Let's wait for Canary coverage
,
Mar 30 2017
,
Mar 30 2017
Please add applicable OSs. Thanks!
,
Mar 30 2017
,
Mar 30 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78 commit 776d89f9ce6e043972c1e611aa8cfaf2feb1fb78 Author: bmeurer <bmeurer@chromium.org> Date: Fri Mar 31 06:01:01 2017 [es2015] Simplify contract between parser and stub for derived constructors. Rewrite returns in derived constructors to only replace undefined with this, and otherwise just return the value, and let the construct stub builtin throw an exception if the result is a primitive instead of a JSReceiver. R=yangguo@chromium.org TBR=marja@chromium.org BUG= chromium:706642 Review-Url: https://codereview.chromium.org/2788033002 Cr-Commit-Position: refs/heads/master@{#44288} [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/builtins/arm/builtins-arm.cc [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/builtins/arm64/builtins-arm64.cc [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/builtins/ia32/builtins-ia32.cc [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/builtins/mips/builtins-mips.cc [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/builtins/mips64/builtins-mips64.cc [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/builtins/x64/builtins-x64.cc [modify] https://crrev.com/776d89f9ce6e043972c1e611aa8cfaf2feb1fb78/src/parsing/parser.cc
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/889d205b523c19e34c75770ff045a4c65d530635 commit 889d205b523c19e34c75770ff045a4c65d530635 Author: bjaideep <bjaideep@ca.ibm.com> Date: Fri Mar 31 16:45:46 2017 PPC/s390: [es2015] Simplify contract between parser and stub for derived constructors. Port 776d89f9ce6e043972c1e611aa8cfaf2feb1fb78 Original Commit Message: Rewrite returns in derived constructors to only replace undefined with this, and otherwise just return the value, and let the construct stub builtin throw an exception if the result is a primitive instead of a JSReceiver. R=bmeurer@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com BUG= chromium:706642 LOG=N Review-Url: https://codereview.chromium.org/2786413003 Cr-Commit-Position: refs/heads/master@{#44315} [modify] https://crrev.com/889d205b523c19e34c75770ff045a4c65d530635/src/builtins/ppc/builtins-ppc.cc [modify] https://crrev.com/889d205b523c19e34c75770ff045a4c65d530635/src/builtins/s390/builtins-s390.cc
,
Apr 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/708debd56967a94ef7d5a13b6f77c9cc6762fae3 commit 708debd56967a94ef7d5a13b6f77c9cc6762fae3 Author: Benedikt Meurer <bmeurer@google.com> Date: Mon Apr 03 05:48:11 2017 Merged: [turbofan] Disable inlining of derived class constructors. Revision: c019e53cbb0e3249d28c2d88eca64bf73df8de56 BUG= chromium:706642 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Change-Id: I61e9d2decb269b33aefcc6e77d6c09bab3a2a994 Reviewed-on: https://chromium-review.googlesource.com/465828 Reviewed-by: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/branch-heads/5.8@{#49} Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1} Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429} [modify] https://crrev.com/708debd56967a94ef7d5a13b6f77c9cc6762fae3/src/compiler/js-inlining.cc [add] https://crrev.com/708debd56967a94ef7d5a13b6f77c9cc6762fae3/test/mjsunit/regress/regress-crbug-706642.js
,
Apr 3 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 3 2017
[Bulk edit] This issue has been approved for a merge to M58. Please ensure the merge is processed by tomorrow at 5 PM PT so it's included in our next beta build.
,
Apr 3 2017
,
Apr 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/9a4bbad80aebcf1719657a6996a8487725e89a52 commit 9a4bbad80aebcf1719657a6996a8487725e89a52 Author: Adam Klein <adamk@chromium.org> Date: Fri Apr 07 22:20:50 2017 MIPS[64]: One more fix for derived construct stub Avoid clobbering argument count when loading instance type. TBR=bmeurer@chromium.org, ivica.bogosavljevic@imgtec.com Bug: chromium:706642 Change-Id: I82ceb6f1270420ec683f0659f9433795562ab1b4 Reviewed-on: https://chromium-review.googlesource.com/471872 Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/master@{#44497} [modify] https://crrev.com/9a4bbad80aebcf1719657a6996a8487725e89a52/src/builtins/mips/builtins-mips.cc [modify] https://crrev.com/9a4bbad80aebcf1719657a6996a8487725e89a52/src/builtins/mips64/builtins-mips64.cc
,
Apr 10 2017
,
Apr 10 2017
,
Jul 6 2017
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by bmeu...@chromium.org
, Mar 30 2017