1317589 - Sync reports a zero'd UID and no device ID on transient error renewing token

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

From the sync pings we can often see a UID with all zeros. There are 2 main times we see this:

a) When sync fails with a network error. My speculation here is that the token has expired normally, then sync fails trying to fetch a new token as part of the sync. At that point we've lost the hashed uid. This seems likely to happen when the PC appears to have been put to sleep during the sync - the laptop wakes, immediately notices the token expiry and tries to fetch a new one before the network has reconnected.

b) Around 7% of syncs that fail with a shutdownerror also seem to have a zero'd UID. This is harder to guess about, but it may be the same basic issue - token has expired and fetching a new token fails. Another edge-case might be (a) user shuts down but one of various bugs causes shutdown to take a long time, (b) during this time the timer fires and we have an expired token, then fail to get one (bug 1317587)

I think we should fix the underlying cause here by storing the hashed UID in memory and *not* discarding it on token expiry under the assumption we will get the same UID. We *will* discard it on sync reset, a new login, etc. That should almost certainly fix (a) and will hopefully fix (b) and we'll let telemetry tell us whether it did.

Blocking bug 1301389, as otherwise that bug will cause a ping to be submitted in these transient cases which we don't want.

Comment 1

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

Attachment: Bug 1317589 - store the most recent hashed UID so we can still report it on transient errors renewing tokens.

Review commit: https://reviewboard.mozilla.org/r/93010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93010/

Comment 2

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

Comment on attachment 8810715 [details]
Bug 1317589 - store the most recent hashed UID so we can still report it on transient errors renewing tokens.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93010/diff/1-2/

Comment 3

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

Comment on attachment 8810715 [details]
Bug 1317589 - store the most recent hashed UID so we can still report it on transient errors renewing tokens.

https://reviewboard.mozilla.org/r/93010/#review93066

::: services/sync/modules/browserid_identity.js:251
(Diff revisions 1 - 2)
>          }
>        }).then(() => {
>          return this._fetchTokenForUser();
>        }).then(token => {
>          this._token = token;
> +        if (token) {

hrm - this block clearly expects a token and only tests failed without it - I think I should dig deeper into token being null/undefined here rather than checking it...

Comment 4

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

Comment on attachment 8810715 [details]
Bug 1317589 - store the most recent hashed UID so we can still report it on transient errors renewing tokens.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93010/diff/2-3/

Comment 5

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

Comment on attachment 8810715 [details]
Bug 1317589 - store the most recent hashed UID so we can still report it on transient errors renewing tokens.

https://reviewboard.mozilla.org/r/93010/#review93066

> hrm - this block clearly expects a token and only tests failed without it - I think I should dig deeper into token being null/undefined here rather than checking it...

As per the new comment, we consider a null token to still be successful if the master-password appears locked to avoid the UI entering a "needs authentication" state.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8810715 [details]
Bug 1317589 - store the most recent hashed UID so we can still report it on transient errors renewing tokens.

https://reviewboard.mozilla.org/r/93010/#review93460

This makes sense, and is also to a certain extent, how the sync ping documentation claims it works (IIRC it says something like the token will only be 00000... if we've never seen it).

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

One thought: Should the same approach be taken for the device ID?

Comment 8

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

We discussed this in person, but FTR, now that we use sync device IDs there's almost zero chance of the current device ID not existing or of it changing without the user signing in to sync again. So I think we can largely ignore the concept of a transient device ID and only consider the uid.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cd67fad42ed
store the most recent hashed UID so we can still report it on transient errors renewing tokens. r=tcsc

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cd67fad42ed