Closed Bug 1130634 Opened 6 years ago Closed 6 years ago

More consistent error handing in FxAccounts.getOauthToken

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: ckarlof, Assigned: zaach)

References

Details

Attachments

(1 file, 2 obsolete files)

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 :)")
See Also: → 1131409
Attachment #8563140 - Flags: review?(mhammond)
Attachment #8563140 - Flags: feedback?(ckarlof)
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)
Attachment #8563140 - Flags: review?(mhammond) → feedback+
Attachment #8563140 - Attachment is obsolete: true
Attachment #8563140 - Flags: feedback?(ckarlof)
Attachment #8563177 - Flags: review?(mhammond)
Attachment #8563177 - Flags: feedback?(ckarlof)
(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.
Attachment #8563177 - Attachment is obsolete: true
Attachment #8563177 - Flags: review?(mhammond)
Attachment #8563177 - Flags: feedback?(ckarlof)
Attachment #8563187 - Flags: review?(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.
Attachment #8563187 - Flags: review?(mhammond) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/332dbc5e0d1a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Product: Core → Firefox
Target Milestone: mozilla38 → Firefox 38
You need to log in before you can comment on or make changes to this bug.