Skip to content

Commit 384a4c7

Browse files
sethbrenithsethbrenith
committed
[MERGE chakra-core#5235 @sethbrenith] fix construction of proxied class
Merge pull request chakra-core#5235 from sethbrenith:user/sethb/bound-class-2 Minimum repro: ```javascript class A {} const p = new Proxy(A, {}); Reflect.construct(p, [], A); ``` Reflect.construct passes new.target as an extra arg on the end, but the class constructor expects new.target as arg zero. Proxy needs to be responsible for putting the argument in the correct place when calling a class constructor. Current behavior is that the class constructor tries to get function info about arg zero (some newly-initialized object), and throws "TypeError: Object doesn't support this action". Fixes chakra-core#5225
2 parents 5a344f6 + 546eaa9 commit 384a4c7

2 files changed

Lines changed: 173 additions & 42 deletions

File tree

‎lib/Runtime/Library/JavascriptProxy.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2190,6 +2190,16 @@ namespace Js
21902190
{
21912191
AnalysisAssert(newCount == ((ushort)args.Info.Count) + 1);
21922192
newValues[args.Info.Count] = newTarget;
2193+
2194+
// If the function we're calling is a class constructor, it expects newTarget
2195+
// as the first arg so it knows how to construct "this". (We can leave the
2196+
// extra copy of newTarget at the end of the arguments; it's harmless so
2197+
// there's no need to introduce additional complexity here.)
2198+
FunctionInfo* functionInfo = JavascriptOperators::GetConstructorFunctionInfo(targetObj, scriptContext);
2199+
if (functionInfo && functionInfo->IsClassConstructor())
2200+
{
2201+
newValues[0] = newTarget;
2202+
}
21932203
}
21942204

21952205
Js::Arguments arguments(calleeInfo, newValues);

‎test/es6/boundConstruction.js‎

Lines changed: 163 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
88
//setup code
99
let constructionCount = 0;
1010
let functionCallCount = 0;
11-
let classConstructorCount = 0;
11+
var classConstructorCount = 0; // var not let, so it can be accessed from other context
1212
function a(arg1, arg2) {
1313
this[arg1] = arg2;
1414
this.localVal = this.protoVal;
@@ -25,14 +25,51 @@ class A {
2525
}
2626
A.prototype.protoVal = 2;
2727

28+
const crossContext = WScript.LoadScript("\
29+
class A { \
30+
constructor(arg1, arg2) { \
31+
this[arg1] = arg2; \
32+
this.localVal = this.protoVal; \
33+
++other.classConstructorCount; \
34+
} \
35+
} \
36+
A.prototype.protoVal = 2; \
37+
this.A2 = A; \
38+
", "samethread");
39+
crossContext.other = this;
40+
2841
const trapped = new Proxy(a, {
2942
construct: function (x, y, z) {
3043
++constructionCount;
3144
return Reflect.construct(x, y, z);
3245
}
3346
});
3447

48+
const trappedClass = new Proxy(A, {
49+
construct: function (x, y, z) {
50+
++constructionCount;
51+
return Reflect.construct(x, y, z);
52+
}
53+
});
54+
55+
const evalTrappedClass = new Proxy(A, {
56+
construct: function (x, y, z) {
57+
return eval("++constructionCount; Reflect.construct(x, y, z);");
58+
}
59+
});
60+
61+
const withTrappedClass = new Proxy(A, {
62+
construct: function (x, y, z) {
63+
with (Reflect) {
64+
++constructionCount;
65+
return construct(x, y, z);
66+
}
67+
}
68+
});
69+
3570
const noTrap = new Proxy(a, {});
71+
const noTrapClass = new Proxy(A, {});
72+
const noTrapClassCrossContext = new Proxy(crossContext.A2, {});
3673

3774
const boundObject = {};
3875

@@ -43,6 +80,9 @@ boundClass.prototype = {};
4380

4481
const boundTrapped = trapped.bind(boundObject, "prop-name");
4582
const boundUnTrapped = noTrap.bind(boundObject, "prop-name");
83+
const boundTrappedClass = trappedClass.bind(boundObject, "prop-name");
84+
const boundUnTrappedClass = noTrapClass.bind(boundObject, "prop-name");
85+
const boundUnTrappedClassCrossContext = noTrapClassCrossContext.bind(boundObject, "prop-name");
4686

4787
class newTarget {}
4888
newTarget.prototype.protoVal = 3;
@@ -60,11 +100,47 @@ ExtendsClass.prototype.protoVal = 7;
60100
const boundClassExtendsFunc = ExtendsFunc.bind(boundObject, "prop-name");
61101
const boundClassExtendsClass = ExtendsClass.bind(boundObject, "prop-name");
62102

63-
function test(ctor, useReflect, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount) {
103+
// flags
104+
const ConstructionMode = {
105+
useReflect: 1,
106+
useEval: 2,
107+
useWith: 4,
108+
};
109+
110+
function testImpl(ctor, constructionMode, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount) {
111+
const useReflect = (constructionMode & 1) === 1;
64112
constructionCount = 0;
65113
functionCallCount = 0;
66114
classConstructorCount = 0;
67-
const obj = useReflect ? Reflect.construct(ctor, ["prop-value"], newTarget) : new ctor("prop-value");
115+
let obj;
116+
switch (constructionMode) {
117+
case 0:
118+
obj = new ctor("prop-value");
119+
break;
120+
case 1:
121+
obj = Reflect.construct(ctor, ["prop-value"], newTarget);
122+
break;
123+
case 2:
124+
eval('obj = new ctor("prop-value");');
125+
break;
126+
case 3:
127+
eval('obj = Reflect.construct(ctor, ["prop-value"], newTarget);');
128+
break;
129+
case 4:
130+
with ({}) { obj = new ctor("prop-value"); }
131+
break;
132+
case 5:
133+
with (Reflect) { obj = construct(ctor, ["prop-value"], newTarget); }
134+
break;
135+
case 6:
136+
eval('with ({}) { obj = new ctor("prop-value"); }');
137+
break;
138+
case 7:
139+
eval('with (Reflect) { obj = construct(ctor, ["prop-value"], newTarget); }');
140+
break;
141+
default:
142+
throw new Error("unrecognized mode");
143+
}
68144
assert.areNotEqual(boundObject, obj, "bound function should ignore bound this when constructing");
69145
assert.areEqual("prop-value", obj["prop-name"], "bound function should keep bound arguments when constructing");
70146
assert.areEqual(expectedConstructionCount, constructionCount, `proxy construct trap should be called ${expectedConstructionCount} times`);
@@ -74,101 +150,146 @@ function test(ctor, useReflect, expectedPrototype, expectedConstructionCount, ex
74150
assert.areEqual(expectedPrototype.prototype.protoVal, obj.localVal, "prototype should be available during construction");
75151
}
76152

153+
function test(ctor, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount) {
154+
testImpl(ctor, 0, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
155+
testImpl(ctor, 1, newTarget, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
156+
testImpl(ctor, 2, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
157+
testImpl(ctor, 3, newTarget, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
158+
testImpl(ctor, 4, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
159+
testImpl(ctor, 5, newTarget, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
160+
testImpl(ctor, 6, expectedPrototype, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
161+
testImpl(ctor, 7, newTarget, expectedConstructionCount, expectedFunctionCallCount, expectedClassConstructorCount);
162+
}
163+
77164
const tests = [
78165
{
79-
name : "Construct trapped bound proxy with new",
166+
name : "Construct trapped bound proxy around function",
80167
body : function() {
81-
test(boundTrapped, false, a, 1, 1, 0);
168+
test(boundTrapped, a, 1, 1, 0);
82169
}
83170
},
84171
{
85-
name : "Construct trapped bound proxy with Reflect",
172+
name : "Construct bound proxy around function",
86173
body : function() {
87-
test(boundTrapped, true, newTarget, 1, 1, 0);
174+
test(boundUnTrapped, a, 0, 1, 0);
88175
}
89176
},
90177
{
91-
name : "Construct bound proxy with new",
178+
name : "Construct trapped bound proxy around class",
92179
body : function() {
93-
test(boundUnTrapped, false, a, 0, 1, 0);
180+
test(boundTrappedClass, A, 1, 0, 1);
94181
}
95182
},
96183
{
97-
name : "Construct bound proxy with Reflect",
184+
name : "Construct bound proxy around class",
98185
body : function() {
99-
test(boundUnTrapped, true, newTarget, 0, 1, 0);
186+
test(boundUnTrappedClass, A, 0, 0, 1);
100187
}
101188
},
102189
{
103-
name : "Construct bound function with Reflect",
190+
name : "Construct bound proxy around class with cross-context construction",
104191
body : function() {
105-
test(boundFunc, true, newTarget, 0, 1, 0);
192+
test(boundUnTrappedClassCrossContext, crossContext.A2, 0, 0, 1);
106193
}
107194
},
108195
{
109-
name : "Construct bound function with new",
110-
body : function() {
111-
test(boundFunc, false, a, 0, 1, 0);
196+
name: "Trapped bound proxy around class using eval",
197+
body: function () {
198+
test(evalTrappedClass.bind(boundObject, "prop-name"), A, 1, 0, 1);
112199
}
113200
},
114201
{
115-
name : "Construct bound class with new",
116-
body : function() {
117-
test(boundClass, false, A, 0, 0, 1);
202+
name: "Trapped bound proxy around class using with",
203+
body: function () {
204+
test(withTrappedClass.bind(boundObject, "prop-name"), A, 1, 0, 1);
118205
}
119206
},
120207
{
121-
name : "Construct bound class with Reflect",
208+
name : "Construct bound function",
122209
body : function() {
123-
test(boundClass, true, newTarget, 0, 0, 1);
210+
test(boundFunc, a, 0, 1, 0);
124211
}
125212
},
126213
{
127-
name : "Construct class extending bound function with new",
214+
name : "Construct bound class",
128215
body : function() {
129-
test(ExtendsBoundFunc, false, ExtendsBoundFunc, 0, 1, 0);
216+
test(boundClass, A, 0, 0, 1);
130217
}
131218
},
132219
{
133-
name : "Construct class extending bound function with Reflect",
220+
name : "Construct class extending bound function",
134221
body : function() {
135-
test(ExtendsBoundFunc, true, newTarget, 0, 1, 0);
222+
test(ExtendsBoundFunc, ExtendsBoundFunc, 0, 1, 0);
136223
}
137224
},
138225
{
139-
name : "Construct class extending bound class with new",
226+
name : "Construct class extending bound class",
140227
body : function() {
141-
test(ExtendsBoundClass, false, ExtendsBoundClass, 0, 0, 1);
228+
test(ExtendsBoundClass, ExtendsBoundClass, 0, 0, 1);
142229
}
143230
},
144231
{
145-
name : "Construct class extending bound class with Reflect",
232+
name : "Construct bound class that extends a function",
146233
body : function() {
147-
test(ExtendsBoundClass, true, newTarget, 0, 0, 1);
234+
test(boundClassExtendsFunc, ExtendsFunc, 0, 1, 0);
148235
}
149236
},
150237
{
151-
name : "Construct bound class that extends a function with new",
238+
name : "Construct bound class that extends another class",
152239
body : function() {
153-
test(boundClassExtendsFunc, false, ExtendsFunc, 0, 1, 0);
240+
test(boundClassExtendsClass, ExtendsClass, 0, 0, 1);
154241
}
155242
},
156243
{
157-
name : "Construct bound class that extends a function with Reflect",
244+
name : "Construct bound proxy around proxy",
158245
body : function() {
159-
test(boundClassExtendsFunc, true, newTarget, 0, 1, 0);
160-
}
161-
},
162-
{
163-
name : "Construct bound class that extends another class with new",
164-
body : function() {
165-
test(boundClassExtendsClass, false, ExtendsClass, 0, 0, 1);
246+
const baseProxies = [
247+
{ proxy: trapped, func: true, trap: true },
248+
{ proxy: trappedClass, func: false, trap: true },
249+
{ proxy: noTrap, func: true, trap: false },
250+
{ proxy: noTrapClass, func: false, trap: false }
251+
];
252+
253+
for (const { proxy, func, trap } of baseProxies) {
254+
const p1 = new Proxy(proxy, {});
255+
const p2 = new Proxy(proxy, {
256+
construct: function (x, y, z) {
257+
++constructionCount;
258+
return Reflect.construct(x, y, z);
259+
}
260+
});
261+
const b1 = p1.bind(boundObject, "prop-name");
262+
const b2 = p2.bind(boundObject, "prop-name");
263+
test(b1, func ? a : A, trap ? 1 : 0, func ? 1 : 0, func ? 0 : 1);
264+
test(b2, func ? a : A, 1 + (trap ? 1 : 0), func ? 1 : 0, func ? 0 : 1);
265+
}
166266
}
167267
},
168268
{
169-
name : "Construct bound class that extends another class with Reflect",
170-
body : function() {
171-
test(boundClassExtendsClass, true, newTarget, 0, 0, 1);
269+
name: "Construct built-in class",
270+
body: function () {
271+
const bound = Boolean.bind(boundObject, false);
272+
const noTrap = new Proxy(Boolean, {});
273+
const trap = new Proxy(Boolean, {
274+
construct: function (x, y, z) {
275+
return Reflect.construct(x, y, z);
276+
}
277+
});
278+
279+
function verify(obj, expectedNewTarget) {
280+
assert.isTrue(Boolean.prototype.valueOf.call(obj) === false, "Boolean should represent value false");
281+
assert.isFalse(obj === false, "Boolean is not a value type");
282+
assert.strictEqual(expectedNewTarget.prototype, obj.__proto__, "Object should get constructed with appropriate prototype");
283+
}
284+
285+
verify(new bound(true), Boolean);
286+
verify(Reflect.construct(bound, [true], newTarget), newTarget);
287+
verify(new noTrap(false), Boolean);
288+
verify(eval("new noTrap(false)"), Boolean);
289+
verify(Reflect.construct(noTrap, [false], newTarget), newTarget);
290+
verify(new trap(false), Boolean);
291+
verify(Reflect.construct(trap, [false], newTarget), newTarget);
292+
verify(eval("Reflect.construct(trap, [false], newTarget)"), newTarget);
172293
}
173294
}
174295
];

0 commit comments

Comments
 (0)