Skip to content

Commit e4e50ff

Browse files
committed
Fix errors in construction of bound Proxies. Fixes:
1. Constructing bound Proxy would call construct trap twice 2. Reflect.construct in construct trap of bound Proxy would attempt to construct "this" #4918 3. Constructing bound functions (or Proxies) would ignore newTarget Add text cases for these points and some related issues that would break if certain other fixes for these issues were used.
1 parent 579267d commit e4e50ff

File tree

5 files changed

+140
-16
lines changed

5 files changed

+140
-16
lines changed

lib/Runtime/Library/BoundFunction.cpp

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ namespace Js
127127
}
128128

129129
BoundFunction *boundFunction = (BoundFunction *) function;
130-
Var targetFunction = boundFunction->targetFunction;
130+
RecyclableObject *targetFunction = boundFunction->targetFunction;
131131

132132
//
133133
// var o = new boundFunction()
@@ -136,16 +136,25 @@ namespace Js
136136
Var newVarInstance = nullptr;
137137
if (callInfo.Flags & CallFlags_New)
138138
{
139-
if (JavascriptProxy::Is(targetFunction))
140-
{
141-
JavascriptProxy* proxy = JavascriptProxy::FromVar(targetFunction);
142-
Arguments proxyArgs(CallInfo(CallFlags_New, 1), &targetFunction);
143-
args.Values[0] = newVarInstance = proxy->ConstructorTrap(proxyArgs, scriptContext, 0);
144-
}
145-
else
146-
{
147-
args.Values[0] = newVarInstance = JavascriptOperators::NewScObjectNoCtor(targetFunction, scriptContext);
148-
}
139+
if (args.HasNewTarget())
140+
{
141+
// target has an overriden new target make a new object from the newTarget
142+
Var newTargetVar = args.GetNewTarget();
143+
AssertOrFailFastMsg(JavascriptOperators::IsConstructor(newTargetVar), "newTarget must be a constructor");
144+
RecyclableObject* newTarget = RecyclableObject::UnsafeFromVar(newTargetVar);
145+
args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(newTarget, scriptContext);
146+
}
147+
else if (!JavascriptProxy::Is(targetFunction))
148+
{
149+
// no new target and target is not a proxy can make a new object in a "normal" way
150+
args.Values[0] = newVarInstance = JavascriptOperators::CreateFromConstructor(targetFunction, scriptContext);
151+
}
152+
else
153+
{
154+
// target is a proxy without an overriden new target
155+
// give nullptr - FunctionCallTrap will make a new object
156+
args.Values[0] = newVarInstance;
157+
}
149158
}
150159

151160
Js::Arguments actualArgs = args;
@@ -206,9 +215,8 @@ namespace Js
206215
}
207216
}
208217

209-
RecyclableObject* actualFunction = RecyclableObject::FromVar(targetFunction);
210218
// Number of arguments are allowed to be more than Constants::MaxAllowedArgs in runtime. Need to use the larger argcount logic for this call.
211-
Var aReturnValue = JavascriptFunction::CallFunction<true>(actualFunction, actualFunction->GetEntryPoint(), actualArgs, /* useLargeArgCount */ true);
219+
Var aReturnValue = JavascriptFunction::CallFunction<true>(targetFunction, targetFunction->GetEntryPoint(), actualArgs, /* useLargeArgCount */ true);
212220

213221
//
214222
// [[Construct]] and call returned a non-object

test/UnitTestFramework/UnitTestFramework.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
// example: testRunner.LoadModule(source, 'samethread', false);
2727
//
2828
// How to use assert:
29+
// assert.strictEqual(expected, actual, "those two should be strictly equal (i.e. === comparison)");
2930
// assert.areEqual(expected, actual, "those two should be equal (i.e. deep equality of objects using ===)");
3031
// assert.areNotEqual(expected, actual, "those two should NOT be equal");
3132
// assert.areAlmostEqual(expected, actual, "those two should be almost equal, numerically (allows difference by epsilon)");
@@ -407,6 +408,10 @@ var assert = function assert() {
407408
}
408409

