Skip to content

Commit 3209462

Browse files
KrinkleKrinkle
committed
Core: Fix crash when "bad thenable" is returned from global module hook
== Background * We run QUnit.test functions via runTest() in src/test.js. This includes both test.callback.call() and resolvePromise(), and thus both are covered by the try-catch in test.run(). * We run (non-global) module hooks via callHook() in src/test.js, which similarly does both, and both are covered by try-catch in test.queueHook(). * We previously ran global module hooks with the resolvePromise() not covered by try-catch. This means a bad thenable returned from a global hook can cause a prettt confusing crash. The ProcessingQueue gets interrupted, a global error is reported (in the console only, not in the UI), the current test then eventually times out, and the ProcessingQueue is unable to continue because it is at non-zero config.depth, yet there are no more top-level tests in the queue, which means advance() crashes with advanceTestQueue() getting undefined from an empty array shift() and trying to call it as a function. == Fix Fix global module hooks to also effectively do a try-catch. However, I'm doing this in a bit of a different way to improve code quality. There is an old TODO in the code from me that planned to actually move the resolvePromise() outside the try-catch for consistency with tests and global hooks. This is a bad idea, as it would spread this bug to module hooks. I had that idea because I didn't realize that: 1. test.run() has a higher-level try-catch so it't actually global hooks that is inconsistent, not module hooks. 2. There is nothing else saving us from a ProcessingQueue crash. Yet, I still believe it is a good idea to move this outside the try-catch because we should not tolerate errors from our internal resolvePromise function. It is only the user-provided `promise.then` call inside thqt function that we want to try-catch. A better way to achieve this is to normalize the user-provided promise via Promise.resolve(). Our reference implementation in lib/ shows that it indeed try-catches both the scheduling of then() callback, and the contents within. So the way I'm fixing this is: * Leave the global hooks code completely unchanged. * Make test.run() and test.queueHook() more like queueGlobalHook, by moving the resolvePromise outside the try-catch. * Fix resolvePromise() to use Promise.resolve() which try-catches just the user-provided "promise.then", nothing else is wrapped in try-catch.
1 parent 6f72eeb commit 3209462

File tree

3 files changed

+194
-68
lines changed

3 files changed

+194
-68
lines changed

‎src/test.js‎

