The current Promise implementation (core.js) swallows exceptions. While we understand the reasons why it does so, we would like an optional mechanism for exception reporting - something we can use during development. I've been bitten by broken code that had exceptions swallowed by the promises. This results in spending time trying to track down obvious mistakes in code - but hard to spot without exception reporting. We would appreciate a fix for this issue. Thank you!

So say log something to the error console when the then callbacks throw?

This is a proposed patch that adds optional exception reporting to the Promise implementation. Things we've taken into consideration: 1. exception reporting cannot be enabled by default, since that could break existing code that relies on rejecting promises using exceptions. Also, for code that doesn't break, simply spewing a lot of output in the error console is not desired. 2. optional exception reporting based on a flag that is meant to be turned on only during development. 3. we only touch the jsm-related boilerplate such that other places where core.js is used are unaffected. 4. we've included a default error reporter, to make it easy for developers to toggle this capability on. 5. the error reporter can be replaced. The Cu.reportError() reporter is the preferred way, but as far as I understand from Panos, that may not be always available in all the embeddings of Gecko. Therefore, I also included a fallback to dump(). Usage: Cu.import("path/to/core.js"); Promise._reportErrors = true; Optionally you can replace the error reporter: Promise._errorReporter = yourFunction; // one argument is given: the exception Potential concern: the error reporter could be better, if, say, we would Cu.import("resource://gre/modules/devtools/Console.jsm"). console.error/debug() present objects in a far nicer way than Cu.reportError() or dump(). However, in the interest of keeping this as minimal and as acceptable as possible, I have avoided doing that. It is in our interest in this bug that we get at least some kind of exception reporting - irrespective of how nice the exceptions are presented. Please let me know if further changes are needed. Thank you!

