Fix reconcilor loop when the primary account is in an auth error state.

Fix reconcilor loop when the primary account is in an auth error state.

This CL changes the reconcilor to not merge accounts which are known to be in some kind of auth error state.  If they are not present in the gaia cookie, their absence will not trigger a new reconcile cycle.

Added tests to help detect new code that may introduce a loop.

BUG=516070
Committed: https://crrev.com/dc1e24b7e50bc39c25f23e272a154837360fb563
Cr-Commit-Position: refs/heads/master@{#342900}

[nyquist, 2015-08-01 00:02:20]

nyquist@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2015-08-01 00:02:21]

lgtm

[Roger Tawa OOO till Jul 10th, 2015-08-06 00:45:39]

Patchset #6 (id:100001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 00:45:51]

Patchset #5 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 00:46:07]

Patchset #5 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 01:24:33]

Patchset #2 (id:20001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 01:24:54]

Patchset #2 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 01:25:00]

Patchset #2 (id:60001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 01:25:05]

Patchset #2 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-08-06 13:42:12]

rogerta@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-08-06 13:42:13]

Hi Anthony,

Can you please take a look?

Tommy: can you take another loop?  This is a completely different CL from the
one you lgtm'ed earlier.

Thanks.

[nyquist, 2015-08-11 05:44:31]

still lgtm

https://codereview.chromium.org/1267843003/diff/180001/chrome/browser/signin/...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/1267843003/diff/180001/chrome/browser/signin/...
chrome/browser/signin/account_reconcilor_unittest.cc:788: // The priamry account
is in auth error, so it is not in the cookie.
Nit: primary

[anthonyvd, 2015-08-11 15:43:10]

lgtm % a couple questions.

Sorry for the delay!

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/oauth2_token_service_delegate_android.cc (right):

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
chrome/browser/signin/oauth2_token_service_delegate_android.cc:198: :
it->second.error.state() != GoogleServiceAuthError::NONE;
Any way to put at least part of this in the base class to reduce duplication?
Maybe at least the last part of the ternary, this way changing it to take
Transient/Persistent errors into account would only have to happen in one place.

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/oauth2_token_service_delegate_android.h (right):

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
chrome/browser/signin/oauth2_token_service_delegate_android.h:115: struct
ErrorInfo {
Any reason why errors_ doesn't just store the GoogleServiceAuthError directly?
Is it not copyable?

[Roger Tawa OOO till Jul 10th, 2015-08-11 16:25:26]

Thanks guys.

Anthony: implemented as we discussed.

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/oauth2_token_service_delegate_android.cc (right):

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
chrome/browser/signin/oauth2_token_service_delegate_android.cc:198: :
it->second.error.state() != GoogleServiceAuthError::NONE;
On 2015/08/11 15:43:10, anthonyvd wrote:
> Any way to put at least part of this in the base class to reduce duplication?
> Maybe at least the last part of the ternary, this way changing it to take
> Transient/Persistent errors into account would only have to happen in one
place.

Done.

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/oauth2_token_service_delegate_android.h (right):

https://codereview.chromium.org/1267843003/diff/200001/chrome/browser/signin/...
chrome/browser/signin/oauth2_token_service_delegate_android.h:115: struct
ErrorInfo {
On 2015/08/11 15:43:10, anthonyvd wrote:
> Any reason why errors_ doesn't just store the GoogleServiceAuthError directly?
> Is it not copyable?

GoogleServiceAuthError does not have a no-arg ctor, so can't be used in an
std::map directly.

[Roger Tawa OOO till Jul 10th, 2015-08-11 16:27:26]

https://codereview.chromium.org/1267843003/diff/180001/chrome/browser/signin/...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/1267843003/diff/180001/chrome/browser/signin/...
chrome/browser/signin/account_reconcilor_unittest.cc:788: // The priamry account
is in auth error, so it is not in the cookie.
On 2015/08/11 05:44:30, nyquist wrote:
> Nit: primary

Done.

[anthonyvd, 2015-08-11 16:29:27]

Thanks! Still lgtm

[Roger Tawa OOO till Jul 10th, 2015-08-11 19:10:58]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-08-11 19:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1267843003/#ps240001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-08-11 19:11:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267843003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267843003/240001

[commit-bot: I haz the power, 2015-08-11 20:10:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-11 20:10:27]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Roger Tawa OOO till Jul 10th, 2015-08-11 20:23:11]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2015-08-11 20:23:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267843003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267843003/240001

[commit-bot: I haz the power, 2015-08-11 21:20:59]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2015-08-11 21:21:43]

Patchset 7 (id:??) landed as
https://crrev.com/dc1e24b7e50bc39c25f23e272a154837360fb563
Cr-Commit-Position: refs/heads/master@{#342900}