Closed Bug 1121329 Opened 5 years ago Closed 5 years ago

Fix some promise logic in browserid_identity and hawk

Categories

(Firefox :: Sync, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.2 - 9 Feb

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

There are 2 potential places where our use of promises might fail in edge-cases.

* In browserid_identity, _fetchTokenForUser() must return a promise, but the first condition in that function returns null.  It must return a promise due to _ensureValidToken doing:

    return this._fetchTokenForUser().then(

so returning null will fail.  This is unlikely to be hit in practice (in the usual case, null is returned only when the MP is locked, and _ensureValidToken shouldn't be called then - but you never know)

* hawkclient's |request| method is similar - in edge-cases the actual request being made might throw rather than calling onComplete.  FxAccountsClient typically expects promise rejection rather than an exception, so I've change it so it returns a rejected promise in this case.

* Other misc stuff not strictly related to the bug title:

** prevent a warning being logged to the console when err has no .code attribute
-        } else if (err.code === 401) {
+        } else if (err.code && err.code === 401) {

** Have _ensureValidToken explicitly set this._token to null before attempting to fetch a new one, as a risk exists the old token might remain and attempt to be used in error conditions.  I can't see where that *actually* happens, but it makes sense to me.

** Have the test explicitly check this._token is null when _fetchTokenForUser resolves with null.

** Add a comment about an unlikely-but-possible exception.

** includes an indentation fix to FxAccountsClient.jsm (tabs->spaces) 

Chris, what do you think?
Attachment #8548648 - Flags: feedback?(ckarlof)
All look like great improvements! Thanks Mark.
Comment on attachment 8548648 [details] [diff] [review]
0003-Bug-XXXXXXX-fixes-to-promise-handling-in-FxA-and-Haw.patch

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

::: services/sync/modules/browserid_identity.js
@@ +713,5 @@
>      let endPointFromIdentityToken = function() {
> +      // XXX - it's possible this.idendity._token will be null here if
> +      // this.identity._canFetchKeys() returned false.  That should be unlikely
> +      // here and is going to cause an exception referencing .endpoint, but
> +      // we should probably do something saner here.

File a bug for this or did you intend to address it in this patch?
Attachment #8548648 - Flags: feedback?(ckarlof) → feedback+
(In reply to Chris Karlof [:ckarlof] from comment #2)
> Comment on attachment 8548648 [details] [diff] [review]
> 0003-Bug-XXXXXXX-fixes-to-promise-handling-in-FxA-and-Haw.patch
> 
> Review of attachment 8548648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/modules/browserid_identity.js
> @@ +713,5 @@
> >      let endPointFromIdentityToken = function() {
> > +      // XXX - it's possible this.idendity._token will be null here if
> > +      // this.identity._canFetchKeys() returned false.  That should be unlikely
> > +      // here and is going to cause an exception referencing .endpoint, but
> > +      // we should probably do something saner here.
> 
> File a bug for this or did you intend to address it in this patch?

I was actually going to ignore it as it should still work correctly - it will throw an exception and cause sync to enter the "transient error" state, which sounds like the right thing to do.  My first cut at this patch checked for null and threw an explicit exception, but I ended up deciding that didn't add enough value and that the exception raised by the null reference + the comment is obvious enough.

Is there something else you can think of I should be doing (or otherwise see a flaw in the above)?
> Is there something else you can think of I should be doing (or otherwise see a flaw in the above)?

Explicitly throwing will add value in the sense that it may make users' sync error logs easier to understand when this occurs. Instead, the engineer at the time would have to spelunk through the code to figure out what a null reference means in this context. Searching for the error string would be easier. :)

Regardless what you decide, the comment should be less wishy-washy and more specific. "we should probably do something saner here" will bit rot quickly because you will forget what you had in mind to do in 1 month time. :) If you keep the code as is, I'd add a description of what the alternative "saner" implementation would look like (hence my question about filing a bug), or if you choose to raise an exception, I'd maybe explain why you're raising an exception.
Flags: firefox-backlog+
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Points: --- → 2
(In reply to Chris Karlof [:ckarlof] from comment #4)
> Explicitly throwing will add value in the sense that it may make users' sync
> error logs easier to understand when this occurs. Instead, the engineer at
> the time would have to spelunk through the code to figure out what a null
> reference means in this context. Searching for the error string would be
> easier. :)

A good point well made!  This is the same as the last but with an explicit exception thrown and some clarifying comments.
Attachment #8548648 - Attachment is obsolete: true
Attachment #8552236 - Flags: review?(ckarlof)
Iteration: --- → 38.1 - 26 Jan
Flags: qe-verify?
Comment on attachment 8552236 [details] [diff] [review]
0003-Bug-1121329-fixes-to-promise-handling-in-FxA-and-Haw.patch

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

Thanks, Mark!
Attachment #8552236 - Flags: review?(ckarlof) → review+
Flags: qe-verify? → qe-verify-
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
https://hg.mozilla.org/mozilla-central/rev/48821a0e6f72
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.