1152703 - Some sync errors prevent future syncs until restart

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect, P1)

Description

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

Attachment: 0007-Bug-XX-unexpected-exceptions-syncing-reading-list-ar.patch

The logic in start():

>  yield this._start();
>  delete this.promise;

means that if ._start fails, all future calls to .start() will immediately be rejected with the initial error. Sadly the FxA NO_ACCOUNT and UNVERIFIED_ACCOUNT errors cause this during an initial signup flow, causing sync to continue failing until restart. Also reported in bug 1152307 - see the log there - if reflects no work being done by the engine.

This can also be demonstrated in a browser scratchpad while in this state:

> Cu.import("resource:///modules/readinglist/Sync.jsm");
> Sync.promise.catch(err => dump("rejected: " + err))

dump()s the initial error without doing any work. Note .promise shouldn't be set while not running.

To repro:
* edit the profile's signedInUser.json and set verified=false. Delete signedInUserOAuthTokens.json. Disconnect from the network and restart.
* ignore all network related errors, but eventually you will end up with a sync log message "Can't sync due to FxA account state UNVERIFIED_ACCOUNT"

We are now in this state. Reconnect the network, wait for it to notice you are verified.  "Sync Now" (and scratchpad) continues to fail with the same error.  With this patch sync actually progresses.

Drew, if you give this r+, do you think you could push it then request uplift?  It would be nice to get this in the next beta - all new FxA-creating users are likely to hit this (existing sync users will not)

Comment 1

[Drew Willcoxon :adw]

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

Comment 2

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

Comment on attachment 8590161 [details] [diff] [review]
0007-Bug-XX-unexpected-exceptions-syncing-reading-list-ar.patch

Approval Request Comment
[Feature/regressing bug #]: reading list
[User impact if declined]: Sync is unlikely to work after the creation of an FxAccount.
[Describe test coverage new/current, TreeHerder]: Existing tests pass
[Risks and why]: Trivial patch, almost zero risk, limited to readinglist
[String/UUID change made/needed]: None

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds important to fix in reading list. If something in reading list broke syncing for all users, what was the regressing bug?  Can we add tests to try and catch that sort of error in future?   If something vague in reading list broke syncing in the first place, how can this be a low risk fix limited to reading list? 

Let's talk about how to verify this once it lands, and what QA or test coverage we can bring to bear.

Comment 4

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

(In reply to Liz Henry (:lizzard) from comment #3)
> Sounds important to fix in reading list. If something in reading list broke
> syncing for all users, what was the regressing bug?

This bug only affects Reading List Sync, as I understand it, not the broader Sync. Any references to "Sync" in this bug are about RL Sync specifically. I think this bug existed since RL Sync was implemented and we're just catching it now.

> Can we add tests to try and catch that sort of error in future?

We should have test coverage for "first sync after setup", but it's hard to test the whole flow because creating accounts and verifying them has a lot of external dependencies. Stuart and others are working on functional end-to-end tests, but it's a lot of work.

Comment 5

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

Yep - see comment 4 - not a regression and while we do try hard to have tests, it's difficult to cover all cases.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks for the clarification! It's good that the overall Sync feature isn't affected.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8590161 [details] [diff] [review]
0007-Bug-XX-unexpected-exceptions-syncing-reading-list-ar.patch

Approving for uplift to aurora.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed1b3f89af33

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590161 [details] [diff] [review]
0007-Bug-XX-unexpected-exceptions-syncing-reading-list-ar.patch

Should be in 38 beta 4.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99a7cf516b9c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8191b45753c7

Comment 12

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

Removing qe-verify flag since the feature was already backed out in favor of Pocket.