1130634 - More consistent error handing in FxAccounts.getOauthToken

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Chris Karlof [:ckarlof]]

We landed getOAuthToken in

http://hg.mozilla.org/integration/mozilla-inbound/rev/14beb450d966#l2.47

Since we expect this will be one of the primary FxA integration points going forward, it would be nice if it had more structured and documented errors.

In particular, at the end of the promise chain, we could catch any errors and then reject with one of several documented errors that corresponds to a more general error class.

For example, such classes could be:

* no logged in user (caller's reaction could be to trigger a login)
* authentication error (caller's reaction could be to trigger a re-auth)
* network error (caller's reaction could be to try again later)
* invalid parameter (hopefully only comes up in dev)
* unexpected error ("something's wrong :)")

Comment 1

[Zachary Carter [:zaach]]

Attachment: More consistent error handing in FxAccounts.getOauthToken

Comment 2

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

Comment on attachment 8563140 [details] [diff] [review]
More consistent error handing in FxAccounts.getOauthToken

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

LGTM, but I think it is worth exploring if we can use an extended Error() object.

::: services/fxaccounts/FxAccounts.jsm
@@ +968,5 @@
> +      }
> +    });
> +  },
> +
> +  _internalErrorToGeneralError: function (aError) {

this method name seems back to front - it looks like we *take* a "general" error (ie, unexpected exceptions etc) and return a specific error.  ie, I'd think it would be called something like "_generalErrorToFxaError()"

@@ +980,5 @@
> +    }
> +    return this._error(ERROR_UNKNOWN, aError);
> +  },
> +
> +  _error: function(aError, aDetails) {

It would seem ideal to me if we could "extend" the builtin error object and return them.  This might make it possible in the future to change all the existing "new Error()" calls with a call to this._error() so the errors are unified (not that we should do that here, but we *could* in the future)

::: services/fxaccounts/FxAccountsOAuthGrantClient.jsm
@@ +145,5 @@
>          if (request.response.success) {
>            return resolve(body);
>          }
>  
> +        if (body.errno) {

might be worth a comment here saying why this simple offset works (ie, I assume that error numbers in FxAccountsCommon are copied from the server, but the server uses an offset of zero)

Comment 3

[Zachary Carter [:zaach]]

Attachment: More consistent error handing in FxAccounts.getOauthToken

Comment 4

[Zachary Carter [:zaach]]

(In reply to Mark Hammond [:markh] from comment #2)
> Comment on attachment 8563140 [details] [diff] [review]
> More consistent error handing in FxAccounts.getOauthToken
> 
> Review of attachment 8563140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but I think it is worth exploring if we can use an extended Error()
> object.
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +968,5 @@
> > +      }
> > +    });
> > +  },
> > +
> > +  _internalErrorToGeneralError: function (aError) {
> 
> this method name seems back to front - it looks like we *take* a "general"
> error (ie, unexpected exceptions etc) and return a specific error.  ie, I'd
> think it would be called something like "_generalErrorToFxaError()"
> 

Ah, well, we return the error in a specific format but it could still be a general error, e.g. UNKNOWN_ERROR. "General" can mean a few different things here so I'll think of something different.

Comment 5

[Zachary Carter [:zaach]]

Attachment: More consistent error handing in FxAccounts.getOauthToken

Comment 6

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

Comment on attachment 8563187 [details] [diff] [review]
More consistent error handing in FxAccounts.getOauthToken

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

LGTM

::: services/fxaccounts/FxAccounts.jsm
@@ +919,5 @@
> +   *        }
> +   *
> +   * @return Promise.<string | Error>
> +   *        The promise resolves the oauth token as a string or rejects with
> +   *        an error object ({error: ERROR, details: {}}) of the following:

I think this comment is now wrong WRT the error description.

@@ +993,5 @@
> +    return this._error(ERROR_UNKNOWN, aError);
> +  },
> +
> +  _error: function(aError, aDetails) {
> +    log.error(aError);

I think we should log a better string here - IIUC, it will typically just log a number.  Something like:

log.error("FxA rejecting with error ${aError}, details ${aDetails}", {aError, aDetails});

If aDetails is itself an Error object that will log *2* stacks, which sounds like the right thing here.

Comment 7

[Chris Karlof [:ckarlof]]

Awesome, :zaach. This really cleans up the error handling. 

Eventually, the getOAuthToken method and its errors will deserve some documentation on our MDN page. I'd do this after you get the profile images landed though.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/332dbc5e0d1a