Skip to content

Commit

Permalink
Core: Fix missing second frame in QUnit.stack() in Safari
Browse files Browse the repository at this point in the history
Safari implements ES6 Tail-Call Optimization, which is when:
- the file is in strict mode,
- and, in a regular function (not async or generator),
- and, the return statement ends in a simple function call.

Then, the current function is removed from the stack before the
child function begins. TCO applies even for calls that are not
recursive.

The result is that, given:

> makeFakeFailure -> exampleMain -> exampleParent -> exampleCurrent ->
> QUnit.stack -> sourceFromStacktrace -> new Error.

In Firefox and Chrome, `e.stack` is:

```
[0] sourceFromStacktrace (SLICED)
[1] QUnit.stack          (SLICED)
[2] exampleCurrent
[3] exampleParent
[4] exampleMain
[5] makeFakeFailure
```

But, in Safari, the second frame gets lost because our tiny
`QUnit.stack()` function is a candidate for Tail-Call Optimization.

```
[0] sourceFromStacktrace (SLICED)
[1] exampleCurrent       (SLICED)
[2] exampleParent
[3] exampleMain
[4] makeFakeFailure
```

This, combined with the fact that we strip the first two frames as
a way to hide internal offsets, meant that in Safari we ended up
attributing failed assertions and test definitions to the parent of
the caller rather than the actual caller, e.g. exampleParent() instead
of exampleCurrent.

Ref https://bugs.webkit.org/show_bug.cgi?id=276187.
Closes #1776.
  • Loading branch information
Krinkle authored Jul 5, 2024
1 parent 888bece commit 52f37b8
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 35 deletions.
5 changes: 4 additions & 1 deletion src/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ extend(QUnit, {

stack: function (offset) {
offset = (offset || 0) + 2;
return sourceFromStacktrace(offset);
// Support Safari: Use temp variable to avoid TCO for consistent cross-browser result
// https://bugs.webkit.org/show_bug.cgi?id=276187
const source = sourceFromStacktrace(offset);
return source;
}
});

Expand Down
59 changes: 25 additions & 34 deletions test/main/stacktrace.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,33 @@
function fooParent () {
return fooCurrent();
}
function fooMain () {
return fooParent();
}

function fooInternal () {
function barInternal () {
return QUnit.stack(2);
}
function fooPublic () {
return fooInternal();
function barPublic () {
return barInternal();
}
function barCaller () {
return fooPublic();
function quuxCaller () {
return barPublic();
}

function norm (str) {
// CRLF
// Windows path
return str.replace(/\\/g, '/');
}

function shorten (str) {
return norm(str)
// Remove browser-specific formatting and line numbers
.replace(/^.*((?:foo|bar|quux)[A-z]+).*$/gm, '$1')
// Remove anything below our entry point
.replace(/(fooMain|quuxCaller)[\s\S\n]*/, '$1');
}

QUnit.test('QUnit.stack()', function (assert) {
var simple = norm(QUnit.stack());
assert.pushResult({
Expand All @@ -36,38 +47,18 @@
message: 'stacktrace cleaning stops before qunit.js'
});

var nested = norm(fooParent());
assert.pushResult({
result: nested.indexOf('fooCurrent') !== -1,
actual: nested,
message: 'include current function'
});
assert.pushResult({
result: nested.indexOf('fooParent') !== -1,
actual: nested,
message: 'include parent function'
});
var fooStack = shorten(fooMain()).split('\n');
assert.deepEqual(fooStack, [
'fooCurrent',
'fooParent',
'fooMain'
]);
});

QUnit.test('QUnit.stack(offset)', function (assert) {
var stack = norm(barCaller());
var line = stack.split('\n')[0];
var barStack = shorten(quuxCaller()).split('\n');

assert.pushResult({
result: line.indexOf('/main/stacktrace.js') !== -1,
actual: line,
message: 'current file'
});
assert.pushResult({
result: line.indexOf('barCaller') !== -1,
actual: line,
message: 'start at offset'
});
assert.pushResult({
result: stack.indexOf('fooInternal') === -1,
actual: stack,
message: 'skip internals'
});
assert.deepEqual(barStack, ['quuxCaller']);
});

// Some browsers support 'stack' only when caught (see sourceFromStacktrace).
Expand Down

0 comments on commit 52f37b8

Please sign in to comment.