(In reply to Dave Townsend (:Mossop) from comment #1) > So say log something to the error console when the then callbacks throw? Yes, this is our intent here.

Faced with the same issue, I have used something quite different. I have at hand a module TaskUtils (not published yet) with the following methods: - |reportErrors(promise)| - returns a promise semantically identical to |promise|, but that reports errors to Cu.reportError; - |spawn(generator)| - returns a promise as per |Task.spawn| (see Task.jsm) but that reports errors using |reportErrors|. I hope to publish this module eventually. It has also additional methods for backgrounding chains of promises using |setTimeout|-style chaining.

Comment on attachment 705452 [details] [diff] [review] proposed patch Review of attachment 705452 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/addon-sdk/promise/core.js @@ +64,5 @@ > } > catch(error) { > + if (exports._reportErrors && exports._errorReporter) { > + exports._errorReporter(error); > + } I fear that this is going to produce looooong outputs that we do not want.

Comment on attachment 705452 [details] [diff] [review] proposed patch Review of attachment 705452 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/addon-sdk/promise/core.js @@ +11,5 @@ > + var factory_export = this[factory.name] = {}; > + var Cu = this.Components.utils; > + if (Cu && Cu.reportError) { > + factory_export._errorReporter = function _errorReporter(error) { > + Cu.reportError("Promise exception: " + error + "\n" + error.stack); This is going to generate a really long console message, do you really need the whole stack? If you just did Cu.reportError(error) then the console will get the file and line number that generated the error automatically. @@ +15,5 @@ > + Cu.reportError("Promise exception: " + error + "\n" + error.stack); > + }; > + } else { > + factory_export._errorReporter = function _errorReporter(error) { > + dump("Promise exception: " + error + "\n" + error.stack); What situations will have dump but not Cu.reportError?

(In reply to David Rajchenbach Teller [:Yoric] from comment #4) > Faced with the same issue, I have used something quite different. > > I have at hand a module TaskUtils (not published yet) with the following > methods: > - |reportErrors(promise)| - returns a promise semantically identical to > |promise|, but that reports errors to Cu.reportError; > - |spawn(generator)| - returns a promise as per |Task.spawn| (see Task.jsm) > but that reports errors using |reportErrors|. > > I hope to publish this module eventually. It has also additional methods for > backgrounding chains of promises using |setTimeout|-style chaining. That sounds like a very interesting approach. However, I would favor seeing errors in promises I don't know about. For example, I'm working on the web console and inadvertently causing an exception in a promise created by the dev toolbox. I'd like to know about any new exceptions, from any promise. Having to change each promise into a promise that reports errors would be too cumbersome when working with a bigger project like the toolbox.

(In reply to Dave Townsend (:Mossop) from comment #6) > Comment on attachment 705452 [details] [diff] [review] > proposed patch > > Review of attachment 705452 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/addon-sdk/promise/core.js > @@ +11,5 @@ > > + var factory_export = this[factory.name] = {}; > > + var Cu = this.Components.utils; > > + if (Cu && Cu.reportError) { > > + factory_export._errorReporter = function _errorReporter(error) { > > + Cu.reportError("Promise exception: " + error + "\n" + error.stack); > > This is going to generate a really long console message, do you really need > the whole stack? If you just did Cu.reportError(error) then the console will > get the file and line number that generated the error automatically. I'm ok with only showing the error, without the full stack. Will change. > @@ +15,5 @@ > > + Cu.reportError("Promise exception: " + error + "\n" + error.stack); > > + }; > > + } else { > > + factory_export._errorReporter = function _errorReporter(error) { > > + dump("Promise exception: " + error + "\n" + error.stack); > > What situations will have dump but not Cu.reportError? I asked Panos now about this and he's not sure - he suggested we do not rely on Cu.reportError's presence because we are not sure in which environments jetpack's embedded. Would you be fine with always relying on Cu.reportError() being present? Thank you!

(In reply to Mihai Sucan [:msucan] from comment #8) > (In reply to Dave Townsend (:Mossop) from comment #6) > > @@ +15,5 @@ > > > + Cu.reportError("Promise exception: " + error + "\n" + error.stack); > > > + }; > > > + } else { > > > + factory_export._errorReporter = function _errorReporter(error) { > > > + dump("Promise exception: " + error + "\n" + error.stack); > > > > What situations will have dump but not Cu.reportError? > > I asked Panos now about this and he's not sure - he suggested we do not rely > on Cu.reportError's presence because we are not sure in which environments > jetpack's embedded. Would you be fine with always relying on > Cu.reportError() being present? > > Thank you! The particular part of the code you're adding to requires that String(this) contain BackstagePass. Pretty sure that's only going to be a jsm and so Cu.reportError will be available. I spoke with others about this yesterday and this seems useful enough that it might be good to have all cases be able to report exceptions like this. Probably using console.error when available then falling back to Cu.reportError when not. What do you think?

(In reply to Dave Townsend (:Mossop) from comment #9) > The particular part of the code you're adding to requires that String(this) > contain BackstagePass. Pretty sure that's only going to be a jsm and so > Cu.reportError will be available. Sounds good then! I will simplify the patch to always use Cu.reportError(). > I spoke with others about this yesterday and this seems useful enough that > it might be good to have all cases be able to report exceptions like this. > Probably using console.error when available then falling back to > Cu.reportError when not. What do you think? console.error could be even more useful. Console.jsm lives in toolkit, shouldn't it always be available? If yes, then why not always use Cu.reportError()? (in the interest of keeping the patch minimal)

(In reply to Mihai Sucan [:msucan] from comment #10) > (In reply to Dave Townsend (:Mossop) from comment #9) > > The particular part of the code you're adding to requires that String(this) > > contain BackstagePass. Pretty sure that's only going to be a jsm and so > > Cu.reportError will be available. > > Sounds good then! I will simplify the patch to always use Cu.reportError(). > > > I spoke with others about this yesterday and this seems useful enough that > > it might be good to have all cases be able to report exceptions like this. > > Probably using console.error when available then falling back to > > Cu.reportError when not. What do you think? > > console.error could be even more useful. Console.jsm lives in toolkit, > shouldn't it always be available? If yes, then why not always use > Cu.reportError()? (in the interest of keeping the patch minimal) Yeah I'm happy using Console.jsm where there isn't already a console defined (browser scope and Jetpack scope both provide a console object that does useful things).

Updated the patch to only use Console.jsm for logging (no more dump()/Cu.reportError(). Please let me know if further changes are needed. Thank you!

Comment on attachment 707574 [details] [diff] [review] updated patch Review of attachment 707574 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I probably wasn't terribly clear, I'd like it to always pass exceptions along to console.error if it exists ragardless of whether the module was loaded as a jsm or not. Make sense? ::: toolkit/addon-sdk/promise/core.js @@ +10,5 @@ > } else if (String(this).indexOf('BackstagePass') >= 0) { // JSM > + var factory_export = this[factory.name] = {}; > + var Cu = this.Components.utils; > + var console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}) > + .console; Assign to this.console @@ +57,5 @@ > return f(input); > } > catch(error) { > + if (exports._reportErrors && exports._errorReporter) { > + exports._errorReporter(error); No need for _errorReporter, just call console.error directly if it exists.

(In reply to Dave Townsend (:Mossop) from comment #13) > Comment on attachment 707574 [details] [diff] [review] > updated patch > > Review of attachment 707574 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry I probably wasn't terribly clear, I'd like it to always pass > exceptions along to console.error if it exists ragardless of whether the > module was loaded as a jsm or not. Make sense? Makes sense. Thanks! I wasn't sure what the intent was, sorry. My initial intent was to make this a "feature" only for JSMs, but it makes sense to have this working for all cases.

Updated to use console.error() whenever it's available. In JSMs we load Console.jsm so it's always available.

Perhaps there is something I don't understand with this patch, but I a problems of console pollution. Problem 1: If |foo| rejects, won't |foo.then(null, null).then(null, null).then(null, null)| print the exception three times? (possibly collapsed by |console|). Problem 2: In many cases, we do not want to catch rejections immediately. Won't this print exceptions on the |console| – exceptions that will be visible to end-users – despite the fact that these exceptions are actually caught?

(In reply to David Rajchenbach Teller [:Yoric] from comment #16) > Perhaps there is something I don't understand with this patch, but I a > problems of console pollution. > > Problem 1: If |foo| rejects, won't |foo.then(null, null).then(null, > null).then(null, null)| print the exception three times? (possibly collapsed > by |console|). Why would this happen? |foo| rejection doesn't mean console.error() is invoked. Only if |foo| throws we print the exception with console.error. Also, the exception happens only once, and it's caught in try-catch and printed. It's not re-thrown, unless .then().then... keeps re-executing |foo| multiple times. Afaik, that's not the case (unless I'm understanding things incorrectly). > Problem 2: In many cases, we do not want to catch rejections immediately. > Won't this print exceptions on the |console| – exceptions that will be > visible to end-users – despite the fact that these exceptions are actually > caught? I'm not sure I follow. attempt() in core.js wraps |f| in a try-catch block. Beyond this, there's no other code that can ever "catch" the exceptions thrown by |f|. There's only the possibility for the promise to have a rejection handler that may or may not print the rejection reason (which may be an exception), and it may or may not provide a graceful error handling for the user. This is a mechanism parallel to that. This new error reporting flag is not meant to be used in production - each promise that might fail is supposed to have a rejection handler, with graceful handling as needed for each case. The only purpose here, for this patch, is to display any possibly unhandled exceptions *during* development. There's a lot of code being written with the assumption that it will not throw exceptions - otherwise we'd wrap it in try-catch blocks and put console.errors() there (or some other case-specific solutions). A lot of the promises have no rejection handlers - unless we know of potential failures, eg. a connection timeout or something. However, mistakes happens even in code where we do not expect any exceptions or rejections. This flag is for those cases only: it makes it easier for us to spot mistakes, and where.

My bad, Problem 1 is effectively a false alarm. Now, let me elaborate on Problem 2. Consider the following snippet: foo. then(function raiser() { throw new ArithmeticError("some kind of error I know how to handle"); }). then(null, function handler(e) { if (e instanceof ArithmeticError) { // Ok, we know how to handle that, it's not a real error } // ... }); Barring any mistake, this will printout this instance of |ArithmeticError|, despite of the fact that this exception is handled (what I called "caught" in my previous comment). Worse than that, this will printout an error message to the end user. Rejecting with an exception and handling the rejection is a very common pattern, that is used pervasively with Task.jsm. > This flag is for those cases only: it makes it easier for us to spot mistakes, and where. I realize that. I believe that the lack of error reporting for promises is the biggest weakness of the lib. I'm just not convinced by the approach. In my implementation of promises for m-c, we printed out messages if errors were raised and there was no rejection handler, and this had its own set of drawbacks. I envisioned writing a more elaborate version that only printed out messages after a few seconds if there was still no rejection handler, but never got to implementing this.

(In reply to David Rajchenbach Teller [:Yoric] from comment #18) > My bad, Problem 1 is effectively a false alarm. No worries. > Now, let me elaborate on Problem 2. > > Consider the following snippet: > > ... > > I realize that. I believe that the lack of error reporting for promises is > the biggest weakness of the lib. I'm just not convinced by the approach. My thought as well - it's all fine until you have unexpected exceptions... > In my implementation of promises for m-c, we printed out messages if errors > were raised and there was no rejection handler, and this had its own set of > drawbacks. I envisioned writing a more elaborate version that only printed > out messages after a few seconds if there was still no rejection handler, > but never got to implementing this. Interesting idea. I see your point, but the "user" here is the developer. Printing the exception to the terminal should be no problem, in my opinion, knowing that case is handled gracefully. Also, as you provided the code snippet ... what happens if the promise throws an unexpected exception, not ArithmeticError? You have a rejection handler that only knows about one kind of exception and it executes silently for the rest of the unknown exceptions. Is the exception propagated from there on? There's also a matter of costs and benefits: a more complex error handling mechanism may not bring too much additional value - unless it's turned on by default, for all promises. I would like that, but until we have a smarter core.js, I believe there's to benefit even from this simpler approach: flag for error logging, to show them all. We could have a kind of convention: promises that do not have a reject handler, have their rejection reasons sent to console.error (whatever they are, exceptions or not). Whenever a reject handler is implemented it should explicitly throw if it doesn't recognize the rejection reason, such that core.js can act on that (send to console.error) - the error doesn't get lost. (my only worry is how well such conventions would work...)

Comment on attachment 710376 [details] [diff] [review] rebased patch r+ however I'm going to land this in the SDK repo on github and then it will come in on the next uplift if that's ok.

(In reply to Dave Townsend (:Mossop) from comment #21) > Comment on attachment 710376 [details] [diff] [review] > rebased patch > > r+ however I'm going to land this in the SDK repo on github and then it will > come in on the next uplift if that's ok. This is fine with me. Thank you!

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/0c8c44fe63599c1ba65916ff5e9856b3bc667072 Bug 833877: Add a way to report exceptions in promises. r=Mossop

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/719e1916965f9a9ca11ad1d3cb708a5206531005 Bug 833877: Tweak Components usage so the SDK linker doesn't reject the file. r=bustage

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/ca099a6ffef77538eb60335b2a550ab3e4de8893 Backout bug 833877 due to B2G test failures.

I've backed this out as it caused B2G reftest failures when we attempted the uplift in bug 842762

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/bf2c2f609babcea412a2349a431ae6bdbedcf05d Remove unintentional file from bug 833877 backout.

Commits pushed to integration at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/ca099a6ffef77538eb60335b2a550ab3e4de8893 Backout bug 833877 due to B2G test failures. https://github.com/mozilla/addon-sdk/commit/bf2c2f609babcea412a2349a431ae6bdbedcf05d Remove unintentional file from bug 833877 backout.

Dave: where can I see the test failures? I'm surprised this patch caused reftest failures.

(In reply to Mihai Sucan [:msucan] from comment #29) > Dave: where can I see the test failures? I'm surprised this patch caused > reftest failures. Me too. See bug 842762 comment 2. I can only assume the reftest harness is using the promises library in some place where Cu.import isn't available.

Dave, thank you for the information. I pushed an updated patch to the try server: https://tbpl.mozilla.org/?tree=Try&rev=335f5a030e95 If this goes green I'll update the patch here.

Updated the patch to be as conservative as possible. Try push looks green to me. https://tbpl.mozilla.org/?tree=Try&rev=335f5a030e95 Can you please confirm if it's green? If yes, can this land again? Thank you!

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/1b26c7568b0d09fd666d5a7bd37ecfdeb2759179 Bug 833877: Add a way to report exceptions in promises. r=Mossop

Comment on attachment 719098 [details] [diff] [review] patch 5: test fixes Looks good to me