992371 - FxAccounts.jsm raises uncatchable async errors

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Once bug 976205 lands, the following errors will cause oranges:

TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: Error: User email verification timed out. - rejection date: Fri Apr 04 2014 08:14:18 GMT-0700 (PDT) - See following stack:
TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: Error: Verification aborted; Another user signing in - rejection date: Fri Apr 04 2014 08:14:18 GMT-0700 (PDT) - See following stack:
TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: TypeError: currentState.whenKeysReadyDeferred is null - rejection date: Fri Apr 04 2014 08:14:18 GMT-0700 (PDT) - See following stack:
TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: Error: Verification aborted; Another user signing in - rejection date: Fri Apr 04 2014 08:14:18 GMT-0700 (PDT) - See following stack:
TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: Error: Verification aborted; Another user signing in - rejection date: Fri Apr 04 2014 08:14:18 GMT-0700 (PDT) - See following stack:
TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: Error: Verification aborted; Another user signing in - rejection date: Fri Apr 04 2014 08:14:21 GMT-0700 (PDT) - See following stack:
TEST-UNEXPECTED-FAIL | resource://gre/modules/FxAccounts.jsm | A promise chain failed to handle a rejection: Error: Verification aborted; Another user signing in - rejection date: Fri Apr 04 2014 08:14:21 GMT-0700 (PDT) - See following stack:

Now, looking at the code, I have no clue how these rejections can be caught, which worries me a little. Tim, Gavin, could one of you dispatch someone to look at this? I would really like to land bug 976205 while the number of uncaught rejections is relatively low.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Mark, I'll need your help on this. It's one of my last blockers for bug 976205.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-992371-avoid-promise-rejection-messages-from-FxA.patch

This patch solves all such messages when running all mochi tests in browser/base/content/test/general - most of them came as a result of browser_aboutAccounts.js.

Try at https://tbpl.mozilla.org/?tree=Try&rev=789bb7d1360c

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8404489 [details] [diff] [review]
0001-Bug-992371-avoid-promise-rejection-messages-from-FxA.patch

Review of attachment 8404489 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-fxaccounts.js
@@ +193,5 @@
>            this.button.setAttribute("label", userData.email);
>            this.button.setAttribute("tooltiptext", userData.email);
>          }
>        }
> +    }

Nit: missing semicolon;

@@ +199,5 @@
> +      doUpdate(userData);
> +    }).then(null, error => {
> +      // This is most likely in tests, were we quickly log users in and out.
> +      // The most likely scenario is a user logged out, so reflect that
> +      doUpdate(null);

Instead of guessing what might have happened, couldn't we just call .getSignedInUser() again (and again, and ...) until that resolves?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Instead of guessing what might have happened, couldn't we just call
> .getSignedInUser() again (and again, and ...) until that resolves?

If we could be sure the problem was another user managing to log out before the promise resolved, I'd agree.  But "other" things might go wrong causing us to retry forever.  I think this is a test-only problem in reality and a worst-case scenario in this patch is the UI state being incorrect.  So maybe a riding-the-trains followup for better rejection errors and a fixup in browser-social would be a good compromise?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

See comment 4

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Hammond [:markh] from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #3)
> > Instead of guessing what might have happened, couldn't we just call
> > .getSignedInUser() again (and again, and ...) until that resolves?
> 
> If we could be sure the problem was another user managing to log out before
> the promise resolved, I'd agree.  But "other" things might go wrong causing
> us to retry forever.  I think this is a test-only problem in reality and a
> worst-case scenario in this patch is the UI state being incorrect.

Fair enough, we don't want to retry forever :)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-992371-avoid-promise-rejection-messages-from-FxA.patch

Same patch but with a comment referencing bug 995134.  Carry r=ttaubert forward.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/c4328bebb529

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4328bebb529

Comment 10

[David Teller [:Yoric] - still alive but not very active]

I still see the errors:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37959151&tree=Try&full=1

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-992371-avoid-A-promise-chain-failed-to-handle-a-.patch

Flagging Jed for review as he is the original author of both the test and the module.

2 changes here (which in theory don't change behaviour at all, other than avoiding the promise rejection warnings)

* startVerifiedCheck() returned a promise, but nothing consumed the result.  If that promise was rejected we get the warning.  This patch no longer returns the promise to make it clear it is a "background" fetch, and adds a default rejection handler.

* One of the tests causes setSignedInUser() to reject, but doesn't have a rejection handler - this patch adds one.

Comment 13

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8413472 [details] [diff] [review]
0001-Bug-992371-avoid-A-promise-chain-failed-to-handle-a-.patch

Review of attachment 8413472 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  Thanks for making the repairs, Mark!

::: services/fxaccounts/FxAccounts.jsm
@@ +704,5 @@
> +    // handler to avoid runtime warnings about the rejection not being handled.
> +    this.whenVerified(data).then(
> +      () => this.getKeys(),
> +      err => log.info("startVerifiedCheck promise was rejected: " + err)
> +    );

Thanks for adding the comment and the error handler.  Curious: why log.info and not log.error?

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #13)
> Thanks for adding the comment and the error handler.  Curious: why log.info
> and not log.error?

I tossed a coin? ;)  Really, I don't think this state is an "error" as such, so just used .info.

Thanks for the speedy review!

https://hg.mozilla.org/integration/fx-team/rev/8bae1e18091d

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Mark Hammond [:markh] from comment #14)

> I tossed a coin? ;)  Really, I don't think this state is an "error" as such,
> so just used .info.

Heh :)  I think you're right.  Thanks again for the patch!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bae1e18091d