868174 - Rejected, unhandled promises in devtools code should cause test failures

Status: RESOLVED INVALID

(DevTools :: Debugger, defect, P3)

Description

[Jim Blandy :jimb]

Attachment: Have the JS debugger register an _unhandledError function for promises.

Because bugs are rare and usually easily found, if a 'fulfilled' handler throws an exception, and the promise has no reject handler, the SDK's promise module retains the exception, without reporting it, in case a rejection handler is later added to the promise.

"Transtellar Cruise Lines would like to apologize to passengers for the continuing delay to this flight. We are currently awaiting the loading of our complement of small lemon-soaked paper napkins for your comfort, refreshment and hygiene during the journey. Meanwhile we thank you for your patience. The cabin crew will shortly be serving coffee and biscuits again." 

One consequence of this promise behavior is that errors in many handlers passed to methods of DebuggerClient and its related classes are never reported; xpcshell tests pass or time out, instead of telling me that I mistyped a function name.

Benvie has proposed an extension to our promises implementation that lets users of the promise module register a default handler, called when an exception occurs and no rejection handler is immediately available. The exception is still retained, in case a rejection handler is added later. His patch is here:
https://github.com/mozilla/addon-sdk/pull/984

Assuming Benvie's patch lands, the attached patch for the JS debugger registers an _unhandledError function for promises. It will probably register the same handler for all other users of that instance of the promises.js module, but since all the handler does is dump and Cu.reportError the exception, that should be all right.

Comment 1

[Domenic Denicola]

The summary change seems inaccurate.

Comment 2

[Eddy Bruel [:ejpbruel]]

(In reply to Domenic Denicola from comment #1)
> The summary change seems inaccurate.

How so?

From Jimb's comment:

"Benvie has proposed an extension to our promises implementation that lets users of the promise module register a default handler, called when an exception occurs and no rejection handler is immediately available. The exception is still retained, in case a rejection handler is added later."

My interpretation of this bug is that we want some kind of error message in our tests when a promise rejection isn't handled (as opposed to the test timing out), and we're trying to achieve that with a default handler that simply reports the error.

Comment 3

[Dave Camp (:dcamp)]

Jim, can you verify how this bug fits in with the general set of plans to make promise debugging better and make sure the summary reflects reality?

Comment 4

[Eddy Bruel [:ejpbruel]]

For what its worth, we have the following bugs on file for promise debugging:

981514, which is about asynchronous call stacks
1033153, which is about making the state of DOM promises inspectable

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Don't we have this already with the Promise.jsm rejection handler[1]?

Or is this referring to DOM Promises?

[1]: http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/toolkit/modules/Promise-backend.js#l250

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> Don't we have this already with the Promise.jsm rejection handler[1]?
> 
> Or is this referring to DOM Promises?
> 
> [1]:
> http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/toolkit/modules/
> Promise-backend.js#l250

We have a way to log unhandled rejections via the finalization witness service, which only happens when promises are gc'd. This is very useful when debugging, but less useful when testing, because it can cause tests to timeout without any error message.

My understanding of benvie's proposal is that we add a default rejection handler which is called immediately when a promise is rejected, but keep the exception around in case a rejection handler for the promise is installed later.

Comment 7

[Jim Blandy :jimb]

(In reply to Dave Camp (:dcamp) from comment #3)
> Jim, can you verify how this bug fits in with the general set of plans to
> make promise debugging better and make sure the summary reflects reality?

This bug is more about ensuring that rejected promises in the devtools code itself cause test failures than anything developer-facing. I've updated the bug summary.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 744812 [details] [diff] [review]
Have the JS debugger register an _unhandledError function for promises.

This patch was based on Benvie's. We now have new machinery, as jryans points out in comment 5, so this patch is obsolete.

Comment 9

[Jason Laster [:jlast]]

Closing as this is no longer relevant.