908955 - Eagerly dump coding exceptions raised in Promise callbacks

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: promise-error

I ran into this problem today. I probably spent about an hour adding dump statements everywhere before I discovered that a JavaScript error (accessing a property on a null object) was being thrown while executing a promise. I guess you're supposed to always end a promise with promise.then(null, Cu.reportError), but some code doesn't do that.

I see now that bug 903433 has already been filed on this. In my opinion, reporting the error when the promise is garbage collected is not sufficient. The garbage collector might not run for many seconds. It's often very important during debugging to see errors as they occur so you can see how they interleave with other print statements.

We can't just report exceptions eagerly because throwing an exception is supposed to be treated as rejecting the promise. That's a big mistake in my opinion, but it's in several specs for promises. Instead, this patch reports only exceptions that are likely to represent coding errors (rather than attempts to reject the promise): EvalError, RangeError, ReferenceError, and TypeError. Also, the patch doesn't change the semantics of the promise at all; it just does some extra dumping, which only shows up if you have the right pref set.

Ideally, we could check the type of the exception using instanceof. However, the exception belongs to a different global, so it EvalError/RangeError/etc. prototypes. So instead this patch checks the name.

Comment 1

[:Paolo Amadini]

Comment on attachment 794970 [details] [diff] [review]
promise-error

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

I think this is a good idea, because:
- It is limited to errors that can be fixed by changing the code
- We have access to all the code that we might need to change to fix the error
- We are using "dump", that is a type of reporting that doesn't appear in the Console

The latter is particularly important for add-ons, that might be using external libraries and be unable to fix the error.

Please specify all of the above in a thorough code comment, so that people can figure out why we are doing this, without reading this bug.

Note that this will cause some redundant output, especially in "Task.jsm" functions with coding errors and correct handlers, but these can be fixed by changing the code.

The dump message should be way more explicit, with a documentation link:

A coding exception was thrown in a Promise callback registered using "then":\n
[ex.toString())]\n
If this error is not reported to the Console also, a downstream rejection callback might be missing.\n
See <https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise>.\n
See the following stack:\n
[ex.stack]

Comment 2

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

Comment on attachment 794970 [details] [diff] [review]
promise-error

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

I like the idea.
I would prefer using Cu.reportError, though.

Comment 3

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

Bill, are you planning to finish that patch soon or would you prefer me to finish it?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I probably won't get to this today and I'm on PTO next week. I'd appreciate it if you want to take it on.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

By the way, I discovered that it would be nice to have something similar in Task.jsm if an exception is thrown.

Comment 6

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> I like the idea.
> I would prefer using Cu.reportError, though.

See comment 1 for the rationale on why this should not report errors in release
builds. Also, a correct error handler on the promise would still report the same
error to the Console (unless it is caught for some reason).

