1059391 - Create debugging pref that forces Sync to fetch a new token for every Sync request

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Chris Karlof [:ckarlof]]

This will make debugging issues like bug 1056523, which involve password change or reset, easier. After you change or reset your password, you have to wait until certain cached credentials expire (the token and signed BID public key) before you'll be asked to "reconnect to sync". I suggest we add a pref that says "don't cache sync authentication credentials".

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

Comment 2

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

Comment 3

[Chris Karlof [:ckarlof]]

Comment on attachment 8480116 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

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

::: services/fxaccounts/FxAccounts.jsm
@@ +162,5 @@
>      ).then(result => this.resolve(result));
>    },
>  
>    getKeyPair: function(mustBeValidUntil) {
> +    let useCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.useCachedAuthCredentials");

Mark, this line is causing a failure in the tests:

0:10.30 TEST-UNEXPECTED-FAIL | /Users/Chris/git/gecko-dev/testing/xpcshell/head.js | Unexpected exception NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref] - See following stack

I get it when I run test_accounts.js: 
% ./mach xpcshell-test services/fxaccounts/tests/xpcshell/test_accounts.js

The rest of the tests seem to pass and manual testing of the pref seems to work for me.

Any ideas?

Comment 4

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

Comment on attachment 8480109 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

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

This looks like a good idea, but I'd be inclined to negate the pref (ie, ...debug.skipCachedAuthCredentials), not bother trying to create a default value for it, and wrap accesses around a try/catch (the negating means the missing pref gets treated as though the pref is false)

Comment 5

[Chris Karlof [:ckarlof]]

(In reply to Mark Hammond [:markh] from comment #4) 
> This looks like a good idea, but I'd be inclined to negate the pref (ie,
> ...debug.skipCachedAuthCredentials), not bother trying to create a default
> value for it, and wrap accesses around a try/catch (the negating means the
> missing pref gets treated as though the pref is false)

Yeah, I started with that, but it can lead to double negatives in the code, which is potentially confusing, but I'll go with your approach.

Thanks!

Comment 6

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

Comment 7

[Chris Karlof [:ckarlof]]

Manual testing done, tests pass locally, and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e8793b2c4e85

Comment 8

[James Bonacci [:jbonacci]]

:ckarlof do you have somebody from one of the QA teams to test this Try build?

Comment 9

[Chris Karlof [:ckarlof]]

james, no I don't. This patch itself doesn't need testing, but this try build (in particular, setting the debugging flag) would make testing bug 1056523 much easier.

Comment 10

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

Comment on attachment 8480721 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

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

::: services/fxaccounts/FxAccounts.jsm
@@ +163,5 @@
>    },
>  
>    getKeyPair: function(mustBeValidUntil) {
> +    // If the debugging pref to ignore cached authentication credentials is set for Sync,
> +    // then don't any cached key pair, i.e., generate a new one and get it signed.

"then don't any" - I think you probably meant to add "use" in there.

@@ +168,5 @@
> +    // The purpose of this pref is to expedite any auth errors as the result of a
> +    // expired or revoked FxA session token, e.g., from resetting or changing the FxA
> +    // password.
> +    let ignoreCachedAuthCredentials = false;
> +    try { ignoreCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.ignoreCachedAuthCredentials"); } catch(e) {}

probably not worth trying to save a few lines here - ie, please just indent as normal (and ditton in bid_identity)

Comment 11

[Chris Karlof [:ckarlof]]

Attachment: 0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

r=markh from previous patch

Comment 12

[Chris Karlof [:ckarlof]]

https://hg.mozilla.org/integration/fx-team/rev/18563fc91735

Comment 13

[Chris Karlof [:ckarlof]]

:markh tells me I shouldn't mark as fixed until it's merged to m-c. Sorry!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18563fc91735

Comment 15

[Bogdan Maris, Desktop QA]

[Tracking Requested - why for this release]:

Can this be pushed to other channels like beta (I think it has arrived in Aurora - if not then aurora as well), it would help for testing.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

We're not going to track, but please go ahead and request patch approvals.

Comment 17

[Bogdan Maris, Desktop QA]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> We're not going to track, but please go ahead and request patch approvals.
Chris, can you please take care of this? I`m not familiar with the process and I don`t even think I have the rights to do that.

Comment 18

[Chris Karlof [:ckarlof]]

Comment on attachment 8480969 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

Approval Request Comment
[Feature/regressing bug #]: Add support for Sync debugging pref.
[User impact if declined]: It will make testing sync password reset bugs harder.
[Describe test coverage new/current, TBPL]:
[Risks and why]: None.
[String/UUID change made/needed]:

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c45a0f90b53e

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing [qa+] based on comment 9.