902866 - Unhandled promise errors not reported

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

A simple example of toolkit promises:

  p = Promise.defer();
  p.promise.then((what) => {
    log("resolved: " + what);
    throw "oh no";
  });
  p.resolve("hello");

Does not report the "oh no" error anywhere.  Adding an onRejected callback would not see it either - you'd need to add another .then() and an onRejected handler to it.

If there are no onRejected handlers to take an exception, it should be reported to the console.  It shouldn't be possible to have a promise silently eat an unhandled error.

I think most uses of promises in our tree would fall victim to this.  Eg, I think errors in http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#395 would go unreported.

Bug 766078 touches on this, but I don't think it is enough to try and warn against "bad uses" - an example like this should be reported to our console.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Yes, that's a recurring issue we have with promises. Unfortunately, there is no good universal solution.

Note that this is actually case 2 of bug 766078, so let's DUP this bug and move conversation over there.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Note that this is actually case 2 of bug 766078, so let's DUP this bug and
> move conversation over there.

I referenced that in comment 0, but don't think it is specific enough and don't think a fix for this should wait until all the mentioned cases can be fixed.  This case in particular is a real and current risk to our code.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Good point. Then let's make this a dependency.

How would you go about that issue?

Comment 4

[:Paolo Amadini]

The "then" function returns a promise, and a rejection callback may throw an
exception just like a resolution callback, hence the last promise in a chain
is always, by definition, lacking a rejection callback for a potential error.

But a rejection handler may be added after the promise has been rejected,
and this may be normal in asynchronous code where promises are returned to a
caller that may register handlers a tick later, or when parallelizing tasks
using functions like Promise.all().

Since there is no general "optimal" way, the Add-on SDK is logging all exceptions
that occur in callbacks if a special debugging preference is manually set (bug
833877, and see description of bug 881047).

This may help with debugging strange behaviors and we may implement that, though
it doesn't excuse adding normal error handlers where appropriate:

https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Handling_errors_and_common_pitfalls

(In reply to Mark Hammond (:markh) from comment #0)
> I think most uses of promises in our tree would fall victim to this.  Eg, I
> think errors in
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/src/SessionStore.jsm#395 would go unreported.

Will notify Tim, thanks!

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Seems like a "Promise exception behavior public service announcement" to firefox-dev might be useful, to make sure this issue is widely understood.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is why I still think that the promise spec should have left in done() or finally() or something along those lines....

Comment 7

[David Teller [:Yoric] - still alive but not very active]

A long time ago (and a few versions of Promise ago), I had a patch that, in DEBUG mode, did the following:
- if a promise is rejected and is the last promise in the chain, launch a 1 second timer;
- when the timer is triggered, if the promise is still the last promise in the chain, Cu.reportError the error.

Would this be acceptable?

Comment 8

[Boris Zbarsky [:bzbarsky]]

That's probably something to ask on public-script-coord, which is where the spec for this more or less lives-ish.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Thanks for reopening this David!

(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> A long time ago (and a few versions of Promise ago), I had a patch that, in
> DEBUG mode, did the following:
...

I do understand the complication in having promises do this by default - an error might be thrown before the onRejected handler has been added.

So while I've currently no concrete thoughts on a solution, I don't think that we should be introducing swathes of code that swallows unhandled errors by default in regular builds, especially as that code increasingly happens on Firefox startup.

Looking over some of the code, I see onRejected used in a fraction of the promises created.  Maybe this offers a clue - a new factory (possibly even the default factory, or even an arg for the promise creation) that returns a promise that doesn't support onRejected at all?

Anyway, I'm veering off into 1/2 thought out solutions, and the solution may not live inside promises at all - but the key requirement for the Fx front-end code should be to make it almost impossible to be introducing code that swallows exceptions, even when written and reviewed by people who aren't fully aware of the subtleties in this area.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Just must have been fixed by bug 903433, bug 908955.