Skip to content

Commit 29265f9

Browse files
hyjorc1hyjorc1
committed
Fix TypeError in parameter destructuring in async function should be a rejected promise
https://bugs.webkit.org/show_bug.cgi?id=247785 rdar://102325201 Reviewed by Yusuke Suzuki. Rest parameter should be caught in async function. So, running this JavaScript program should print "caught". ``` async function f(...[[]]) { } f().catch(e => print("caught")); ``` V8 (used console.log) ``` $ node input.js caught ``` GraalJS ``` $ js input.js caught ``` https://tc39.es/ecma262/#sec-async-function-definitions ... AsyncFunctionDeclaration[Yield, Await, Default] : async [no LineTerminator here] function BindingIdentifier[?Yield, ?Await] ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } [+Default] async [no LineTerminator here] function ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } AsyncFunctionExpression : async [no LineTerminator here] function BindingIdentifier[~Yield, +Await]opt ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } ... According to the spec, it indicates `FormalParameters` is used for Async Function, where `FormalParameters` can be converted to `FunctionRestParameter`. https://tc39.es/ecma262/#sec-parameter-lists ... FormalParameters[Yield, Await] : [empty] FunctionRestParameter[?Yield, ?Await] FormalParameterList[?Yield, ?Await] FormalParameterList[?Yield, ?Await] , FormalParameterList[?Yield, ?Await] , FunctionRestParameter[?Yield, ?Await] ... And based on RS: EvaluateAsyncFunctionBody, it will invoke the promise.reject callback function with abrupt value ([[value]] of non-normal completion record). https://tc39.es/ecma262/#sec-runtime-semantics-evaluateasyncfunctionbody ... 2. Let declResult be Completion(FunctionDeclarationInstantiation(functionObject, argumentsList)). 3. If declResult is an abrupt completion, then a. Perform ! Call(promiseCapability.[[Reject]], undefined, « declResult.[[Value]] »). ... In that case, any non-normal results of evaluating rest parameters should be caught and passed to the reject callback function. To resolve this problem, we should allow the emitted RestParameterNode be wrapped by the catch handler for promise. However, we should remove `m_restParameter` and emit rest parameter byte code in `initializeDefaultParameterValuesAndSetupFunctionScopeStack` if we can prove that change has no side effect. In that case, we can only use one exception handler. Current fix is to add another exception handler. And move the handler byte codes to the bottom of code block in order to make other byte codes as much compact as possible. Input: ``` async function f(arg0, ...[[]]) { } f(); ``` Dumped Byte Codes: ``` ... bb#2 Predecessors: [ #1 ] [ 20] mov dst:loc9, src:<JSValue()>(const0) ... bb#3 Predecessors: [ #2 ] [ 29] get_rest_length dst:loc11, numParametersToSkip:1 ... bb#12 Predecessors: [ #8 #9 #10 ] [ 138] new_func_exp dst:loc10, scope:loc4, functionDecl:0 ... bb#13 Predecessors: [ ] [ 170] catch exception:loc10, thrownValue:loc8 [ 174] jmp targetLabel:8(->182) Successors: [ #15 ] bb#14 Predecessors: [ #7 #11 ] [ 176] catch exception:loc10, thrownValue:loc8 [ 180] jmp targetLabel:2(->182) Successors: [ #15 ] bb#15 Predecessors: [ #13 #14 ] [ 182] mov dst:loc12, src:Undefined(const1) ... Exception Handlers: 1: { start: [ 20] end: [ 29] target: [ 170] } synthesized catch 2: { start: [ 29] end: [ 138] target: [ 176] } synthesized catch ``` * JSTests/stress/catch-rest-parameter.js: Added. (throwError): (shouldThrow): (async f): (throwError.async f): (throwError.async let): (async let): (x.async f): (x): (async shouldThrow): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::BytecodeGenerator): (JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: Canonical link: https://commits.webkit.org/256864@main
1 parent 5bc1319 commit 29265f9

File tree

3 files changed

+250
-36
lines changed

3 files changed

+250
-36
lines changed