(In reply to Bill McCloskey (:billm) from comment #5)
> By the way, I discovered that it would be nice to have something similar in
> Task.jsm if an exception is thrown.

This change also affects exception handling in Task.jsm.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Paolo Amadini from comment #6)
> This change also affects exception handling in Task.jsm.

Why do you think that? Exceptions are handled here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm#227

In the patch I posted, reject doesn't print anything. We could perhaps change that, but I'm not sure it's a good idea.

Also, as an aside, I think that the handling of StopIteration in Task.jsm is broken. I haven't tested it, but it seems like it has the same problem with multiple globals as I mentioned in comment 0. Task.jsm is doing an instanceof test against its own StopIteration while the code that throws will be using a local copy from its own global scope.

Comment 8

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

(In reply to :Paolo Amadini from comment #1)
> Comment on attachment 794970 [details] [diff] [review]
> promise-error
> 
> Review of attachment 794970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is a good idea, because:
> - It is limited to errors that can be fixed by changing the code
> - We have access to all the code that we might need to change to fix the
> error
> - We are using "dump", that is a type of reporting that doesn't appear in
> the Console
> 
> The latter is particularly important for add-ons, that might be using
> external libraries and be unable to fix the error.

I am not sure about that point. Given that our intention is to report programmer errors, if (ideal case) the programmers produce correct code, the error messages will never reach the user. Alternatively, if (realistic case) the code has bugs, we want to increase likelihood of bug reports, don't we?

> A coding exception was thrown in a Promise callback registered using
> "then":\n
> [ex.toString())]\n
> If this error is not reported to the Console also, a downstream rejection
> callback might be missing.\n

I believe that this obscures the message. Our real intention here is to warn the developer of a coding error, so that they can fix it, not report them better.

Comment 9

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

Attachment: Reporting coding errors in Promise.jsm and Task.jsm

Comment 10

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

Incidentally, I just filed bug 911799 for cross-module exception matching.

Comment 11

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

Attachment: Reporting coding errors in Promise.jsm and Task.jsm, v2

Forgot the qref.

Comment 12

[:Paolo Amadini]

(In reply to Bill McCloskey (:billm) from comment #7)
> Why do you think that? Exceptions are handled here:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm#227

You're right, sorry for the noise. It's fine to handle these notifications
in Task.jsm too.

> Also, as an aside, I think that the handling of StopIteration in Task.jsm is
> broken. I haven't tested it, but it seems like it has the same problem with
> multiple globals as I mentioned in comment 0. Task.jsm is doing an
> instanceof test against its own StopIteration while the code that throws
> will be using a local copy from its own global scope.

It seems likely that this doesn't apply here because the exception is thrown by
the JavaScript runtime, maybe using the global scope of the function calling
the generator. Otherwise, I don't see how this could have worked until now, in
all the places that use Task.jsm and terminate the task normally.

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> > The latter is particularly important for add-ons, that might be using
> > external libraries and be unable to fix the error.
> 
> I am not sure about that point. Given that our intention is to report
> programmer errors, if (ideal case) the programmers produce correct code, the
> error messages will never reach the user. Alternatively, if (realistic case)
> the code has bugs, we want to increase likelihood of bug reports, don't we?

The point is that the programmer may not be able to debug or fix the issue, for
example because the module they're using is minified. And we don't want to
promote a general practice of always catching and ignoring exceptions in each
individual callback. Conversely, a single, non-redundant rejection handler
taking action at the end of the promise chain is the correct solution here.

> > A coding exception was thrown in a Promise callback registered using
> > "then":\n
> > [ex.toString())]\n
> > If this error is not reported to the Console also, a downstream rejection
> > callback might be missing.\n
> 
> I believe that this obscures the message. Our real intention here is to warn
> the developer of a coding error, so that they can fix it, not report them
> better.

I think the message is useful as an occasion to promote correct error handling
in promises, but I don't have any strong feeling about keeping or removing it.

Comment 13

[:Paolo Amadini]

Comment on attachment 798588 [details] [diff] [review]
Reporting coding errors in Promise.jsm and Task.jsm, v2

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

Looks good, with the changes below (some nits optional). I'd also like to have a quick glance at the next version before check-in.

::: toolkit/modules/Promise.jsm
@@ +567,5 @@
> +
> +      if (ex &&
> +          typeof ex == "object" &&
> +          "name" in ex &&
> +          ERRORS_TO_REPORT.indexOf(ex.name) != -1) {

nit: I'd prefer seeing the first three expressions on one line.

@@ +571,5 @@
> +          ERRORS_TO_REPORT.indexOf(ex.name) != -1) {
> +        // We suspect that the exception is a programmer error, so we
> +        // now display it using dump().  Note that we do not use
> +        // Cu.reportError as we assume that this is a programming
> +        // error, so we do not want end users to witness it.

Also, I'd add that correct promise error handling would report the error anyways.

nit: you can use up to the 80th character of the line for comment text.

@@ +576,5 @@
> +
> +        dump("A coding exception was thrown in a Promise " +
> +             ((nextStatus == STATUS_RESOLVED) ? "resolution":"rejection") +
> +             " handler.\n");
> +        dump("This is probably a programming error.\n");

We don't use the term "handler" in the documentation, use "callback" instead.

The line "This is probably a programming error." doesn't add any information unless you've read this bug first, thus it can be removed.

@@ +579,5 @@
> +             " handler.\n");
> +        dump("This is probably a programming error.\n");
> +        dump("Full message: " + ex + "\n");
> +        dump("See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise\n");
> +        dump("Full stack: " + ex.stack + "\n");

Is "stack" always defined for the types of exception we handle here?

(All of the above applies to the Task.jsm handler too.)

::: toolkit/modules/Task.jsm
@@ +233,5 @@
> +        // now display it using dump().  Note that we do not use
> +        // Cu.reportError as we assume that this is a programming
> +        // error, so we do not want end users to witness it.
> +
> +        dump("A coding exception was thrown in a Task\n");

nit: full stop at end of message

Comment 14

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

(In reply to :Paolo Amadini from comment #13)
> > +        dump("Full stack: " + ex.stack + "\n");
> 
> Is "stack" always defined for the types of exception we handle here?

It should be, but you are right, let's not fragilize the code by assuming this.

Comment 15

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

Attachment: Reporting coding errors in Promise.jsm and Task.jsm, v3

Comment 16

[:Paolo Amadini]

Comment on attachment 798796 [details] [diff] [review]
Reporting coding errors in Promise.jsm and Task.jsm, v3

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

Sorry for the delay!

::: toolkit/modules/Task.jsm
@@ +236,5 @@
> +
> +        dump("A coding exception was thrown and uncaught in a Task.\n");
> +        dump("Full message: " + ex + "\n");
> +        dump("Full stack: " + (("stack" in ex)?ex.stack:"not available") + "\n");
> +        }

typo?

Comment 17

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

Attachment: Reporting coding errors in Promise.jsm and Task.jsm, v4

Fixed typo.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6628a6169cc7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8a10f6868072

Comment 19

[Eric Shepherd [:sheppy]]

Sounds likely to be useful to mention stuff like this to devs.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a10f6868072