983270 - Error notification indicating bad password disappears as scheduled sync starts and fails

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

STR:

* Sign into FxA, exit the browser.
* In a different profile, change the password to the FxA account.
* Start the original profile

Note the after ~10 seconds there is a yellow notification bar indicating the password is incorrect with a button to correct the issue.

* Wait for a scheduled sync (or alternatively, in a "browser" scratchpad execute "Weave.Service.sync()" - the sync will fail as expected.

Actual:
* Yellow notification bar is removed.

Expected:
* Yellow notification bar remains.

Comment 1

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

Attachment: 0001-Bug-983270-part-1-testonly-refactor-of-FxA-tests-to-.patch

Test only fix that factors out tokenserver mocking so it can be used by other tests

Comment 2

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

Attachment: 0002-Bug-983270-part-2-_findCluster-should-return-null-on.patch

The fix and test

Comment 3

[Chris Karlof [:ckarlof]]

Comment on attachment 8390893 [details] [diff] [review]
0001-Bug-983270-part-1-testonly-refactor-of-FxA-tests-to-.patch

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

::: services/sync/modules-testing/fxa_utils.js
@@ +13,5 @@
> +Cu.import("resource://testing-common/services-common/logging.js");
> +Cu.import("resource://testing-common/services/sync/utils.js");
> +
> +// Create a new browserid_identity object and initialize it with a
> +// mocked TokenServerClient which always gets the specified response.

"gets" -> "receives" for clarity.

@@ +55,5 @@
> +
> +  yield browseridManager.initializeWithCurrentIdentity();
> +  try {
> +    yield browseridManager.whenReadyToAuthenticate.promise;
> +    Assert.ok(false, "expecting this promise to resolve with an error");

I'm not clear on the reason this is here. If the response we pass in above is a success, then it seems that this assert will be executed.

::: services/sync/tests/unit/test_browserid_identity.js
@@ +343,5 @@
>  add_task(function test_getTokenErrors() {
>    _("BrowserIDManager correctly handles various failures to get a token.");
>  
>    _("Arrange for a 401 - Sync should reflect an auth error.");
> +  yield initializeIdentityWithTokenServerResponse({

It looks like this previously was always expected to trigger an error, which explains the "Assert(false,...)" call in fxa_utils.js.

@@ +355,5 @@
>  
>    // And for good measure, some totally "unexpected" errors - we generally
>    // assume these problems are going to magically go away at some point.
>    _("Arrange for an empty body with a 200 response - should reflect a network error.");
> +  yield initializeIdentityWithTokenServerResponse({

This also triggers an error due to a JSON parse error.

Comment 4

[Chris Karlof [:ckarlof]]

Comment on attachment 8390896 [details] [diff] [review]
0002-Bug-983270-part-2-_findCluster-should-return-null-on.patch

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

The new tests here also need to be refactored after the appropriate fixes are made to the other patch attached to this bug.

::: services/sync/modules/browserid_identity.js
@@ +629,5 @@
> +    }).then(
> +      null, err => {
> +      // If there is an authentication error we must return null to indicate
> +      // a non-exceptional "can't get it" - an exception is treated as a
> +      // transient error

I think it would be useful to include additional information in the comment why we're doing this, specifically (IIUC), that the error reporting is going to show if the clusterURL is null (indicating a failure to auth), but not if we raise a error (indicating a transient network error). Including some reference to where elsewhere in the code these checks are made (e.g., clusterURL is null) would also be useful. 

I think the relevant piece is in service.js: http://hg.mozilla.org/mozilla-central/annotate/f073b3d6db1f/services/sync/modules/service.js#l720

When we get an auth error with the storage server and try to refresh the clusterURL, if that also triggers an auth error we should null out the clusterURL which will cause: this.status.login = LOGIN_FAILED_LOGIN_REJECTED.

Otherwise, if we encounter any other error, we re-throw it, and it will be caught and set this.status.login = LOGIN_FAILED_NETWORK_ERROR, which is the behavior we want.

::: services/sync/tests/unit/test_fxa_service_cluster.js
@@ +5,5 @@
> +Cu.import("resource://services-sync/util.js");
> +Cu.import("resource://testing-common/services/sync/fxa_utils.js");
> +Cu.import("resource://testing-common/services/sync/utils.js");
> +
> +function do_check_throws(func) {

This function isn't used in this file.

@@ +39,5 @@
> +  Assert.strictEqual(cluster, null);
> +
> +  _("_findCluster() works with correct tokenserver response.");
> +  let endpoint = "http://example.com/something";
> +  yield initializeIdentityWithTokenServerResponse({

If this call is generally expected to succeed, then any error it throws may be masked in the last lines of the helper function in fxa_utils.js. As we discussed, this test may be improperly passing due to a masked error.

Comment 5

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

Attachment: 0001-Bug-983270-part-1-testonly-refactor-of-FxA-tests-to-.patch

I hope I didn't screw up

Comment 6

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

Attachment: 0002-Bug-983270-part-2-_findCluster-should-return-null-on.patch

Comment 7

[Chris Karlof [:ckarlof]]

Comment on attachment 8391629 [details] [diff] [review]
0002-Bug-983270-part-2-_findCluster-should-return-null-on.patch

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

looks good.

::: services/sync/modules/browserid_identity.js
@@ +637,5 @@
> +    }).then(
> +      null, err => {
> +      // If there is an authentication error we must return null to indicate
> +      // a non-exceptional "can't get it" - an exception is treated as a
> +      // transient error

I still believe this comment could use some more exposition and code references to help a reader understand why we need this needs to return null. This will likely help our future selves make sense of this. :) See my previous review for some suggestions.

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/aa19dcc93745
https://hg.mozilla.org/integration/fx-team/rev/bf2baa6d94aa

Comment 9

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

Comment on attachment 8391628 [details] [diff] [review]
0001-Bug-983270-part-1-testonly-refactor-of-FxA-tests-to-.patch

This request is for both patches as landed on m-c.  I'm requesting approval for uplift before the patch has landed in m-c under the assumption it will make it to m-c and in the interests of ensuring it uplifts before merge.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: The user may not be notified of authentication errors when syncing.
Testing completed (on m-c, etc.): Landed with tests.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aa19dcc93745
https://hg.mozilla.org/mozilla-central/rev/bf2baa6d94aa

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/361cac9505e5
https://hg.mozilla.org/releases/mozilla-aurora/rev/07f5580d8a54

Comment 12

[Andrei Vaida [:avaida]]

The error notification is no longer disappearing once a scheduled sync starts and fails on Firefox 29 (Build ID: 20140421221237) [1] and Aurora 30 (2014-04-28) [2], but the notification flickers twice if you initiate a manual sync. Mark, could you please let me know if this behavior is expected?

1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
2. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 13

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

(In reply to Andrei Vaida, QA [:avaida] from comment #12)
> The error notification is no longer disappearing once a scheduled sync
> starts and fails on Firefox 29 (Build ID: 20140421221237) [1] and Aurora 30
> (2014-04-28) [2], but the notification flickers twice if you initiate a
> manual sync. Mark, could you please let me know if this behavior is expected?

Thanks!  I think some flickering when doing a manual sync is not ideal, but acceptable - assuming the error bar remains after the flickering.

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Mark Hammond [:markh] from comment #13)
> Thanks!  I think some flickering when doing a manual sync is not ideal, but
> acceptable - assuming the error bar remains after the flickering.
The error notification bar remains in place in case of both scheduled and manually initiated sync. Thank you for clarifying this! Fix confirmed on Firefox 29 (Build ID: 20140421221237) and Aurora 30 (2014-04-28), as previously mentioned, using:
 * Windows 7 64-bit [1],
 * Ubuntu 13.10 64-bit [2],
 * Mac OS X 10.9 [3].

1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
2. Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
3. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0