Rejected, unhandled promises in devtools code should cause test failures

NEW
Unassigned

Status

()

Firefox
Developer Tools: Debugger
P3
normal
5 years ago
3 years ago

People

(Reporter: jimb, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 744812 [details] [diff] [review]
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.
(Reporter)

Updated

5 years ago
Depends on: 868278
Priority: -- → P3

Updated

3 years ago
Summary: JS Debugger: promises should log unhandled rejections → Implement a default handler for rejected promises

Comment 1

3 years ago
The summary change seems inaccurate.
(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

3 years ago
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?
Flags: needinfo?(jimb)
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
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
(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.
(Reporter)

Comment 7

3 years ago
(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.
Flags: needinfo?(jimb)
Summary: Implement a default handler for rejected promises → Rejected, unhandled promises in devtools code should cause test failures
(Reporter)

Comment 8

3 years ago
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.
Attachment #744812 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.