From 3209462b88bd0ca4a492c7aa60c3cbee7a31cf22 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Mon, 8 Jul 2024 21:26:52 +0100 Subject: [PATCH] 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. --- src/test.js | 124 ++++++++++----------- test/cli/fixtures/async-test-throw.js | 61 ++++++++++ test/cli/fixtures/async-test-throw.tap.txt | 77 +++++++++++++ 3 files changed, 194 insertions(+), 68 deletions(-) create mode 100644 test/cli/fixtures/async-test-throw.js create mode 100644 test/cli/fixtures/async-test-throw.tap.txt diff --git a/src/test.js b/src/test.js index ce2811aba..d551bd7b5 100644 --- a/src/test.js +++ b/src/test.js @@ -177,43 +177,39 @@ Test.prototype = { run: function () { config.current = this; + let promise; if (config.notrycatch) { - runTest(this); - return; - } - - try { - runTest(this); - } catch (e) { - this.pushFailure('Died on test #' + (this.assertions.length + 1) + ': ' + - (e.message || e) + '\n' + this.stack, extractStacktrace(e, 0)); - - // Else next test will carry the responsibility - saveGlobal(); - - // Restart the tests if they're blocking - if (config.blocking) { - internalRecover(this); + promise = (this.withData) + ? this.callback.call(this.testEnvironment, this.assert, this.data) + : this.callback.call(this.testEnvironment, this.assert); + } else { + try { + promise = (this.withData) + ? this.callback.call(this.testEnvironment, this.assert, this.data) + : this.callback.call(this.testEnvironment, this.assert); + } catch (e) { + this.pushFailure('Died on test #' + (this.assertions.length + 1) + ': ' + + (e.message || e) + '\n' + this.stack, extractStacktrace(e, 0)); + + // Else next test will carry the responsibility + saveGlobal(); + + // Restart the tests if they're blocking + if (config.blocking) { + internalRecover(this); + } } } - function runTest (test) { - let promise; - if (test.withData) { - promise = test.callback.call(test.testEnvironment, test.assert, test.data); - } else { - promise = test.callback.call(test.testEnvironment, test.assert); - } - test.resolvePromise(promise); - - // If the test has an async "pause" on it, but the timeout is 0, then we push a - // failure as the test should be synchronous. - if (test.timeout === 0 && test.pauses.size > 0) { - pushFailure( - 'Test did not finish synchronously even though assert.timeout( 0 ) was used.', - sourceFromStacktrace(2) - ); - } + this.resolvePromise(promise); + + // If the test has an async "pause" on it, but the timeout is 0, then we push a + // failure as the test should be synchronous. + if (this.timeout === 0 && this.pauses.size > 0) { + pushFailure( + 'Test did not finish synchronously even though assert.timeout( 0 ) was used.', + sourceFromStacktrace(2) + ); } }, @@ -222,11 +218,10 @@ Test.prototype = { }, queueGlobalHook (hook, hookName) { - const runHook = () => { + const runGlobalHook = () => { config.current = this; let promise; - if (config.notrycatch) { promise = hook.call(this.testEnvironment, this.assert); } else { @@ -244,22 +239,10 @@ Test.prototype = { this.resolvePromise(promise, hookName); }; - - return runHook; + return runGlobalHook; }, queueHook (hook, hookName, hookOwner) { - const callHook = () => { - let promise; - if (hookName === 'before' || hookName === 'after') { - // before and after hooks are called with the owning module's testEnvironment - promise = hook.call(hookOwner.testEnvironment, this.assert); - } else { - promise = hook.call(this.testEnvironment, this.assert); - } - this.resolvePromise(promise, hookName); - }; - const runHook = () => { if (hookName === 'before' && hookOwner.testsRun !== 0) { return; @@ -274,25 +257,27 @@ Test.prototype = { } config.current = this; + + // before and after hooks are called with the owning module's testEnvironment + const testEnvironment = (hookName === 'before' || hookName === 'after') + ? hookOwner.testEnvironment + : this.testEnvironment; + + let promise; if (config.notrycatch) { - callHook(); - return; - } - try { - // This try-block includes the indirect call to resolvePromise, which shouldn't - // have to be inside try-catch. But, since we support any user-provided thenable - // object, the thenable might throw in some unexpected way. - // This subtle behaviour is undocumented. To avoid new failures in minor releases - // we will not change this until QUnit 3. - // TODO: In QUnit 3, reduce this try-block to just hook.call(), matching - // the simplicity of queueGlobalHook. - callHook(); - } catch (error) { - this.pushFailure(hookName + ' failed on ' + this.testName + ': ' + - (error.message || error), extractStacktrace(error, 0)); + promise = hook.call(testEnvironment, this.assert); + } else { + try { + promise = hook.call(testEnvironment, this.assert); + } catch (error) { + this.pushFailure(hookName + ' failed on ' + this.testName + ': ' + + (error.message || error), extractStacktrace(error, 0)); + return; + } } - }; + this.resolvePromise(promise, hookName); + }; return runHook; }, @@ -757,12 +742,11 @@ Test.prototype = { resolvePromise: function (promise, phase) { if (promise !== undefined && promise !== null) { const test = this; - const then = promise.then; - if (typeof then === 'function') { + if (typeof promise.then === 'function') { const resume = test.internalStop(); const resolve = function () { resume(); }; if (config.notrycatch) { - then.call(promise, resolve); + promise.then(resolve); } else { const reject = function (error) { const message = 'Promise rejected ' + @@ -777,7 +761,11 @@ Test.prototype = { // Unblock internalRecover(test); }; - then.call(promise, resolve, reject); + // Note that `promise` is a user-supplied thenable, not per-se a standard Promise. + // This means it can be a "bad thenable" where calling then() can already throw. + // We must catch this to avoid breaking the ProcessingQueue. Promise.resolve() + // normalizes and catches even these internal errors and sends them to reject. + Promise.resolve(promise).then(resolve, reject); } } } diff --git a/test/cli/fixtures/async-test-throw.js b/test/cli/fixtures/async-test-throw.js new file mode 100644 index 000000000..6a2592139 --- /dev/null +++ b/test/cli/fixtures/async-test-throw.js @@ -0,0 +1,61 @@ +QUnit.test('throw early', async function (_assert) { + throw new Error('boo'); +}); + +QUnit.test('throw late', async function (assert) { + await Promise.resolve(''); + assert.true(true); + throw new Error('boo'); +}); + +// NOTE: This is not about testing a rejected Promise or throwing async function. +// In those cases, `ret.then(, cb)` will not throw, but inform you via cb(err). +// Instead, this is testing a bad Thenable implementation, where then() itself +// throws an error. This is not possible with native Promise, but is possible with +// custom Promise-compatible libraries and +QUnit.test('test with bad thenable', function (assert) { + assert.true(true); + return { + then: function () { + throw new Error('boo'); + } + }; +}); + +QUnit.hooks.beforeEach(function (assert) { + if (assert.test.testName !== 'example') { return; } + return { + then: function () { + throw new Error('global brocoli'); + } + }; +}); +QUnit.hooks.afterEach(function (assert) { + if (assert.test.testName !== 'example') { return; } + return { + then: function () { + throw new Error('global artichoke'); + } + }; +}); + +QUnit.module('hooks with bad thenable', function (hooks) { + hooks.beforeEach(function () { + return { + then: function () { + throw new Error('banana'); + } + }; + }); + hooks.afterEach(function () { + return { + then: function () { + throw new Error('apple'); + } + }; + }); + + QUnit.test('example', function (assert) { + assert.true(true); + }); +}); diff --git a/test/cli/fixtures/async-test-throw.tap.txt b/test/cli/fixtures/async-test-throw.tap.txt new file mode 100644 index 000000000..69f825e2b --- /dev/null +++ b/test/cli/fixtures/async-test-throw.tap.txt @@ -0,0 +1,77 @@ +# command: ["qunit", "async-test-throw.js"] + +TAP version 13 +not ok 1 throw early + --- + message: "Promise rejected during \"throw early\": boo" + severity: failed + actual : null + expected: undefined + stack: | + Error: boo + at /qunit/test/cli/fixtures/async-test-throw.js:2:9 + ... +not ok 2 throw late + --- + message: "Promise rejected during \"throw late\": boo" + severity: failed + actual : null + expected: undefined + stack: | + Error: boo + at /qunit/test/cli/fixtures/async-test-throw.js:8:9 + ... +not ok 3 test with bad thenable + --- + message: "Promise rejected during \"test with bad thenable\": boo" + severity: failed + actual : null + expected: undefined + stack: | + Error: boo + at /qunit/test/cli/fixtures/async-test-throw.js:20:13 + ... +not ok 4 hooks with bad thenable > example + --- + message: "Promise rejected before \"example\": global brocoli" + severity: failed + actual : null + expected: undefined + stack: | + Error: global brocoli + at /qunit/test/cli/fixtures/async-test-throw.js:29:13 + ... + --- + message: "Promise rejected before \"example\": banana" + severity: failed + actual : null + expected: undefined + stack: | + Error: banana + at /qunit/test/cli/fixtures/async-test-throw.js:46:15 + ... + --- + message: "Promise rejected after \"example\": apple" + severity: failed + actual : null + expected: undefined + stack: | + Error: apple + at /qunit/test/cli/fixtures/async-test-throw.js:53:15 + ... + --- + message: "Promise rejected after \"example\": global artichoke" + severity: failed + actual : null + expected: undefined + stack: | + Error: global artichoke + at /qunit/test/cli/fixtures/async-test-throw.js:37:13 + ... +1..4 +# pass 0 +# skip 0 +# todo 0 +# fail 4 + +# exit code: 1