Closed Bug 989960 Opened 11 years ago Closed 9 years ago

Unhandled rejections in DOM Promises should cause xpcshell tests to fail (like Promise.jsm)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

Attachments

(1 file)

MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric
(deleted), text/x-review-board-request
Yoric
: review+
Details 
Bug 966452 is about reporting unhandled rejections in a DOM Promise to the console on garbage collection. When an automated test terminates, however, these errors may be discarded instead of causing the test to fail.

With the technique implemented in bug 976205, it is possible to detect those errors at test termination time, and make the test fail.
Blocks: 976205, asyncfailures
No longer depends on: 976205
> With the technique implemented in bug 976205, it is possible to detect those errors at
> test termination time, and make the test fail.

So the basic idea is that we have something that keeps track of all promises (for a given window) and then we call some function at test termination time that walks all of them and fails the test if there are any live rejected promises with no rejection handlers, right?
I expect we want to track all promises, not just those for a given window, because tests can use multiple windows.

We probably only want to do this when some pref is set or something...
Also, not sure what to do about unhandled promise rejections in workers.  Handling those might be pretty tough.  :(
(In reply to Boris Zbarsky [:bz] from comment #1)
> So the basic idea is that we have something that keeps track of all promises
> (for a given window) and then we call some function at test termination time
> that walks all of them and fails the test if there are any live rejected
> promises with no rejection handlers, right?

Yes.

(In reply to Boris Zbarsky [:bz] from comment #2)
> We probably only want to do this when some pref is set or something...

Since we need to keep track of those for handling at GC time, there is probably no additional cost, so no need to keep this behind a preference, in my opinion.
> Since we need to keep track of those for handling at GC time

Fwiw, I think this is making some assumption that happens to not be true.  We currently have no global list of promises anywhere.
(In reply to Boris Zbarsky [:bz] from comment #5)
> Fwiw, I think this is making some assumption that happens to not be true. 
> We currently have no global list of promises anywhere.

A list of rejected promises that have no rejection handlers registered will probably be needed for bug 966452, unless you can suggest a better approach. I'm saying that the same list can be browsed explicitly after a test is executed, to ensure these are reported during tests.

This is why I've indicated bug 966452 as a dependency of this one.
> will probably be needed for bug 966452

Not at all; promises can just self-report in their destructor.  That's what they do right now already.
Oh, and the reason I'd like to avoid a global list of promises is because it would be slow and fragile, given workers.

We could keep per-thread lists in TLS, which would at least only be slow-ish, without the fragility, but if we want unhandled promises in workers to cause test failures we'll need to figure out how to make that work...
We should review https://github.com/domenic/unhandled-rejections-browser-spec since that proposes to have a global list of rejected promises and various other things.
(In reply to Anne (:annevk) from comment #9)
> We should review
> https://github.com/domenic/unhandled-rejections-browser-spec since that
> proposes to have a global list of rejected promises and various other things.

Cool, we should have a bug to track our alignment with the spec and maybe provide feedback based on our implementation.

By the way, David, what is the current state of this bug? I see all dependencies as resolved and this is the last blocker for bug 939636.
Flags: needinfo?(dteller)
I filed bug 1179244 to track implementation of that specification.
Well, I'd like the more generic bug 1080457 done before I work on that. And that bug is blocked on review.
Flags: needinfo?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #12)
> Well, I'd like the more generic bug 1080457 done before I work on that. And
> that bug is blocked on review.

It seems it will take quite some time to get that done. In the meantime, cannot we just add calls to the DOM Promise hooks where we currently call the Promise.jsm hooks? What is the area of the code that has to be changed, and the API functions we need to call?

This would allow use to remove Promise.jsm completely so we don't have to maintain two implementations, for example the tweaks required for async stacks.
Flags: needinfo?(dteller)
I agree that we can add calls to PromiseDebugger. Unfortunately, I do not have much time to look at this. By any chance, do you have a chance to handle this?
Flags: needinfo?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #14)
> I agree that we can add calls to PromiseDebugger. Unfortunately, I do not
> have much time to look at this. By any chance, do you have a chance to
> handle this?

Probably for the next release. Where do I find the new API calls I have to use?
Flags: needinfo?(dteller)
I would imagine David was thinking about bug 1083361:
  https://hg.mozilla.org/mozilla-central/rev/331d71cabe1e#l8.2
With PromiseDebugging object exposing a new `addUncaughtRejectionObserver` function that allows to track consumed/uncaught promises.
Indeed, as ochameau says.
Flags: needinfo?(dteller)
Better late than never! I've posted a patch that adds a shared module to detect both Promise.jsm and DOM Promise rejections with a single interface, initially used by the xpcshell infrastructure.

Browser-chrome tests will follow using the same infrastructure, although we may need to the adjust the ignore mechanism for blanket "thisTestLeaksUncaughtRejectionsAndShouldBeFixed" usages that are currently around the tree.

I'm running a try build to see how many uncaught DOM Promise rejections we may have to fix or whitelist in xpcshell tests, hopefully not too many.
You may want to coordinate with Till in terms of bug 911216 (aka DOM promises more or less going away).
Oh, I see, you saw that bug already.  Never mind me.
Assignee: nobody → paolo.mozmail
Ahaha, that's a patch I was waiting for! I'll try and review this by tomorrow.
Comment on attachment 8709410 [details]
MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric

https://reviewboard.mozilla.org/r/31429/#review28307

Good v1 :)
Thanks for picking this up, this has been an itch for a long time.

::: testing/xpcshell/head.js
(Diff revision 1)
> -  _Promise.Debugging.flushUncaughtErrors();

Mmmh... So you don't need to flush here anymore?

::: testing/xpcshell/head.js:502
(Diff revision 1)
> -  _Promise.Debugging.clearUncaughtErrorObservers();
> +  _PromiseTestUtils.initialize();

That's definitely cleaner.

::: testing/xpcshell/head.js:613
(Diff revision 1)
> +    // It's important to terminate the module avoid crashes on shutdown.

Nit: "terminate the module _to_ avoid crashes".

::: toolkit/modules/tests/PromiseTestUtils.jsm:25
(Diff revision 1)
> +   * Array of objects containing the details of the Promise rejections that are

Nit: Could you document whether these are only DOM Promise or also JSM Promise.

Also, documenting the structure of the objects (or even creating a constructor for these objects) would be useful.

::: toolkit/modules/tests/PromiseTestUtils.jsm:32
(Diff revision 1)
> +   * When an uncaught rejection is detected, it is ignored if one of the

Good idea.

::: toolkit/modules/tests/PromiseTestUtils.jsm:45
(Diff revision 1)
> +  initialize() {

Nit: I think we generally call these `init()`.

::: toolkit/modules/tests/PromiseTestUtils.jsm:65
(Diff revision 1)
> +  terminate() {

Nit: `uninit()`?

::: toolkit/modules/tests/PromiseTestUtils.jsm:83
(Diff revision 1)
> +    let message = "Unable to convert rejection reason to string.";

Nit: I believe it would be slightly clearer if it was `"(Unable ... )"` (with parentheses).

::: toolkit/modules/tests/PromiseTestUtils.jsm:102
(Diff revision 1)
> +    let index = this._rejections.findIndex(rejection => rejection.id == id);

I realize that we shouldn't have many uncaught promises hanging, so this linear algorithm should be ok. Could you document this, though?

::: toolkit/modules/tests/PromiseTestUtils.jsm:103
(Diff revision 1)
> +    if (index != -1) {

I assume that we could have `index == -1` if this module gets initialized while some instances of `Promise` are already in memory. Could you document this?

::: toolkit/modules/tests/PromiseTestUtils.jsm:115
(Diff revision 1)
> +  expectUncaughtRejection(messageSubstring) {

I'd rather we registered either a regexp or a function. I believe that it would be more generic and more useful.

::: toolkit/modules/tests/PromiseTestUtils.jsm:138
(Diff revision 1)
> +        this._rejectionIgnoreFns.splice(index, 1);

Here and in `assertNoMoreExpectedRejections`, you're implementing rejections that _must_ appear. I'd prefer if we also had rejections that _may_ appear, which I believe will be more common.

::: toolkit/modules/tests/PromiseTestUtils.jsm:142
(Diff revision 1)
> +      // Report the error. This operation can throw an exception, depending on

This handles only the case in which we have only a single rejection. Could you handle more?

::: toolkit/modules/tests/PromiseTestUtils.jsm:157
(Diff revision 1)
> +  assertNoMoreExpectedRejections() {

It might be useful to be able to unregister ignores.
Maybe in a followup bug.
Attachment #8709410 - Flags: review?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #23)
> > -  _Promise.Debugging.flushUncaughtErrors();
> 
> Mmmh... So you don't need to flush here anymore?

It happens nearly at the same time of the test execution, only in the more sensible place after the call to _do_main().

> > +  initialize() {
> 
> Nit: I think we generally call these `init()`.
> 
> > +  terminate() {
> 
> Nit: `uninit()`?

Heh, consistency wins here. While I prefer the former I see the latter is used much more frequently in Toolkit. We still use the word "_initialized" to track the state internally, though.

As a side note, when searching for "opposite of initialize" the first result I got was a page that said that "this question will likely solicit debate, arguments, polling, or extended discussion" ;-)

> Here and in `assertNoMoreExpectedRejections`, you're implementing rejections
> that _must_ appear. I'd prefer if we also had rejections that _may_ appear,
> which I believe will be more common.

I'll see what cases we'll have in practice, this would mostly affect rejections that may or may not happen due to varying code paths or due to being fired near the end of the test. The timing behavior of Promises is deterministic, so we may actually have few or none of these cases.

> ::: toolkit/modules/tests/PromiseTestUtils.jsm:142
> (Diff revision 1)
> > +      // Report the error. This operation can throw an exception, depending on
> 
> This handles only the case in which we have only a single rejection. Could
> you handle more?

What I've noticed is that since we enter this function again a few times during test shutdown, we may actually have more than one Promise reported for xpcshell tests. This should be enough to debug most tests.

I find it a little bit sub-optimal that Assert.jsm assertions may or may not be fatal based on the test suite. We may improve the situation by adding generic failure reporting functions that never throw, but I think it should be done in a separate bug.

> ::: toolkit/modules/tests/PromiseTestUtils.jsm:157
> (Diff revision 1)
> > +  assertNoMoreExpectedRejections() {
> 
> It might be useful to be able to unregister ignores.
> Maybe in a followup bug.

Yeah, let's wait for an actual need before implementing that.
Comment on attachment 8709410 [details]
MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31429/diff/1-2/
Attachment #8709410 - Attachment description: MozReview Request: Bug 989960 - Part 1 of 2 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric → MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric
Attachment #8709410 - Flags: review?(dteller)
Officially moved the browser-chrome part to bug 1242505 so we can land this one as soon as it's reviewed and all tests succeed in automation.
Summary: Unhandled rejections in DOM Promises should cause tests to fail (like Promise.jsm) → Unhandled rejections in DOM Promises should cause xpcshell tests to fail (like Promise.jsm)
Depends on: 1242968
Comment on attachment 8709410 [details]
MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31429/diff/2-3/
I've updated more code and whitelisted more tests, and hopefully solved the detection of pending unhandled rejections on shutdown in a more elegant way than the fixed number of event loop ticks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=538bd5310c4b
In the future, it would be great if you could reply on MozReview to comments made on MozReview :)
(In reply to :Paolo Amadini from comment #24)
> As a side note, when searching for "opposite of initialize" the first result
> I got was a page that said that "this question will likely solicit debate,
> arguments, polling, or extended discussion" ;-)

:)

> > Here and in `assertNoMoreExpectedRejections`, you're implementing rejections
> > that _must_ appear. I'd prefer if we also had rejections that _may_ appear,
> > which I believe will be more common.
> 
> I'll see what cases we'll have in practice, this would mostly affect
> rejections that may or may not happen due to varying code paths or due to
> being fired near the end of the test. The timing behavior of Promises is
> deterministic, so we may actually have few or none of these cases.

I'm not as optimistic as you are, but fair enough. Just make it very clear in the doc that it's a _must_ and not a _may_.

> > ::: toolkit/modules/tests/PromiseTestUtils.jsm:142
> > (Diff revision 1)
> > > +      // Report the error. This operation can throw an exception, depending on
> > 
> > This handles only the case in which we have only a single rejection. Could
> > you handle more?
> 
> What I've noticed is that since we enter this function again a few times
> during test shutdown, we may actually have more than one Promise reported
> for xpcshell tests. This should be enough to debug most tests.

Still a bit weird. I realize that showing the first test failure is probably sufficient for each test, but could you document this?


> I find it a little bit sub-optimal that Assert.jsm assertions may or may not
> be fatal based on the test suite. We may improve the situation by adding
> generic failure reporting functions that never throw, but I think it should
> be done in a separate bug.

Fair enough.

> 
> > ::: toolkit/modules/tests/PromiseTestUtils.jsm:157
> > (Diff revision 1)
> > > +  assertNoMoreExpectedRejections() {
> > 
> > It might be useful to be able to unregister ignores.
> > Maybe in a followup bug.
> 
> Yeah, let's wait for an actual need before implementing that.

Sounds good.
Comment on attachment 8709410 [details]
MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric

https://reviewboard.mozilla.org/r/31429/#review29169

Minor stuff, but could you walk through all the issues left open and make sure that you have addressed them? It's pretty much documentation.

::: dom/push/PushService.jsm:461
(Diff revision 3)
> -      });
> +      }).catch(Cu.reportError);

So you need to swallow the error here? Could you add a comment?

Also, `Cu.reportError` is still pretty **** at reporting errors. Any chance you could use `console.error`?

::: dom/push/PushServiceHttp2.jsm:731
(Diff revision 3)
> +                .catch(Cu.reportError);

As above.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:2104
(Diff revision 3)
> +          deferSaveComplete.promise.catch(() => {});

Here and everywhere else, could you document why you swallow the error?

::: toolkit/modules/tests/PromiseTestUtils.jsm:108
(Diff revision 3)
> +      Services.tm.mainThread.processNextEvent(true);

I suspect that an infinite loop is possible here. That's mostly ok (it's just going to timeout the test), but could you document it?

Do you think it would also be possible/useful to add some indicator to the number of promise that are still pending, with a countdown every e.g. 100 iterations of `processNextEvent`?

::: toolkit/modules/tests/PromiseTestUtils.jsm:141
(Diff revision 3)
> +    // It's important that we don't sore any reference to the provided Promise

Nit: "store".

::: toolkit/modules/tests/PromiseTestUtils.jsm:165
(Diff revision 3)
> +   * Ignores one uncaught rejection so that tests will not fail because of its

Well, that's not exactly true. The test will also fail if the rejection is not pending.

Could you update the doc to make it clear that the rejection *must* appear? Also, could you make it clearer that the method must be called once for each *instance* of each expected rejection?
Attachment #8709410 - Flags: review?(dteller) → review+
https://reviewboard.mozilla.org/r/31429/#review29169

> So you need to swallow the error here? Could you add a comment?
> 
> Also, `Cu.reportError` is still pretty crappy at reporting errors. Any chance you could use `console.error`?

As far as I remember, by default in JSM files Cu.reportError works while console.error doesn't.

In fact, these statements change nothing about the code flow, we're just reporting the errors differently.

> Here and everywhere else, could you document why you swallow the error?

I don't think we need to add a comment to every place where we catch errors - what's going on mechanically is pretty obvious from the code.

The reason why errors are not propagated is useful to document, but probably should have been clarified by the owner of the code in the first place if it was necessary.

(For this specific Downloads.jsm case, I'll add a comment since it's the only place where the flow is a bit special, and I know what's going on.)

> I suspect that an infinite loop is possible here. That's mostly ok (it's just going to timeout the test), but could you document it?
> 
> Do you think it would also be possible/useful to add some indicator to the number of promise that are still pending, with a countdown every e.g. 100 iterations of `processNextEvent`?

An infinite loop shouldn't happen because we post the promise we're waiting for. This function should always resolve in a few ticks.
Comment on attachment 8709410 [details]
MozReview Request: Bug 989960 - Unhandled rejections in DOM Promises should cause xpcshell tests to fail. r=Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31429/diff/3-4/
Everything looks good in the try build except for an intermittent failure in the whitelisted test "dom/push/test/xpcshell/test_registration_success_http2.js". This is because the expected rejections sometimes don't happen. Maybe waiting one more tick before the test ends might make the rejections happen consistently, I've started a new build to verify:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8481ff92180

I think it's better to try and handle that case specifically rather than having a function to declare that a rejection may or may not occur. Ideally we should have the Push Notifications and the database code catch all the rejections it may generate, but I haven't figured out where that happens.

If this doesn't solve the failure, I think the next best option is to implement a generic thisTestLeaksUncaughtRejectionsAndShouldBeFixed that disables the checks.
(In reply to :Paolo Amadini from comment #34)
> If this doesn't solve the failure, I think the next best option is to
> implement a generic thisTestLeaksUncaughtRejectionsAndShouldBeFixed that
> disables the checks.

I went ahead and implemented this solution. I'll land it if tests are green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06ad48221429
Let's try disabling the DOM Promise observers by whitelisting the watchdog tests. New try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47ec6bdc71e

Bobby, Nick, there's an intermittent crash at NS_CycleCollectorSuspect3:

https://treeherder.mozilla.org/logviewer.html#?job_id=6925318&repo=fx-team

This only seems to happen in this test where the script is terminated by the watchdog and there is an uncaught rejection observer registered. I've observed crashes at shutdown when calls to PromiseDebugging.addUncaughtRejectionObserver and PromiseDebugging.removeUncaughtRejectionObserver are mismatched, but I'm not sure whether it's related.

Ideas? If disabling the observers works I'll land that as a workaround, but we may file a separate bug than bug 1077403 if we know or suspect the cause of the crash.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(bobbyholley)
(In reply to :Paolo Amadini from comment #38)

Maybe :mccr8 knows something about that intermittent crash in the cycle collector?
Flags: needinfo?(nfitzgerald) → needinfo?(continuation)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #41)
> Maybe :mccr8 knows something about that intermittent crash in the cycle
> collector?

It looks like some cycle collected object is being released after the cycle collector has shut down. The line numbers aren't really great in there, but it looks like the release starts with a call to nsTArray::Clear() from ~CycleCollectedJSRuntime. I see three arrays in that class that look like they could be related to Promises: mUncaughtRejections, mConsumedRejections and mUncaughtRejectionObservers. It might help to manually clear all three of those arrays at the start of ~CycleCollectedJSRuntime(), before the cycle collector shutdown begins.
Flags: needinfo?(continuation)
I don't have any particular insight to add here.
Flags: needinfo?(bobbyholley)
side note, bug 1244723 will solve some of the uncaught exceptions that were whitelisted here (in webextensions)... I just noticed cause I patched on top of this fix that then was backed out...
(In reply to Marco Bonardo [::mak] from comment #44)
> side note, bug 1244723 will solve some of the uncaught exceptions that were
> whitelisted here (in webextensions)... I just noticed cause I patched on top
> of this fix that then was backed out...

And that was test_webextensions.js, test_update_webextensions.js and test_webextension_icons.js
(In reply to Marco Bonardo [::mak] from comment #45)
> And that was test_webextensions.js, test_update_webextensions.js and
> test_webextension_icons.js

Yeah, I figured out it should have raced with another bug. Good to know which one :-)

Too bad this one was backed out so soon! I'll reland it with your change.
Flags: needinfo?(paolo.mozmail)
https://hg.mozilla.org/mozilla-central/rev/58aa679982e6
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1245991
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: