1646017 - Perma JavaScript error: file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/shell.js, line 127: TypeError: Assertion failed: got "false", expected "true": [stack: ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Treeherder Bug Filer]

Filed by: archaeopteryx [at] coole-files.de
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=306271131&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/XaUUidPIQqKsSOb6AEuOgQ/runs/0/artifacts/public/logs/live_backing.log
Reftest URL: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/XaUUidPIQqKsSOb6AEuOgQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

---

These error messages are returned for jsreftests since bug 1601179 landed. The tasks containing the runs have the state of success.

[task 2020-06-14T03:50:53.305Z] 03:50:53 INFO - REFTEST TEST-START | js/src/tests/non262/Promise/any-stack.js
[task 2020-06-14T03:50:53.307Z] 03:50:53 INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/jsreftest.html?test=non262/Promise/any-stack.js | 728 / 12522 (5%)
[task 2020-06-14T03:50:53.381Z] 03:50:53 INFO - JavaScript error: file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/shell.js, line 127: TypeError: Assertion failed: got "false", expected "true": [stack: ]
[task 2020-06-14T03:50:53.382Z] 03:50:53 INFO - JavaScript error: file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/shell.js, line 127: TypeError: Assertion failed: got "false", expected "true": [stack: ]
[task 2020-06-14T03:50:53.432Z] 03:50:53 INFO - REFTEST TEST-PASS | js/src/tests/non262/Promise/any-stack.js | item 1

Comment 1

[Logan Smyth [:loganfsmyth]]

Alright, this appears to be an existing issue with this test, but my code slightly changed the code to expose the issue.

This test, before my patch, assumed that the javascript.options.asyncstack pref was on. That means that even before https://bugzilla.mozilla.org/show_bug.cgi?id=1601179 landed, this test would have failed the same way on beta and stable.

https://bugzilla.mozilla.org/show_bug.cgi?id=1601179 changed the code so that the pref on its own is not enough for this test to pass, exposing the issue on Nightly and Dev edition too.

I don't actually know what stack would be expected for AggregateError other than the empty string we're seeing here when async stack collection is not enabled.

Comment 2

[Logan Smyth [:loganfsmyth]]

Jason, not sure what the actual expected behavior here should be. I suppose we could force Promise.any to always capture promise allocation information independent of the option.

Comment 3

[Logan Smyth [:loganfsmyth]]

I realize I should add more of a clear explanation to this. Essentially if you run the following:

data:text/html,<script>Promise.any([Promise.reject(0)]).catch(e => alert(e.stack));</script>

on Nightly now it will be an empty string because of my patch, but before my patch, if you turned off javascript.options.asyncstack you'd also get an empty string.

An empty string stack seems awful and is presumable something we'd never want?

Comment 4

[Jason Orendorff [:jorendorff]]

I'm OK with capturing an async stack here, or not.

An AggregateError will have an .errors field, containing more errors, which will have their own stacks... except in the case of Promise.any([]); but happily that does capture a stack, because in that case the AggregateError is created synchronously (while the stack exists).

Promise.reject(0) is already an error with no stack, I think.

In SM's defense, the AggregateError is in fact created when there is nothing on the stack. :-\ Are there no other cases where this happens? I'm a little surprised.

Comment 5

[Logan Smyth [:loganfsmyth]]

Looking into this and at other examples of similar promise-like APIs, I think an empty string here is the correct thing to return, so I'll correct the test.

I think there's a broader conversation that could be had here around whether async frames actually belong in the .stack serialization, and if so, how consistently, for example

data:text/html,<script>fetch("https://fake.com").catch(err => { alert(err.stack); });</script>

prints an empty string even though technically the promise returned by fetch would capture an async trace because fetch does not attempt to use that trace when throwing it's error. Were an error thrown by a fetch implementation written entirely in JS wrapping a callback-based API like XMLHttpRequest likely would have included async frames as a side-effect of normal promise/callback-based async frame data storage.

Comment 6

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1646017 - Promise.any errors should not assume async stack data is available. r=jorendorff!

This test assumed that AggregateError stack traces would include frames from
across the asynchronous boundary, which is not guaranteed to be the case. This
test originally passed because Promise.any was only enabled on Nightly and
the Nightly release had the "javascript.options.asyncstack" pref enabled by
default, which guaranteed that async frames were guaranteed to be captured.

Now that bug 1601179 has landed, this is no longer the case because the realm
is required to be a debuggee before async frames will be included in the trace.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b99ddeb9dcb3
Promise.any errors should not assume async stack data is available. r=jorendorff,anba

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/b99ddeb9dcb3