Lines changed: 56 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -177,43 +177,39 @@ Test.prototype = {
177177
run: function () {
178178
config.current = this;
179179

180+
let promise;
180181
if (config.notrycatch) {
181-
runTest(this);
182-
return;
183-
}
184-
185-
try {
186-
runTest(this);
187-
} catch (e) {
188-
this.pushFailure('Died on test #' + (this.assertions.length + 1) + ': ' +
189-
(e.message || e) + '\n' + this.stack, extractStacktrace(e, 0));
190-
191-
// Else next test will carry the responsibility
192-
saveGlobal();
193-
194-
// Restart the tests if they're blocking
195-
if (config.blocking) {
196-
internalRecover(this);
182+
promise = (this.withData)
183+
? this.callback.call(this.testEnvironment, this.assert, this.data)
184+
: this.callback.call(this.testEnvironment, this.assert);
185+
} else {
186+
try {
187+
promise = (this.withData)
188+
? this.callback.call(this.testEnvironment, this.assert, this.data)
189+
: this.callback.call(this.testEnvironment, this.assert);
190+
} catch (e) {
191+
this.pushFailure('Died on test #' + (this.assertions.length + 1) + ': ' +
192+
(e.message || e) + '\n' + this.stack, extractStacktrace(e, 0));
193+
194+
// Else next test will carry the responsibility
195+
saveGlobal();
196+
197+
// Restart the tests if they're blocking
198+
if (config.blocking) {
199+
internalRecover(this);
200+
}
197201
}
198202
}
199203

200-
function runTest (test) {
201-
let promise;
202-
if (test.withData) {
203-
promise = test.callback.call(test.testEnvironment, test.assert, test.data);
204-
} else {
205-
promise = test.callback.call(test.testEnvironment, test.assert);
206-
}
207-
test.resolvePromise(promise);
208-
209-
// If the test has an async "pause" on it, but the timeout is 0, then we push a
210-
// failure as the test should be synchronous.
211-
if (test.timeout === 0 && test.pauses.size > 0) {
212-
pushFailure(
213-
'Test did not finish synchronously even though assert.timeout( 0 ) was used.',
214-
sourceFromStacktrace(2)
215-
);
216-
}
204+
this.resolvePromise(promise);
205+
206+
// If the test has an async "pause" on it, but the timeout is 0, then we push a
207+
// failure as the test should be synchronous.
208+
if (this.timeout === 0 && this.pauses.size > 0) {
209+
pushFailure(
210+
'Test did not finish synchronously even though assert.timeout( 0 ) was used.',
211+
sourceFromStacktrace(2)
212+
);
217213
}
218214
},
219215

@@ -222,11 +218,10 @@ Test.prototype = {
222218
},
223219

224220
queueGlobalHook (hook, hookName) {
225-
const runHook = () => {
221+
const runGlobalHook = () => {
226222
config.current = this;
227223

228224
let promise;
229-
230225
if (config.notrycatch) {
231226
promise = hook.call(this.testEnvironment, this.assert);
232227
} else {
@@ -244,22 +239,10 @@ Test.prototype = {
244239

245240
this.resolvePromise(promise, hookName);
246241
};
247-
248-
return runHook;
242+
return runGlobalHook;
249243
},
250244

251245
queueHook (hook, hookName, hookOwner) {
252-
const callHook = () => {
253-
let promise;
254-
if (hookName === 'before' || hookName === 'after') {
255-
// before and after hooks are called with the owning module's testEnvironment
256-
promise = hook.call(hookOwner.testEnvironment, this.assert);
257-
} else {
258-
promise = hook.call(this.testEnvironment, this.assert);
259-
}
260-
this.resolvePromise(promise, hookName);
261-
};
262-
263246
const runHook = () => {
264247
if (hookName === 'before' && hookOwner.testsRun !== 0) {
265248
return;
@@ -274,25 +257,27 @@ Test.prototype = {
274257
}
275258

276259
config.current = this;
260+
261+
// before and after hooks are called with the owning module's testEnvironment
262+
const testEnvironment = (hookName === 'before' || hookName === 'after')
263+
? hookOwner.testEnvironment
264+
: this.testEnvironment;
265+
266+
let promise;
277267
if (config.notrycatch) {
278-
callHook();
279-
return;
280-
}
281-
try {
282-
// This try-block includes the indirect call to resolvePromise, which shouldn't
283-
// have to be inside try-catch. But, since we support any user-provided thenable
284-
// object, the thenable might throw in some unexpected way.
285-
// This subtle behaviour is undocumented. To avoid new failures in minor releases
286-
// we will not change this until QUnit 3.
287-
// TODO: In QUnit 3, reduce this try-block to just hook.call(), matching
288-
// the simplicity of queueGlobalHook.
289-
callHook();
290-
} catch (error) {
291-
this.pushFailure(hookName + ' failed on ' + this.testName + ': ' +
292-
(error.message || error), extractStacktrace(error, 0));
268+
promise = hook.call(testEnvironment, this.assert);
269+
} else {
270+
try {
271+
promise = hook.call(testEnvironment, this.assert);
272+
} catch (error) {
273+
this.pushFailure(hookName + ' failed on ' + this.testName + ': ' +
274+
(error.message || error), extractStacktrace(error, 0));
275+
return;
276+
}
293277
}
294-
};
295278

279+
this.resolvePromise(promise, hookName);
280+
};
296281
return runHook;
297282
},
298283

@@ -757,12 +742,11 @@ Test.prototype = {
757742
resolvePromise: function (promise, phase) {
758743
if (promise !== undefined && promise !== null) {
759744
const test = this;
760-
const then = promise.then;
761-
if (typeof then === 'function') {
745+
if (typeof promise.then === 'function') {
762746
const resume = test.internalStop();
763747
const resolve = function () { resume(); };
764748
if (config.notrycatch) {
765-
then.call(promise, resolve);
749+
promise.then(resolve);
766750
} else {
767751
const reject = function (error) {
768752
const message = 'Promise rejected ' +
@@ -777,7 +761,11 @@ Test.prototype = {
777761
// Unblock
778762
internalRecover(test);
779763
};
780-
then.call(promise, resolve, reject);
764+
// Note that `promise` is a user-supplied thenable, not per-se a standard Promise.
765+
// This means it can be a "bad thenable" where calling then() can already throw.
766+
// We must catch this to avoid breaking the ProcessingQueue. Promise.resolve()
767+
// normalizes and catches even these internal errors and sends them to reject.
768+
Promise.resolve(promise).then(resolve, reject);
781769
}
782770
}
783771
}

‎test/cli/fixtures/async-test-throw.js‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
QUnit.test('throw early', async function (_assert) {
2+
throw new Error('boo');
3+
});
4+
5+
QUnit.test('throw late', async function (assert) {
6+
await Promise.resolve('');
7+
assert.true(true);
8+
throw new Error('boo');
9+
});
10+
11+
// NOTE: This is not about testing a rejected Promise or throwing async function.
12+
// In those cases, `ret.then(, cb)` will not throw, but inform you via cb(err).
13+
// Instead, this is testing a bad Thenable implementation, where then() itself
14+
// throws an error. This is not possible with native Promise, but is possible with
15+
// custom Promise-compatible libraries and
16+
QUnit.test('test with bad thenable', function (assert) {
17+
assert.true(true);
18+
return {
19+
then: function () {
20+
throw new Error('boo');
21+
}
22+
};
23+
});
24+
25+
QUnit.hooks.beforeEach(function (assert) {
26+
if (assert.test.testName !== 'example') { return; }
27+
return {
28+
then: function () {
29+
throw new Error('global brocoli');
30+
}
31+
};
32+
});
33+
QUnit.hooks.afterEach(function (assert) {
34+
if (assert.test.testName !== 'example') { return; }
35+
return {
36+
then: function () {
37+
throw new Error('global artichoke');
38+
}
39+
};
40+
});
41+
42+
QUnit.module('hooks with bad thenable', function (hooks) {
43+
hooks.beforeEach(function () {
44+
return {
45+
then: function () {
46+
throw new Error('banana');
47+
}
48+
};
49+
});
50+
hooks.afterEach(function () {
51+
return {
52+
then: function () {
53+
throw new Error('apple');
54+
}
55+
};
56+
});
57+
58+
QUnit.test('example', function (assert) {
59+
assert.true(true);
60+
});
61+
});

‎test/cli/fixtures/async-test-throw.tap.txt‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
# command: ["qunit", "async-test-throw.js"]
2+
3+
TAP version 13
4+
not ok 1 throw early
5+
---
6+
message: "Promise rejected during \"throw early\": boo"
7+
severity: failed
8+
actual : null
9+
expected: undefined
10+
stack: |
11+
Error: boo
12+
at /qunit/test/cli/fixtures/async-test-throw.js:2:9
13+
...
14+
not ok 2 throw late
15+
---
16+
message: "Promise rejected during \"throw late\": boo"
17+
severity: failed
18+
actual : null
19+
expected: undefined
20+
stack: |
21+
Error: boo
22+
at /qunit/test/cli/fixtures/async-test-throw.js:8:9
23+
...
24+
not ok 3 test with bad thenable
25+
---
26+
message: "Promise rejected during \"test with bad thenable\": boo"
27+
severity: failed
28+
actual : null
29+
expected: undefined
30+
stack: |
31+
Error: boo
32+
at /qunit/test/cli/fixtures/async-test-throw.js:20:13
33+
...
34+
not ok 4 hooks with bad thenable > example
35+
---
36+
message: "Promise rejected before \"example\": global brocoli"
37+
severity: failed
38+
actual : null
39+
expected: undefined
40+
stack: |
41+
Error: global brocoli
42+
at /qunit/test/cli/fixtures/async-test-throw.js:29:13
43+
...
44+
---
45+
message: "Promise rejected before \"example\": banana"
46+
severity: failed
47+
actual : null
48+
expected: undefined
49+
stack: |
50+
Error: banana
51+
at /qunit/test/cli/fixtures/async-test-throw.js:46:15
52+
...
53+
---
54+
message: "Promise rejected after \"example\": apple"
55+
severity: failed
56+
actual : null
57+
expected: undefined
58+
stack: |
59+
Error: apple
60+
at /qunit/test/cli/fixtures/async-test-throw.js:53:15
61+
...
62+
---
63+
message: "Promise rejected after \"example\": global artichoke"
64+
severity: failed
65+
actual : null
66+
expected: undefined
67+
stack: |
68+
Error: global artichoke
69+
at /qunit/test/cli/fixtures/async-test-throw.js:37:13
70+
...
71+
1..4
72+
# pass 0
73+
# skip 0
74+
# todo 0
75+
# fail 4
76+
77+
# exit code: 1

0 commit comments

Comments
 (0)