Closed Bug 992371 Opened 6 years ago Closed 6 years ago

FxAccounts.jsm raises uncatchable async errors

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: Yoric, Assigned: markh)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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+
(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?
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+
Same patch but with a comment referencing bug 995134.  Carry r=ttaubert forward.
Attachment #8404489 - Attachment is obsolete: true
Attachment #8405284 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4328bebb529
Status: ASSIGNED → RESOLVED
Closed: 6 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)
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)
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+
(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: 6 years ago6 years ago
Resolution: --- → FIXED
Product: Core → Firefox
Target Milestone: mozilla31 → Firefox 31
You need to log in before you can comment on or make changes to this bug.