‎JSTests/stress/catch-rest-parameter.js‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
function assert(b) {
2+
if (!b)
3+
throw "bad assert!";
4+
}
5+
6+
function shouldThrow(func, errorMessage) {
7+
var errorThrown = false;
8+
var error = null;
9+
try {
10+
func();
11+
} catch (e) {
12+
errorThrown = true;
13+
error = e;
14+
}
15+
if (!errorThrown)
16+
throw new Error('not thrown');
17+
if (String(error) !== errorMessage)
18+
throw new Error(`
19+
bad error: ${String(error)}
20+
expected error: ${errorMessage}
21+
`);
22+
}
23+
24+
let expected = "TypeError: undefined is not an object (evaluating '[[]]')";
25+
26+
// AsyncFunction
27+
(() => {
28+
async function f(...[[]]) { }
29+
let isExpected = false;
30+
f().then(e => isExpected = false).catch(e => isExpected = e == expected);
31+
drainMicrotasks();
32+
assert(isExpected);
33+
})();
34+
35+
(() => {
36+
async function f([[]]) { }
37+
let isExpected = false;
38+
f().then(e => isExpected = false).catch(e => isExpected = e == expected);
39+
drainMicrotasks();
40+
assert(isExpected);
41+
})();
42+
43+
(() => {
44+
async function f(a, ...[[]]) { }
45+
let isExpected = false;
46+
f().then(e => isExpected = false).catch(e => isExpected = e == expected);
47+
drainMicrotasks();
48+
assert(isExpected);
49+
})();
50+
51+
(() => {
52+
var a = 1, b = 2, c = 3;
53+
async function f(arg0, ...args) {
54+
a = arg0;
55+
b = args[0];
56+
c = args[1];
57+
}
58+
let isExpected = true;
59+
f(4, 5, 6).then(e => isExpected = true).catch(e => isExpected = false);
60+
drainMicrotasks();
61+
assert(isExpected && a === 4 && b === 5 && c === 6);
62+
})();
63+
64+
65+
// AsyncArrowFunction
66+
(() => {
67+
let f = async (...[[]]) => { };
68+
let isExpected = false;
69+
f().then(e => isExpected = false).catch(e => isExpected = e == expected);
70+
drainMicrotasks();
71+
assert(isExpected);
72+
})();
73+
74+
(() => {
75+
let f = async ([[]]) => { };
76+
let isExpected = false;
77+
f().then(e => isExpected = false).catch(e => isExpected = e == expected);
78+
drainMicrotasks();
79+
assert(isExpected);
80+
})();
81+
82+
(() => {
83+
let f = async (a, ...[[]]) => { };
84+
let isExpected = false;
85+
f().then(e => isExpected = false).catch(e => isExpected = e == expected);
86+
drainMicrotasks();
87+
assert(isExpected);
88+
})();
89+
90+
(() => {
91+
var a = 1, b = 2, c = 3;
92+
let f = async (arg0, ...args) => {
93+
a = arg0;
94+
b = args[0];
95+
c = args[1];
96+
};
97+
let isExpected = true;
98+
f(4, 5, 6).then(e => isExpected = true).catch(e => isExpected = false);
99+
drainMicrotasks();
100+
assert(isExpected && a === 4 && b === 5 && c === 6);
101+
})();
102+
103+
104+
// AsyncMethod
105+
(() => {
106+
class x { static async f(...[[]]) { } }
107+
let isExpected = false;
108+
x.f().then(e => isExpected = false).catch(e => isExpected = e == expected);
109+
drainMicrotasks();
110+
assert(isExpected);
111+
})();
112+
113+
(() => {
114+
class x { static async f([[]]) { } }
115+
let isExpected = false;
116+
x.f().then(e => isExpected = false).catch(e => isExpected = e == expected);
117+
drainMicrotasks();
118+
assert(isExpected);
119+
})();
120+
121+
(() => {
122+
class x { static async f(a, ...[[]]) { } }
123+
let isExpected = false;
124+
x.f().then(e => isExpected = false).catch(e => isExpected = e == expected);
125+
drainMicrotasks();
126+
assert(isExpected);
127+
})();
128+
129+
(() => {
130+
var a = 1, b = 2, c = 3;
131+
class x {
132+
static async f(arg0, ...args) {
133+
a = arg0;
134+
b = args[0];
135+
c = args[1];
136+
}
137+
}
138+
let isExpected = true;
139+
x.f(4, 5, 6).then(e => isExpected = true).catch(e => isExpected = false);
140+
drainMicrotasks();
141+
assert(isExpected && a === 4 && b === 5 && c === 6);
142+
})();
143+
144+
// AsyncGenerator
145+
shouldThrow(async function* f([[]]) { }, expected);
146+
shouldThrow(async function* f(...[[]]) { }, expected);
147+
shouldThrow(async function* f(a, ...[[]]) { }, expected);
148+
(() => {
149+
var a = 1, b = 2, c = 3;
150+
async function* f(arg0, ...args) {
151+
a = arg0;
152+
b = args[0];
153+
c = args[1];
154+
}
155+
f(4, 5, 6);
156+
drainMicrotasks();
157+
assert(a === 1 && b === 2 && c === 3);
158+
})();
159+
160+
// Generator
161+
shouldThrow(function* f([[]]) { }, expected);
162+
shouldThrow(function* f(...[[]]) { }, expected);
163+
shouldThrow(function* f(a, ...[[]]) { }, expected);
164+
(() => {
165+
var a = 1, b = 2, c = 3;
166+
function* f(arg0, ...args) {
167+
a = arg0;
168+
b = args[0];
169+
c = args[1];
170+
}
171+
f(4, 5, 6);
172+
assert(a === 1 && b === 2 && c === 3);
173+
})();
174+
175+
// Function
176+
shouldThrow(function f([[]]) { }, expected);
177+
shouldThrow(function f(...[[]]) { }, expected);
178+
shouldThrow(function f(a, ...[[]]) { }, expected);
179+
(() => {
180+
var a = 1, b = 2, c = 3;
181+
function f(arg0, ...args) {
182+
a = arg0;
183+
b = args[0];
184+
c = args[1];
185+
}
186+
f(4, 5, 6);
187+
assert(a === 4 && b === 5 && c === 6);
188+
})();

