1498775 - Report unhandled rejection even if the promise for .then() is optimized out

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

related to bug 1342070.

If the return value of .then() or .catch() is not used, we optimize away the handling of the resulting promise, but it results in the following difference:

code A:
  Promise.resolve().then(() => { throw 1 });
result for code A:
  uncaught exception is not reported to console

code B:
  let a = Promise.resolve().then(() => { throw 1 });
result for code B:
  uncaught exception is reported to console

of course this is not something specced behavior, but reporting the exception also for code A might help developers.

Comment 1

[Ori Avtalion (salty-horse)]

I have this issue.

I'm developing an HTML game.
I have a function that loads assets that the game depends on. First thing that happens is I call loadAssets() which returns a promise.
In the page's onload event, I resolve the promise and call the main game function. So all of the game code is inside a promise.

window.addEventListener('load', function() {
  assetsLoaded.then(() => newGame());
});

Firefox silently fails on various kinds of errors, which is very unhelpful.
Chrome, however, prints out detailed errors, such as:

 a.html:15 Uncaught (in promise) TypeError: 1 is not iterable
   at newGame (a.html:15)
   at assetsLoaded.then (a.html:20)

Comment 2

[Tooru Fujisawa [:arai]]

we can detect the case here

https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/js/src/builtin/Promise.cpp#2426
> static MOZ_MUST_USE bool
> RunResolutionFunction(JSContext *cx, HandleObject resolutionFun, HandleValue result,
>                       ResolutionMode mode, HandleObject promiseObj)
> {
> ...
>     if (!promiseObj) {
>         return true;
>     }

in the optimized case, promieObj is null (because we optimized it out), and it just ignores the rejection.
we can check if the mode is RejectMode, and somehow report there.

the issue is that, `cx->runtime()->addUnhandledRejectedPromise` needs PromiseObject.

till, have we thought about this case?
how do you think about modifying the callback API to support this case?

or, do you think creating a temporary promise here makes sense and works?
I've prepared the following which seems to work, but maybe broken.

>     if (!promiseObj) {
>         if (mode == RejectMode) {
>             // The rejection will never be handled, given the returned promise
>             // is known to be unused, and already optimized away.
>             //
>             // Create temporary Promise object and reject it, in order to
>             // report the unhandled rejection.
>             //
>             // Allocation time and site are wrong but won't matter much (really?).
>             Rooted<PromiseObject*> temporaryPromise(cx);
>             temporaryPromise = CreatePromiseObjectWithoutResolutionFunctions(cx);
>             if (!temporaryPromise) {
>                 cx->clearPendingException();
>                 return true;
>             }
> 
>             return RejectPromiseInternal(cx, temporaryPromise, result);
>         }
> 
>         return true;
>     }

Comment 3

[Ori Avtalion (salty-horse)]

Should this be marked as [parity-chrome], Since Chrome does report the error?

Comment 5

[László Jánszky]

I think the current behavior is inconsistent. I mean both `new Promise` and `Promise.prototype.then` are about Promise instantiation and return a new promise object, yet the errors are reported only by the first one... I think these behaviors should be mutually exclusive, so either we report unhandled rejections by default or not, and I vote on reporting it every time, because we report uncaught errors too. Is there anything about this in the ECMAScript standard?

Comment 6

[André Bargull [:anba]]

Reporting unhandled promise rejections is implementation-defined in the ES spec: https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker

Comment 7

[Tooru Fujisawa [:arai]]

apparently this is important for ourselves :P

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Report unhandled rejection for optimized away promise.

attaching for other bug report

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 9023538 [details] [diff] [review]
Report unhandled rejection for optimized away promise.

looks like this works, and catches so many test failure :P

Changed RunResolutionFunction to lazily create the Promise object, when rejecting a promise which is optimized-out, and reject that Promise object.
as mentioned in the comment there, the information about the promise allocation time/site doesn't match to not-optimized case, but I think that won't matter much.

Comment 10

[Tooru Fujisawa [:arai]]

it turns out that it's necessary per HTML spec
  https://html.spec.whatwg.org/multipage/webappapis.html#notify-about-rejected-promises

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Report unhandled rejection for optimized away promise.

Rebased.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 9031265 [details] [diff] [review]
Report unhandled rejection for optimized away promise.

Review of attachment 9031265 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/non262/Promise/promise-rejection-tracking-optimized.js
@@ +7,5 @@
> +// If the return value of then is not used, the promise object is optimized
> +// away, but if a rejection happens, the rejection should be notified.
> +Promise.resolve().then(() => { throw 1; });
> +drainJobQueue();
> +assertEq(rejections.size, 1);

This should work:

    let [[promise, state]] = rejections;
    assertEq(state, 0);

And I suppose you could even:

    let exc;
    promise.catch(x => { exc = x; });
    assertEq(rejections.get(promise), 1);  // we handled it after all
    drainJobQueue();
    assertEq(exc, 1);  // the right exception was reported

@@ +9,5 @@
> +Promise.resolve().then(() => { throw 1; });
> +drainJobQueue();
> +assertEq(rejections.size, 1);
> +
> +rejections.clear();

This seems unnecessary.

Comment 13

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3058a4b2450317636a81badc374600e913940c0
Bug 1498775 - Report unhandled rejection for optimized away promise. r=jorendorff

Comment 14

[Raul Gurzau (:RaulG)]

Backed out changeset d3058a4b2450 (bug 1498775) for failing at promise-rejection-tracking-optimized.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2af45123d1c8888018d5b691e6a7676c4f20238

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=d3058a4b2450317636a81badc374600e913940c0

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217378181&repo=mozilla-inbound&lineNumber=8882

Log snippet: 

[task 2018-12-17T07:39:28.359Z] 07:39:28     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/Promise/promise-rejection-tracking-optimized.js
[task 2018-12-17T07:39:28.359Z] 07:39:28     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/Promise/promise-rejection-tracking-optimized.js | 526 / 6544 (8%)
[task 2018-12-17T07:39:28.375Z] 07:39:28     INFO - ++DOMWINDOW == 52 (0xdf655000) [pid = 1143] [serial = 1038] [outer = 0xf7065940]
[task 2018-12-17T07:39:28.432Z] 07:39:28     INFO - TEST-INFO |  FAILED!  ReferenceError: setPromiseRejectionTrackerCallback is not defined
[task 2018-12-17T07:39:28.434Z] 07:39:28     INFO - JavaScript error: file:///builds/worker/workspace/build/tests/jsreftest/tests/non262/Promise/promise-rejection-tracking-optimized.js, line 8: ReferenceError: setPromiseRejectionTrackerCallback is not defined
[task 2018-12-17T07:39:28.443Z] 07:39:28     INFO - [Child 1143, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 3751
[task 2018-12-17T07:39:28.474Z] 07:39:28     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/Promise/promise-rejection-tracking-optimized.js | Unknown file:///builds/worker/workspace/build/tests/jsreftest/tests/non262/Promise/promise-rejection-tracking-optimized.js:8: ReferenceError: setPromiseRejectionTrackerCallback is not defined item 1
[task 2018-12-17T07:39:28.475Z] 07:39:28     INFO - REFTEST INFO | Saved log: START file:///builds/worker/workspace/build/tests/jsrefte

Comment 15

[Tooru Fujisawa [:arai]]

thanks!
patch is fixed locally.
I'll re-land after bug 1514673 fixed.

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf8034d81325271d3fb4c68d14e018d0dbb6053
Bug 1498775 - Report unhandled rejection for optimized away promise. r=jorendorff

Comment 17

[Eliza Balazs [:ebalazs_]]

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