FxAccounts.jsm raises uncatchable async errors

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Yoric, Assigned: markh)

Tracking

unspecified
Firefox 31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Flags: needinfo?(ttaubert)
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(gavin.sharp) → needinfo?(mhammond)
Mark, I'll need your help on this. It's one of my last blockers for bug 976205.
Assignee

Comment 2

5 years ago
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
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Attachment #8404489 - Flags: review?(ttaubert)
Flags: needinfo?(ttaubert)
Flags: needinfo?(mhammond)
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?
Attachment #8404489 - Flags: review?(ttaubert) → feedback+
Assignee

Comment 4

5 years ago
(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?
Assignee

Comment 5

5 years ago
See comment 4
Flags: needinfo?(ttaubert)
(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 :)
Flags: needinfo?(ttaubert)
Attachment #8404489 - Flags: feedback+ → review+
Assignee

Comment 7

5 years ago
Same patch but with a comment referencing bug 995134.  Carry r=ttaubert forward.
Attachment #8404489 - Attachment is obsolete: true
Attachment #8405284 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4328bebb529
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla31
I still see the errors:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37959151&tree=Try&full=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(mhammond)
Assignee

Comment 11

5 years ago
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.
Attachment #8413472 - Flags: review?(jparsons)
Flags: needinfo?(mhammond)
Assignee

Updated

5 years ago
Duplicate of this bug: 959744
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?
Attachment #8413472 - Flags: review?(jparsons) → review+
Assignee

Comment 14

5 years ago
(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
(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!
https://hg.mozilla.org/mozilla-central/rev/8bae1e18091d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

2 years ago
Product: Core → Firefox
Target Milestone: mozilla31 → Firefox 31
You need to log in before you can comment on or make changes to this bug.