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 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 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.

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

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.

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

(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.

(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.

(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.

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

Forgot the qref.

(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 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

(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 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?

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

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