409410
return {
411+
strictEqual: function strictEqual(expected, actual, message) {
412+
validate(expected === actual, "strictEqual", message);
413+
},
414+
410415
areEqual: function areEqual(expected, actual, message) {
411416
/// <summary>
412417
/// IMPORTANT: NaN compares equal.<br/>

test/es6/boundConstruction.js

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
7+
8+
//setup code
9+
let constructionCount = 0;
10+
let functionCallCount = 0;
11+
function a(arg1, arg2) {
12+
this[arg1] = arg2;
13+
++functionCallCount;
14+
}
15+
16+
const trapped = new Proxy(a, {
17+
construct: function (x, y, z) {
18+
++constructionCount;
19+
return Reflect.construct(x, y, z);
20+
}
21+
});
22+
23+
const noTrap = new Proxy(a, {});
24+
25+
const boundObject = {};
26+
27+
const boundFunc = a.bind(boundObject, "prop-name");
28+
29+
const boundTrapped = trapped.bind(boundObject, "prop-name");
30+
const boundUnTrapped = noTrap.bind(boundObject, "prop-name");
31+
32+
const tests = [
33+
{
34+
name : "Construct trapped bound proxy with new",
35+
body : function() {
36+
constructionCount = 0;
37+
functionCallCount = 0;
38+
const obj = new boundTrapped("prop-value");
39+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
40+
assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
41+
assert.areEqual(1, constructionCount, "bound proxy should be constructed once");
42+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
43+
assert.strictEqual(a.prototype, obj.__proto__, "constructed object should be instance of original function");
44+
}
45+
},
46+
{
47+
name : "Construct trapped bound proxy with Reflect",
48+
body : function() {
49+
class newTarget {}
50+
constructionCount = 0;
51+
functionCallCount = 0;
52+
const obj = Reflect.construct(boundTrapped, ["prop-value-2"], newTarget);
53+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
54+
assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
55+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
56+
assert.areEqual(1, constructionCount, "bound proxy should be constructed once");
57+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
58+
}
59+
},
60+
{
61+
name : "Construct bound proxy with new",
62+
body : function() {
63+
functionCallCount = 0;
64+
const obj = new boundUnTrapped("prop-value");
65+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
66+
assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
67+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
68+
assert.strictEqual(a.prototype, obj.__proto__, "constructed object should be instance of original function");
69+
}
70+
},
71+
{
72+
name : "Construct bound proxy with Reflect",
73+
body : function() {
74+
class newTarget {}
75+
functionCallCount = 0;
76+
const obj = Reflect.construct(boundUnTrapped, ["prop-value-2"], newTarget);
77+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
78+
assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
79+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
80+
assert.areEqual(1, functionCallCount, "bound proxy function should be called once");
81+
}
82+
},
83+
{
84+
name : "Construct bound function with Reflect",
85+
body : function() {
86+
class newTarget {}
87+
functionCallCount = 0;
88+
const obj = Reflect.construct(boundFunc, ["prop-value-2"], newTarget);
89+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
90+
assert.strictEqual(newTarget.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
91+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
92+
assert.areEqual(1, functionCallCount, "bound function should be called once");
93+
}
94+
},
95+
{
96+
name : "Construct bound function with new",
97+
body : function() {
98+
functionCallCount = 0;
99+
const obj = new boundFunc("prop-value-2");
100+
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
101+
assert.strictEqual(a.prototype, obj.__proto__, "bound function should use explicit newTarget if provided");
102+
assert.areEqual("prop-value-2", obj["prop-name"], "bound function should keep bound arguments when constructing");
103+
assert.areEqual(1, functionCallCount, "bound function should be called once");
104+
}
105+
}
106+
];
107+
108+
testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

test/es6/proxytest9.baseline

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,6 @@ constructor trap
7777
constructor trap
7878
get trap prototype
7979
constructor trap
80-
constructor trap
81-
get trap prototype
82-
constructor trap
8380
true
8481
get trap m
8582
def

test/es6/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,12 @@
567567
<compile-flags>-ES6Classes -args summary -endargs</compile-flags>
568568
</default>
569569
</test>
570+
<test>
571+
<default>
572+
<files>boundConstruction.js</files>
573+
<compile-flags>-args summary -endargs</compile-flags>
574+
</default>
575+
</test>
570576
<test>
571577
<default>
572578
<files>CrossContextSpreadfunctionCall.js</files>

0 commit comments

Comments
 (0)