‎Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp‎

Lines changed: 52 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,28 @@ FinallyContext::FinallyContext(BytecodeGenerator& generator, Label& finallyLabel
148148
generator.moveEmptyValue(completionValueRegister());
149149
}
150150

151+
template<typename EmitBytecodeFunctor>
152+
void BytecodeGenerator::asyncFuncParametersTryCatchWrap(const EmitBytecodeFunctor& emitBytecode)
153+
{
154+
TryData* tryData = nullptr;
155+
if (m_asyncFuncParametersTryCatchInfo) {
156+
AsyncFuncParametersTryCatchInfo& info = m_asyncFuncParametersTryCatchInfo.value();
157+
ASSERT(info.catchStartLabel && info.thrownValue);
158+
Ref<Label> tryStartLabel = newEmittedLabel();
159+
tryData = pushTry(tryStartLabel.get(), *info.catchStartLabel.get(), HandlerType::SynthesizedCatch);
160+
}
161+
162+
emitBytecode();
163+
164+
if (m_asyncFuncParametersTryCatchInfo) {
165+
AsyncFuncParametersTryCatchInfo& info = m_asyncFuncParametersTryCatchInfo.value();
166+
Ref<Label> tryEndLabel = newEmittedLabel();
167+
popTry(tryData, tryEndLabel.get());
168+
169+
emitOutOfLineCatchHandler(info.thrownValue.get(), nullptr, tryData);
170+
}
171+
}
172+
151173
ParserError BytecodeGenerator::generate(unsigned& size)
152174
{
153175
if (UNLIKELY(m_outOfMemoryDuringConstruction))
@@ -174,8 +196,13 @@ ParserError BytecodeGenerator::generate(unsigned& size)
174196
if (m_needToInitializeArguments)
175197
initializeVariable(variable(propertyNames().arguments), m_argumentsRegister);
176198

177-
if (m_restParameter)
178-
m_restParameter->emit(*this);
199+
if (m_restParameter) {
200+
// We should move moving m_restParameter->emit(*this) to initializeDefaultParameterValuesAndSetupFunctionScopeStack
201+
// as an optimization if we can prove that change has no side effect.
202+
asyncFuncParametersTryCatchWrap([&] {
203+
m_restParameter->emit(*this);
204+
});
205+
}
179206

180207
{
181208
RefPtr<RegisterID> temp = newTemporary();
@@ -244,6 +271,22 @@ ParserError BytecodeGenerator::generate(unsigned& size)
244271
tryData->target = WTFMove(realCatchTarget);
245272
}
246273

274+
if (m_asyncFuncParametersTryCatchInfo) {
275+
AsyncFuncParametersTryCatchInfo& info = m_asyncFuncParametersTryCatchInfo.value();
276+
ASSERT(info.catchStartLabel && info.thrownValue);
277+
emitLabel(*info.catchStartLabel.get());
278+
// @rejectPromiseWithFirstResolvingFunctionCallCheck(@promise, thrownValue);
279+
// return @promise;
280+
RefPtr<RegisterID> rejectPromise = moveLinkTimeConstant(nullptr, LinkTimeConstant::rejectPromiseWithFirstResolvingFunctionCallCheck);
281+
CallArguments args(*this, nullptr, 2);
282+
emitLoad(args.thisRegister(), jsUndefined());
283+
move(args.argumentRegister(0), promiseRegister());
284+
move(args.argumentRegister(1), info.thrownValue.get());
285+
JSTextPosition divot(m_scopeNode->firstLine(), m_scopeNode->startOffset(), m_scopeNode->lineStartOffset());
286+
emitCall(newTemporary(), rejectPromise.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
287+
emitReturn(promiseRegister());
288+
}
289+
247290
m_staticPropertyAnalyzer.kill();
248291

249292
for (auto& range : m_tryRanges) {
@@ -798,40 +841,14 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
798841
emitPutDerivedConstructorToArrowFunctionContextScope();
799842
}
800843

801-
Ref<Label> catchLabel = newLabel();
802-
TryData* tryFormalParametersData = nullptr;
803-
bool needTryCatch = isAsyncFunctionWrapperParseMode(parseMode) && !isSimpleParameterList;
804-
if (needTryCatch) {
805-
Ref<Label> tryFormalParametersStart = newEmittedLabel();
806-
tryFormalParametersData = pushTry(tryFormalParametersStart.get(), catchLabel.get(), HandlerType::SynthesizedCatch);
807-
}
808-
809-
// All "addVar()"s needs to happen before "initializeDefaultParameterValuesAndSetupFunctionScopeStack()" is called
810-
// because a function's default parameter ExpressionNodes will use temporary registers.
811-
initializeDefaultParameterValuesAndSetupFunctionScopeStack(parameters, isSimpleParameterList, functionNode, functionSymbolTable, symbolTableConstantIndex, captures, shouldCreateArgumentsVariableInParameterScope);
812-
813-
if (needTryCatch) {
814-
Ref<Label> didNotThrow = newLabel();
815-
emitJump(didNotThrow.get());
816-
emitLabel(catchLabel.get());
817-
popTry(tryFormalParametersData, catchLabel.get());
818-
819-
RefPtr<RegisterID> thrownValue = newTemporary();
820-
emitOutOfLineCatchHandler(thrownValue.get(), nullptr, tryFormalParametersData);
821-
822-
// @rejectPromiseWithFirstResolvingFunctionCallCheck(@promise, thrownValue);
823-
// return @promise;
824-
RefPtr<RegisterID> rejectPromise = moveLinkTimeConstant(nullptr, LinkTimeConstant::rejectPromiseWithFirstResolvingFunctionCallCheck);
825-
CallArguments args(*this, nullptr, 2);
826-
emitLoad(args.thisRegister(), jsUndefined());
827-
move(args.argumentRegister(0), promiseRegister());
828-
move(args.argumentRegister(1), thrownValue.get());
829-
JSTextPosition divot(functionNode->firstLine(), functionNode->startOffset(), functionNode->lineStartOffset());
830-
emitCall(newTemporary(), rejectPromise.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
844+
if (isAsyncFunctionWrapperParseMode(parseMode) && !isSimpleParameterList)
845+
m_asyncFuncParametersTryCatchInfo = { newLabel(), newTemporary() };
831846

832-
emitReturn(promiseRegister());
833-
emitLabel(didNotThrow.get());
834-
}
847+
asyncFuncParametersTryCatchWrap([&]() {
848+
// All "addVar()"s needs to happen before "initializeDefaultParameterValuesAndSetupFunctionScopeStack()" is called
849+
// because a function's default parameter ExpressionNodes will use temporary registers.
850+
initializeDefaultParameterValuesAndSetupFunctionScopeStack(parameters, isSimpleParameterList, functionNode, functionSymbolTable, symbolTableConstantIndex, captures, shouldCreateArgumentsVariableInParameterScope);
851+
});
835852

836853
// If we don't have default parameter expression, then loading |this| inside an arrow function must be done
837854
// after initializeDefaultParameterValuesAndSetupFunctionScopeStack() because that function sets up the

‎Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1290,7 +1290,16 @@ namespace JSC {
12901290
Vector<std::pair<FunctionMetadataNode*, FunctionVariableType>> m_functionsToInitialize;
12911291
bool m_needToInitializeArguments { false };
12921292
RestParameterNode* m_restParameter { nullptr };
1293-
1293+
1294+
struct AsyncFuncParametersTryCatchInfo {
1295+
RefPtr<Label> catchStartLabel { nullptr };
1296+
RefPtr<RegisterID> thrownValue { nullptr };
1297+
};
1298+
std::optional<AsyncFuncParametersTryCatchInfo> m_asyncFuncParametersTryCatchInfo;
1299+
1300+
template<typename EmitBytecodeFunctor>
1301+
void asyncFuncParametersTryCatchWrap(const EmitBytecodeFunctor&);
1302+
12941303
Vector<TryRange> m_tryRanges;
12951304
SegmentedVector<TryData, 8> m_tryData;
12961305

0 commit comments

Comments
 (0)