Delay fetching account info until OnRefreshTokensLoaded().

Delay fetching account info until OnRefreshTokensLoaded().

Always fire OnRefreshTokensLoaded after token service is ready.
Previously this was only called if previously signed in.

Also fix a missing BCKS dependency of the SigninManager on the
AccountFetcherService.
This causes a cyclic dependency among:
AccountFetcherService - SigninManager - InvalidationProvider
Solution is to inject InvalidationService before it is used.

Added a Shutdown notification from InvalidationService to facilitate this.

This will resolve 2 issues:
1> Android OS uses email id whereas Chrome uses the GAIA id to identify
the account. This translation is only guaranteed to be available after
OnRefreshTokensLoaded(), which is required before fetching anything.
2> In case of changes to the account, force refreshing before
OnRefreshTokensLoaded() will result in stale values which are not
updated.

BUG=535211, 525207
Committed: https://crrev.com/3a5f21ae2dfb0b5fa385b1eb75c8e2a66f4fbdb5
Cr-Commit-Position: refs/heads/master@{#356820}

[knn, 2015-10-01 13:47:24]

knn@chromium.org changed reviewers:
+ gogerald@chromium.org, mlerman@chromium.org, nyquist@chromium.org,
rogerta@chromium.org

[knn, 2015-10-01 13:47:24]

knn@chromium.org changed required reviewers:
+ mlerman@chromium.org, nyquist@chromium.org

[knn, 2015-10-01 13:47:24]

PTAL.

[knn, 2015-10-01 13:47:47]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-01 13:48:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103004/1

[Mike Lerman, 2015-10-01 13:54:44]

This seems module nyquist's review.

profiles LGTM

https://codereview.chromium.org/1380103004/diff/1/chrome/browser/android/sign...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://codereview.chromium.org/1380103004/diff/1/chrome/browser/android/sign...
chrome/browser/android/signin/account_tracker_service_android.cc:42:
AccountFetcherServiceFactory::GetForProfile(profile)
nit: add { and } for multi-line statement.

[Roger Tawa OOO till Jul 10th, 2015-10-01 14:17:38]

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

[Roger Tawa OOO till Jul 10th, 2015-10-01 14:17:38]

+Anthony who owns AccountFetcherService

https://codereview.chromium.org/1380103004/diff/1/chrome/browser/android/sign...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://codereview.chromium.org/1380103004/diff/1/chrome/browser/android/sign...
chrome/browser/android/signin/account_tracker_service_android.cc:45: }
I suspect this could come up in other situations too (i.e. we need to seed ATS
earlier for some reason).  So I think it might be better to do the following:

- change AFS::EnableNetworkFetches() to be a noop instead of DCHECKing if it has
already been called.  This should be easy since it already maintains a bool

- no need for bool here, just unconditionally call EnableNetworkFetches()

- undo the change in profile_manager.cc

[commit-bot: I haz the power, 2015-10-01 14:31:49]

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

[commit-bot: I haz the power, 2015-10-01 14:31:50]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2015-10-01 14:33:20]

https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/andro...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/andro...
chrome/browser/android/signin/account_tracker_service_android.cc:45: }
On 2015/10/01 14:17:38, Roger Tawa wrote:
> I suspect this could come up in other situations too (i.e. we need to seed ATS
> earlier for some reason).  So I think it might be better to do the following:
> 
> - change AFS::EnableNetworkFetches() to be a noop instead of DCHECKing if it
has
> already been called.  This should be easy since it already maintains a bool
> 
> - no need for bool here, just unconditionally call EnableNetworkFetches()
> 
> - undo the change in profile_manager.cc

Agreed that EnableNetworkFetches should be idempotent so removing the DCHECK and
no-op'ing when it was already called is probably the right thing to do. It won't
allow undoing the profile_manager.cc changes though (I think) since the goal is
to delay the call to after accounts are seeded.

https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/profi...
File chrome/browser/profiles/profile_manager.cc (right):

https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/profi...
chrome/browser/profiles/profile_manager.cc:1112: // Accounts need to be
partially seeded on Android.
Can you just add a little more to this comment (for future us :))?Something like
"Accounts need to be partially seeded on Android before enabling network fetches
because [reason]".

[chromium-reviews, 2015-10-01 14:59:24]

Don't know why flaky crash could happen, just offer you some information
for reference. If signed in, the accounts previously on the device should
already in ATS during the startup, new accounts will be seeded and
OAuth2TokenService will fire refresh token available for these new accounts
to trigger AccountFetcherService start fetching accounts info. If not
signed in, OAuth2TokenService will fire refresh token available for all the
accounts on the device.

On Thu, Oct 1, 2015 at 10:33 AM, <anthonyvd@chromium.org> wrote:

>
>
>
https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/andro...
> File chrome/browser/android/signin/account_tracker_service_android.cc
> (right):
>
>
>
https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/andro...
> chrome/browser/android/signin/account_tracker_service_android.cc:45: }
> On 2015/10/01 14:17:38, Roger Tawa wrote:
>
>> I suspect this could come up in other situations too (i.e. we need to
>>
> seed ATS
>
>> earlier for some reason).  So I think it might be better to do the
>>
> following:
>
> - change AFS::EnableNetworkFetches() to be a noop instead of DCHECKing
>>
> if it has
>
>> already been called.  This should be easy since it already maintains a
>>
> bool
>
> - no need for bool here, just unconditionally call
>>
> EnableNetworkFetches()
>
> - undo the change in profile_manager.cc
>>
>
> Agreed that EnableNetworkFetches should be idempotent so removing the
> DCHECK and no-op'ing when it was already called is probably the right
> thing to do. It won't allow undoing the profile_manager.cc changes
> though (I think) since the goal is to delay the call to after accounts
> are seeded.
>
>
>
https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/profi...
> File chrome/browser/profiles/profile_manager.cc (right):
>
>
>
https://chromiumcodereview.appspot.com/1380103004/diff/1/chrome/browser/profi...
> chrome/browser/profiles/profile_manager.cc:1112: // Accounts need to be
> partially seeded on Android.
> Can you just add a little more to this comment (for future us
> :))?Something like "Accounts need to be partially seeded on Android
> before enabling network fetches because [reason]".
>
> https://chromiumcodereview.appspot.com/1380103004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[knn, 2015-10-01 16:25:23]

I have added the reason and also the stack trace for more context.

As Anthony explained, we can't solve this issue with NOPing multiple
EnableNetworkFetchs as we want to delay until the partial seeding has completed.
We can split it into ProfileLoaded and AccountIdMapInitialized methods if
you think the reverse order is likely to happen.

WDYT?

[Roger Tawa OOO till Jul 10th, 2015-10-02 02:50:47]

On 2015/10/01 16:25:23, knn wrote:
> I have added the reason and also the stack trace for more context.
> 
> As Anthony explained, we can't solve this issue with NOPing multiple
> EnableNetworkFetchs as we want to delay until the partial seeding has
completed.
> We can split it into ProfileLoaded and AccountIdMapInitialized methods if
> you think the reverse order is likely to happen.
> 
> WDYT?

I think you forgot to upload the patchset?

[knn, 2015-10-02 13:28:31]

There seems to be other cases where this crash could occur so I was wondering
whether a different solution is required here.
One is if an account is stopped being tracked by the AccountTrackerService and
Started again.

Another case is,
OAuth2TokenServiceDelegateAndroid::MapAccountNameToAccountId
OAuth2TokenServiceDelegateAndroid::ValidateAccounts
Java_org_chromium_chrome_browser_signin_OAuth2TokenService_nativeValidateAccounts

I haven't completely understood what is triggering the second one so I don't
know how to deal with it. Let me dig in some more.
I wanted to land some independent changes before the branch point today.

[knn, 2015-10-05 14:06:51]

PTAL. Thanks!

I have added defensive checks against the bad state.
I added a AccountSeedingTracker to hide the OS_ANDROID ifdefs otherwise it could
probably exist inside the AccountFetcherService itself.

[knn, 2015-10-05 14:07:15]

knn@chromium.org changed required reviewers:
- mlerman@chromium.org

[knn, 2015-10-05 14:07:31]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-05 14:07:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103004/20001

[knn, 2015-10-05 14:11:45]

knn@chromium.org changed reviewers:
- mlerman@chromium.org

[knn, 2015-10-05 14:11:45]

Removing Mike from review to avoid spamming him.

https://codereview.chromium.org/1380103004/diff/20001/components/signin/core/...
File components/signin/core/browser/account_fetcher_service.h (right):

https://codereview.chromium.org/1380103004/diff/20001/components/signin/core/...
components/signin/core/browser/account_fetcher_service.h:73: #if
defined(OS_ANDROID)
Can be removed.

[commit-bot: I haz the power, 2015-10-05 14:12:49]

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

[commit-bot: I haz the power, 2015-10-05 14:12:50]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[knn, 2015-10-05 14:24:13]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-05 14:24:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103004/40001

[commit-bot: I haz the power, 2015-10-05 14:32:30]

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

[commit-bot: I haz the power, 2015-10-05 14:32:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2015-10-05 16:05:50]

https://chromiumcodereview.appspot.com/1380103004/diff/40001/chrome/browser/a...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://chromiumcodereview.appspot.com/1380103004/diff/40001/chrome/browser/a...
chrome/browser/android/signin/account_tracker_service_android.cc:38:
AccountFetcherServiceFactory::GetForProfile(profile)->OnAccountsSeeded();
The idea behind splitting the AFS into the tracker and the fetcher was to break
some logical dependencies and make it so the Fetcher knew about the tracker but
not the other way around. If the AccountFetcherService cares about things that
happen in the AccountTrackerService, it should be done as the AFS observing the
ATS imo.

https://chromiumcodereview.appspot.com/1380103004/diff/40001/components/signi...
File components/signin/core/browser/account_seeding_tracker.h (right):

https://chromiumcodereview.appspot.com/1380103004/diff/40001/components/signi...
components/signin/core/browser/account_seeding_tracker.h:19: class
AccountSeedingTracker {
Since this class is so incredibly Android-specific, is there a way to just move
the work it does directly in AccountTrackerServiceAndroid?

[knn, 2015-10-05 16:35:55]

Thanks for taking a quick look, Anthony!

https://chromiumcodereview.appspot.com/1380103004/diff/40001/chrome/browser/a...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://chromiumcodereview.appspot.com/1380103004/diff/40001/chrome/browser/a...
chrome/browser/android/signin/account_tracker_service_android.cc:38:
AccountFetcherServiceFactory::GetForProfile(profile)->OnAccountsSeeded();
On 2015/10/05 16:05:50, anthonyvd wrote:
> The idea behind splitting the AFS into the tracker and the fetcher was to
break
> some logical dependencies and make it so the Fetcher knew about the tracker
but
> not the other way around. If the AccountFetcherService cares about things that
> happen in the AccountTrackerService, it should be done as the AFS observing
the
> ATS imo.

That ordering is still maintained (ATS does not know anything about the AFS). Do
you prefer the ATS to directly notify the AFS? That would add a lot of Android
specific APIs that I did not want to add.

https://chromiumcodereview.appspot.com/1380103004/diff/40001/components/signi...
File components/signin/core/browser/account_seeding_tracker.h (right):

https://chromiumcodereview.appspot.com/1380103004/diff/40001/components/signi...
components/signin/core/browser/account_seeding_tracker.h:19: class
AccountSeedingTracker {
On 2015/10/05 16:05:50, anthonyvd wrote:
> Since this class is so incredibly Android-specific, is there a way to just
move
> the work it does directly in AccountTrackerServiceAndroid?

You can think of this as dividing the AccountFetcherService rather than ATS into
Android specific and general parts. Another way to accomplish the separation
would have been to subclass AFS but composition is cleaner imho.
AccountTrackerServiceAndroid is really just a Java bridge. Appreciate any rename
suggestions there.

[anthonyvd, 2015-10-05 20:45:21]

https://chromiumcodereview.appspot.com/1380103004/diff/40001/chrome/browser/a...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://chromiumcodereview.appspot.com/1380103004/diff/40001/chrome/browser/a...
chrome/browser/android/signin/account_tracker_service_android.cc:38:
AccountFetcherServiceFactory::GetForProfile(profile)->OnAccountsSeeded();
On 2015/10/05 16:35:55, knn wrote:
> On 2015/10/05 16:05:50, anthonyvd wrote:
> > The idea behind splitting the AFS into the tracker and the fetcher was to
> break
> > some logical dependencies and make it so the Fetcher knew about the tracker
> but
> > not the other way around. If the AccountFetcherService cares about things
that
> > happen in the AccountTrackerService, it should be done as the AFS observing
> the
> > ATS imo.
> 
> That ordering is still maintained (ATS does not know anything about the AFS).
Do
> you prefer the ATS to directly notify the AFS? That would add a lot of Android
> specific APIs that I did not want to add.

My bad, I thought the AccountTrackerServiceAndroid was a subclass of
AccountTrackerService for some reason. You can disregard that.

https://chromiumcodereview.appspot.com/1380103004/diff/40001/components/signi...
File components/signin/core/browser/account_seeding_tracker.h (right):

https://chromiumcodereview.appspot.com/1380103004/diff/40001/components/signi...
components/signin/core/browser/account_seeding_tracker.h:19: class
AccountSeedingTracker {
On 2015/10/05 16:35:55, knn wrote:
> On 2015/10/05 16:05:50, anthonyvd wrote:
> > Since this class is so incredibly Android-specific, is there a way to just
> move
> > the work it does directly in AccountTrackerServiceAndroid?
> 
> You can think of this as dividing the AccountFetcherService rather than ATS
into
> Android specific and general parts. Another way to accomplish the separation
> would have been to subclass AFS but composition is cleaner imho.
> AccountTrackerServiceAndroid is really just a Java bridge. Appreciate any
rename
> suggestions there.

I see. Agreed that composition is better than inheritance.

My concern about this class is that it's present everywhere but only does
something on Android. For example, IsAccountSeeded() is misleading on desktop
because it always returns true, although that's not always correct. Same for
AreAllAccountsSeeded(), which is even worse since that doesn't really mean
anything except on Android.

I feel like this should be an Android-specific file/class (so only compile it
there and maybe have Android in the name) and the ATS can then decide if it uses
it depending on platform. WDYT?

[knn, 2015-10-06 14:11:37]

Anthony,
I think I have a better design now. Let me know if you still want me to compile
out the code.

[anthonyvd, 2015-10-06 18:18:55]

lgtm % small nit, although I'm not an OWNER in signin so Roger can take another
look.

Thanks for making it less Android-specific, I think your latest design is
cleaner and clearer :)

https://chromiumcodereview.appspot.com/1380103004/diff/80001/components/signi...
File components/signin/core/browser/account_fetcher_service.cc (right):

https://chromiumcodereview.appspot.com/1380103004/diff/80001/components/signi...
components/signin/core/browser/account_fetcher_service.cc:328: bool
AccountFetcherService::CanFetchAccountInfo(const std::string& account_id) {
Maybe rename this so that it's clearer that it can have side effects? As-is the
name implies it's only a boolean check but it can insert things in
|pending_user_info_fetches_|.

[Roger Tawa OOO till Jul 10th, 2015-10-13 12:40:56]

Hi Khannan,

See comments below.  I don't fully understand the "other" cases of the crash. 
For exaomple, OAuth2TokenServiceDelegateAndroid::ValidateAccounts() waits for
seeding to complete before doing anything.  Can you explain further?  Thanks.

https://codereview.chromium.org/1380103004/diff/80001/chrome/browser/android/...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://codereview.chromium.org/1380103004/diff/80001/chrome/browser/android/...
chrome/browser/android/signin/account_tracker_service_android.cc:39:
->OnAccountIdNameMapSeeded();
That doesn't seem right.  There should be no references from ATS to AFS.  While
this is just a helper function, it's called from ATSJ, so it really is a
reference from ATS.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
File components/signin/core/browser/account_fetcher_service.h (right):

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_fetcher_service.h:8: #include <map>
Doesn't look like we need map anymore.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:151: return
!GetAccountInfo(account_id).email.empty();
This should check if both gaiaid and email are not empty.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:156: if
(account.second.info.email.empty())
Same as previous comment.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:160: }
Seems like this function is the same as IsMigratable() when
!defined(OS_CHROMEOS).

[knn, 2015-10-13 13:15:42]

Hi Roger,

The 'other' cases of crashes are when the accounts in the device changes at any
point and the AFS tries to do something. I also added an additional condition in
the isSystemAccountsSeeded() to handle the ValidateAccounts call.

These are the only 2 entry points to the crash.

https://codereview.chromium.org/1380103004/diff/80001/chrome/browser/android/...
File chrome/browser/android/signin/account_tracker_service_android.cc (right):

https://codereview.chromium.org/1380103004/diff/80001/chrome/browser/android/...
chrome/browser/android/signin/account_tracker_service_android.cc:39:
->OnAccountIdNameMapSeeded();
On 2015/10/13 12:40:55, Roger Tawa wrote:
> That doesn't seem right.  There should be no references from ATS to AFS. 
While
> this is just a helper function, it's called from ATSJ, so it really is a
> reference from ATS.

The class name is giving a false sense of dependency violation here both
AccountTrackerServiceAndroid and the Java counter part only handle Seeding
Account info and have nothing to do specifically with the ATS or AFS. Happy to
rename both.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
File components/signin/core/browser/account_fetcher_service.cc (right):

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_fetcher_service.cc:328: bool
AccountFetcherService::CanFetchAccountInfo(const std::string& account_id) {
On 2015/10/06 18:18:55, anthonyvd wrote:
> Maybe rename this so that it's clearer that it can have side effects? As-is
the
> name implies it's only a boolean check but it can insert things in
> |pending_user_info_fetches_|.

Will do.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
File components/signin/core/browser/account_fetcher_service.h (right):

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_fetcher_service.h:8: #include <map>
On 2015/10/13 12:40:56, Roger Tawa wrote:
> Doesn't look like we need map anymore.

Will remove.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:151: return
!GetAccountInfo(account_id).email.empty();
On 2015/10/13 12:40:56, Roger Tawa wrote:
> This should check if both gaiaid and email are not empty.

Done.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:156: if
(account.second.info.email.empty())
On 2015/10/13 12:40:56, Roger Tawa wrote:
> Same as previous comment.

Done.

https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:160: }
On 2015/10/13 12:40:56, Roger Tawa wrote:
> Seems like this function is the same as IsMigratable() when
> !defined(OS_CHROMEOS).

My bad. Do you think it makes sense to call this from IsMigratable() for a more
understandable name?

[gogerald1, 2015-10-13 16:34:53]

Hi Khannan,

I left few comments there for your reference. Still don't see how it defends
asynchronous seeding process caused problem. The seeding process is called on UI
thread, and all of the operations are synchronous except fetching of Gaia id.
That means all the accounts in ATS should have valid map of Id<->name if it use
gaia id as account id (migratable). Otherwise the data was corrupted, then we
may need to know why and how it was corrupted.

Ganggui,

https://codereview.chromium.org/1380103004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java
(right):

https://codereview.chromium.org/1380103004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:73:
&& nativeAreAllAccountsSeeded()) {
seedSystemAccounts is called on UI thread and SeedAccountsInfo() in
account_tracker_service_android.cc is an synchronous process, so check of
SEEDING_DONE may be enough.

https://codereview.chromium.org/1380103004/diff/100001/components/signin/core...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1380103004/diff/100001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:152: return
!account.email.empty() && !account.gaia.empty();
This is always true, if IsMigratable() is true and account_id is one of the
account in ATS. Otherwise the data was corrupted.

https://codereview.chromium.org/1380103004/diff/100001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:158: if
(account_info.email.empty() || account_info.gaia.empty())
Same as above comments

[chromium-reviews, 2015-10-13 16:43:14]

"accounts in the device changes at any point and the AFS tries to do
something"-->AFS should only try to do something with accounts in ATS or
OAuth2TokenService (except prefetch from java). SigninHelper.java will
force refresh accounts' information in ATS and validate accounts in
OAuth2TokenService before AFS notice the changes (do anything).

On Tue, Oct 13, 2015 at 9:15 AM, <knn@chromium.org> wrote:

> Hi Roger,
>
> The 'other' cases of crashes are when the accounts in the device changes
> at any
> point and the AFS tries to do something. I also added an additional
> condition in
> the isSystemAccountsSeeded() to handle the ValidateAccounts call.
>
> These are the only 2 entry points to the crash.
>
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/chrome/browser/android/...
> File chrome/browser/android/signin/account_tracker_service_android.cc
> (right):
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/chrome/browser/android/...
> chrome/browser/android/signin/account_tracker_service_android.cc:39:
> ->OnAccountIdNameMapSeeded();
> On 2015/10/13 12:40:55, Roger Tawa wrote:
>
>> That doesn't seem right.  There should be no references from ATS to
>>
> AFS.  While
>
>> this is just a helper function, it's called from ATSJ, so it really is
>>
> a
>
>> reference from ATS.
>>
>
> The class name is giving a false sense of dependency violation here both
> AccountTrackerServiceAndroid and the Java counter part only handle
> Seeding Account info and have nothing to do specifically with the ATS or
> AFS. Happy to rename both.
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> File components/signin/core/browser/account_fetcher_service.cc (right):
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> components/signin/core/browser/account_fetcher_service.cc:328: bool
> AccountFetcherService::CanFetchAccountInfo(const std::string&
> account_id) {
> On 2015/10/06 18:18:55, anthonyvd wrote:
>
>> Maybe rename this so that it's clearer that it can have side effects?
>>
> As-is the
>
>> name implies it's only a boolean check but it can insert things in
>> |pending_user_info_fetches_|.
>>
>
> Will do.
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> File components/signin/core/browser/account_fetcher_service.h (right):
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> components/signin/core/browser/account_fetcher_service.h:8: #include
> <map>
> On 2015/10/13 12:40:56, Roger Tawa wrote:
>
>> Doesn't look like we need map anymore.
>>
>
> Will remove.
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> File components/signin/core/browser/account_tracker_service.cc (right):
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> components/signin/core/browser/account_tracker_service.cc:151: return
> !GetAccountInfo(account_id).email.empty();
> On 2015/10/13 12:40:56, Roger Tawa wrote:
>
>> This should check if both gaiaid and email are not empty.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> components/signin/core/browser/account_tracker_service.cc:156: if
> (account.second.info.email.empty())
> On 2015/10/13 12:40:56, Roger Tawa wrote:
>
>> Same as previous comment.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1380103004/diff/80001/components/signin/core/...
> components/signin/core/browser/account_tracker_service.cc:160: }
> On 2015/10/13 12:40:56, Roger Tawa wrote:
>
>> Seems like this function is the same as IsMigratable() when
>> !defined(OS_CHROMEOS).
>>
>
> My bad. Do you think it makes sense to call this from IsMigratable() for
> a more understandable name?
>
> https://codereview.chromium.org/1380103004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nyquist, 2015-10-15 19:33:45]

this looks reasonable to me, but it'd be great if rogerta@ also could take a
look at this.

https://codereview.chromium.org/1380103004/diff/100001/components/signin/core...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1380103004/diff/100001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:152: return
!account.email.empty() && !account.gaia.empty();
On 2015/10/13 16:34:53, gogerald1 wrote:
> This is always true, if IsMigratable() is true and account_id is one of the
> account in ATS. Otherwise the data was corrupted.

I thought GetAccountInfo(...) returns an empty struct if the account does not
exist in the mapping?

https://codereview.chromium.org/1380103004/diff/100001/google_apis/gaia/oauth...
File google_apis/gaia/oauth2_token_service_delegate.h (right):

https://codereview.chromium.org/1380103004/diff/100001/google_apis/gaia/oauth...
google_apis/gaia/oauth2_token_service_delegate.h:58: // uses the account id.
This mapping needs to be seeded usage.
The last sentence here seems a bit off. Do you mean that we need to seed the
account id <-> name map if necessary, i.e. this method returns true?

[knn, 2015-10-16 13:30:52]

Sorry about the lack of updates here.
This bug is getting more and more difficult to replicate.
While there are definitely tests that put you in an invalid state(resulting in a
crash),
I was able to reproduce it even w/o that before using a combination of Play
service missing
and interacting with multiple accounts.

I have split up the unrelated changes and have a fix at least for some tests
here (http://crrev.com/1409083002). Some unrelated failing tests can be
re-enabled after that perhaps.

We can proceed with this patch if Roger agrees this is a correct way to make it
robust.
Otherwise I'll need some more time to investigate as all the interactions are
not very obvious (to me).

[Roger Tawa OOO till Jul 10th, 2015-10-20 19:59:14]

On 2015/10/16 13:30:52, knn wrote:
> Sorry about the lack of updates here.
> This bug is getting more and more difficult to replicate.
> While there are definitely tests that put you in an invalid state(resulting in
a
> crash),
> I was able to reproduce it even w/o that before using a combination of Play
> service missing
> and interacting with multiple accounts.
> 
> I have split up the unrelated changes and have a fix at least for some tests
> here (http://crrev.com/1409083002). Some unrelated failing tests can be
> re-enabled after that perhaps.
> 
> We can proceed with this patch if Roger agrees this is a correct way to make
it
> robust.
> Otherwise I'll need some more time to investigate as all the interactions are
> not very obvious (to me).

No worries Khannan.

This CL contains two separate changes.  The first is a cleanup of the JNI stuff
and that looks good.  Maybe factor that out into its own CL.

The second is to delay the AFS fetches until the ATS has a mapping from gaiaid
to email for all accounts.  I still don't believe this is the correct approach. 
Turns out that on desktop we also have a case where AFS is fetching too early,
see Anthony's CL https://codereview.chromium.org/1376933005/.  In fact I haven't
l-g-t-m'ed Anthony's CL because I think it won't be needed once this CL lands.

i think the right approach is for AFS to delay all network fetches until these
two conditions are met:

1/ EnableNetworkFetches() is called
2/ OnRefreshTokensLoaded() is called

(Keep in mind that the token service guarantees that OnRefreshTokensLoaded()
happens after the ATS has a mapping from gaiaid to email for all accounts.) 
Implemented this way, there is no need to change the delegate interface, ATS,
O2TSDA, nor call from ATSJ to AFS which is a PKS dependency violation.  If you
also factor out the JNI changes to another CL, this CL might only need a few
lines of code in AFS.  What do you think?

[knn, 2015-10-21 07:35:42]

Thanks for explaining Roger!
Agreed. Let us go with delaying the AFS since that will fix the other issue as
well.

I am uploading the cl for tryjobs now. I'll update comments/issue description
later(I will be OOO first half of the day due to some last minute work).

[knn, 2015-10-21 13:11:59]

Description was changed from

==========
[Android]Ensure Account ID <-> Name mapping is seeded before any fetching.

Android uses the Account Name (email) as an identifier whereas Chrome
uses the Account ID (GAIA ID). Hence we need to seed this mapping by
querying Google Play services before it is used.

However the seeding happens asynchronously whereas
EnableNetworkFetches is called on loading the profile.
Hence this causes a flaky crash on unrelated changes that may affect the
timeline of the two methods.

This cl adds defensive checks in place that will detect this condition
and defer processing until the account mapping has been seeded.

Refer to the bug for crash stack traces.

BUG=535211
==========

to

==========
Delay fetching account info until OnRefreshTokensLoaded().

This will resolve 2 issues:
1> Android OS uses email id whereas Chrome uses the GAIA id to identify
the account. This translation is only guaranteed to be available after
OnRefreshTokensLoaded(), which is required before fetching anything.
2> In case of changes to the account, force refreshing before
OnRefreshTokensLoaded() will result in stale values which are not
updated.

BUG=535211,525207
==========

[knn, 2015-10-21 13:12:00]

knn@chromium.org changed reviewers:
- nyquist@chromium.org

[knn, 2015-10-21 13:12:00]

knn@chromium.org changed required reviewers:
- nyquist@chromium.org

[knn, 2015-10-21 13:13:50]

Roger, PTAL. Thanks!

[Roger Tawa OOO till Jul 10th, 2015-10-21 19:09:00]

Nice Khannan!  I think we need to keep the EnableNetworkFetches() function and
the call to it though.  If you start with a non-signed in profile and then sign
in, OnRefreshTokensLoaded() won't get called (I should have mentioned that).  So
network_fetches_enabled_ will never be set to true.

I also think we shouldn't silently drop calls to StartFetchingAccountInfo() if
network_fetches_enabled_ is false.  So I think we should keep the pending list.

(This is complicated code and my recommendations were not clear enough, so sorry
about that.)

So EnableNetworkFetches() should remain.  It should do everything it does today
except process the pending list.  However, it should set
network_fetches_enabled_ *after* calling ScheduleNextRefresh().  This makes sure
if ScheduleNextRefresh() forces a fetch, it still won't happen until
OnRefreshTokensLoaded() is called.

OnRefreshTokensLoaded() should process pending list and then do what it does
today.

Feel free to email or IM me if anything I've said is still unclear.

https://codereview.chromium.org/1380103004/diff/140001/components/signin/core...
File components/signin/core/browser/account_fetcher_service.cc (right):

https://codereview.chromium.org/1380103004/diff/140001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:202: "422460
AccountFetcherService::OnRefreshTokenAvailable"));
Nit: fix method name in string.

[knn, 2015-10-22 11:06:22]

If I understand correctly what you mean is:

EnableNetworkFetches() {
  ScheduleNextRefresh();
  network_fetches_enabled_ = true;
}

OnRefreshTokenLoaded() {
  Process pending set
  Sanity refresh invalid account info
}

This works based on the fact that on first signin when OnRefreshTokenLoaded
is not fired we should not have any pending fetches anyway.
There seem to edge cases here like if the timer goes off before
OnRefreshTokenLoaded could be fired, although unlikely.
I guess we could put OnRefreshTokenLoaded not being fired in the same bucket as
OnUserInfoFetchFailure, so that is fine.
The reason why I got rid of the pending fetches is we also need to ensure we
remove them if OnRefreshTokenRevoked is fired.
I agree silently dropping fetches is bad as well, perhaps we should handle
not fetching and just loading valid account info from pref separately
similar to only_fetch_if_invalid? WDYT?

Finally I feel things would get less complicated if we had a single token
service is ready call regardless of 1st signin, Can we get OnRefreshTokenLoaded
even for signin or is that another beast?

Sorry about the delay in responding.

[knn, 2015-10-23 09:50:43]

Roger,
PTAL at the new patch which always fires OnRefreshTokenLoaded. :)
I have removed the dependency of AFS on the invalidation service to avoid a
cycle.
We can safely inject it in before it is used though and assert that, whereas
the dependency of SM on AFS is easy to miss as only the OnRefreshTokenLoaded
notification will not come.

[Roger Tawa OOO till Jul 10th, 2015-10-23 12:42:12]

lgtm

Please wait for another lgtm from Anthony, since the CL has changed a lot since
he gave his :-)

Thanks for all the work Khannan!

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
File components/signin/core/browser/account_fetcher_service.cc (right):

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:88:
RefreshAllAccountInfo(true);
We no longer need this call here.  It is a noop with this CL anyway I think,
since StartFetchingUserInfo() silently drops fetches when network is not
enabled.  Since we now assume that OnRefreshTokensLoaded() is always called, the
refresh will happen there.

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:174: return;
Let's change this silent drop to:

  DCHECK(network_fetches_enabled_);

If you do as I mentioned at line 88 above, I think this should be OK.

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:207: "422460
AccountFetcherService::OnRefreshTokenAvailable"));
Nit: change to match function name.

[knn, 2015-10-23 13:32:52]

knn@chromium.org changed reviewers:
+ mlerman@chromium.org

[knn, 2015-10-23 13:32:53]

Thanks for the review Roger!
I'll wait for Anthony's review but also
adding back Mike since it looks like I am using his lgtm here. Mike PTAL,
Thanks!

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
File components/signin/core/browser/account_fetcher_service.cc (right):

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:88:
RefreshAllAccountInfo(true);
On 2015/10/23 12:42:12, Roger Tawa wrote:
> We no longer need this call here.  It is a noop with this CL anyway I think,
> since StartFetchingUserInfo() silently drops fetches when network is not
> enabled.  Since we now assume that OnRefreshTokensLoaded() is always called,
the
> refresh will happen there.

Done.

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:174: return;
On 2015/10/23 12:42:12, Roger Tawa wrote:
> Let's change this silent drop to:
> 
>   DCHECK(network_fetches_enabled_);
> 
> If you do as I mentioned at line 88 above, I think this should be OK.

Done. Silently dropping in OnRefreshToken{Available/Revoked} instead.

https://codereview.chromium.org/1380103004/diff/180001/components/signin/core...
components/signin/core/browser/account_fetcher_service.cc:207: "422460
AccountFetcherService::OnRefreshTokenAvailable"));
On 2015/10/23 12:42:12, Roger Tawa wrote:
> Nit: change to match function name.

Done.

[knn, 2015-10-23 13:39:56]

knn@chromium.org changed reviewers:
+ pavely@chromium.org

[knn, 2015-10-23 13:39:56]

pavely@chromium.org: Please review changes in 
components/invalidation/*
I have added a new Invalidation state to notify shutdown to the handlers.
This is because AccountFetcherService cannot have a BCKS dependency on
InvalidationService anymore and needs some way to unregister itself before
shutdown.

[knn, 2015-10-23 13:55:46]

Description was changed from

==========
Delay fetching account info until OnRefreshTokensLoaded().

This will resolve 2 issues:
1> Android OS uses email id whereas Chrome uses the GAIA id to identify
the account. This translation is only guaranteed to be available after
OnRefreshTokensLoaded(), which is required before fetching anything.
2> In case of changes to the account, force refreshing before
OnRefreshTokensLoaded() will result in stale values which are not
updated.

BUG=535211,525207
==========

to

==========
Delay fetching account info until OnRefreshTokensLoaded().

Always fire OnRefreshTokensLoaded after token service is ready.
Previously this was only called if previously signed in.

Also fix a missing BCKS dependency of the SigninManager on the
AccountFetcherService.
This causes a cyclic dependency among:
AccountFetcherService - SigninManager - InvalidationProvider
Solution is to inject InvalidationService before it is used.

Added a Shutdown notification from InvalidationService to facilitate this.

This will resolve 2 issues:
1> Android OS uses email id whereas Chrome uses the GAIA id to identify
the account. This translation is only guaranteed to be available after
OnRefreshTokensLoaded(), which is required before fetching anything.
2> In case of changes to the account, force refreshing before
OnRefreshTokensLoaded() will result in stale values which are not
updated.

BUG=535211,525207
==========

[anthonyvd, 2015-10-23 14:22:55]

lgtm I think this simplifies the AFS a lot as well. Thanks :)

[knn, 2015-10-26 13:53:44]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-26 13:54:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103004/200001

[commit-bot: I haz the power, 2015-10-26 14:17:37]

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

[commit-bot: I haz the power, 2015-10-26 14:17:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[knn, 2015-10-27 19:56:12]

On 2015/10/23 13:39:56, knn wrote:
> mailto:pavely@chromium.org: Please review changes in 
> components/invalidation/*
> I have added a new Invalidation state to notify shutdown to the handlers.
> This is because AccountFetcherService cannot have a BCKS dependency on
> InvalidationService anymore and needs some way to unregister itself before
> shutdown.

Friendly ping Pavel.

[pavely, 2015-10-28 02:09:00]

lgtm

[knn, 2015-10-28 16:51:00]

Hi Roger, Anthony,

I had to make some more changes to the AFS relating to how it schedules
refreshes.

Many unit_tests specifically RunUntilIdle
(https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sim...)
assume that there are no loops in pending tasks.
I can't think of a clean fix to this(w/o a major yak shave) as there are many
tests that fail in spectacular ways.

PTAL. Thanks!

[knn, 2015-10-28 16:51:13]

The CQ bit was checked by knn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-28 16:52:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103004/220001

[Roger Tawa OOO till Jul 10th, 2015-10-28 18:05:51]

Thanks for the heads up Khannan.  Actually, I would like a change to this new
code.

Nit: I prefer positive member names rather than negative ones.  Could you rename
new member to scheduled_refresh_enabled_ ?

Since this is only for testing purposes, could we not add this argument to the
Initialize() method.  Instead add a new method like:

  void disable_scheduled_refresh_for_testing();

and call this in:

AccountFetcherServiceFactory::BuildServiceInstanceFor()
FakeAccountFetcherServiceBuilder::BuildForTests() maybe?

either before or after Initialize().  Does that make sense?

[commit-bot: I haz the power, 2015-10-28 18:21:44]

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

[commit-bot: I haz the power, 2015-10-28 18:21:45]

Dry run: This issue passed the CQ dry run.

[knn, 2015-10-28 20:44:44]

Absolutely! Done. PTAL. Thanks!

[Roger Tawa OOO till Jul 10th, 2015-10-28 21:05:57]

lgtm

Thanks Khannan.

[knn, 2015-10-29 06:42:52]

The CQ bit was checked by knn@chromium.org

[knn, 2015-10-29 06:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, pavely@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/1380103004/#ps240001
(title: "extract separate method")

[commit-bot: I haz the power, 2015-10-29 06:43:07]

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

[commit-bot: I haz the power, 2015-10-29 06:59:27]

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

[commit-bot: I haz the power, 2015-10-29 06:59:28]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[knn, 2015-10-29 09:14:37]

Patchset #14 (id:260001) has been deleted

[knn, 2015-10-29 09:32:09]

knn@chromium.org changed reviewers:
+ msarda@chromium.org

[knn, 2015-10-29 09:32:10]

Mihai: Please review changes in 
 ios/chrome/browser/signin/account_fetcher_service_factory.cc

invalidation_service is no longer passed at Initialize but is setup in
profile_manager.cc (It looks like iOS shares this code so no further changes
should be required)

[Bernhard Bauer, 2015-10-29 09:42:28]

On 2015/10/28 16:51:00, knn wrote:
> Hi Roger, Anthony,
> 
> I had to make some more changes to the AFS relating to how it schedules
> refreshes.
> 
> Many unit_tests specifically RunUntilIdle
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sim...)
> assume that there are no loops in pending tasks.
> I can't think of a clean fix to this(w/o a major yak shave) as there are many
> tests that fail in spectacular ways.

This is why RunUntilIdle is bad 😉

The way that this is solved in content/ (where a lot of things continuously
schedule stuff, like animations) is by running pending tasks a finite but large
enough number of times. content::RunMessageLoop() will do that for you.

[knn, 2015-10-29 09:59:41]

On 2015/10/29 09:42:28, Bernhard Bauer wrote:
> On 2015/10/28 16:51:00, knn wrote:
> > Hi Roger, Anthony,
> > 
> > I had to make some more changes to the AFS relating to how it schedules
> > refreshes.
> > 
> > Many unit_tests specifically RunUntilIdle
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sim...)
> > assume that there are no loops in pending tasks.
> > I can't think of a clean fix to this(w/o a major yak shave) as there are
many
> > tests that fail in spectacular ways.
> 
> This is why RunUntilIdle is bad 😉
> 
> The way that this is solved in content/ (where a lot of things continuously
> schedule stuff, like animations) is by running pending tasks a finite but
large
> enough number of times. content::RunMessageLoop() will do that for you.

:( We should deprecate RunUntilIdle/QuitWhenIdle methods in favor of finite
runs.

[msarda, 2015-10-29 10:57:55]

iOS changes LGTM.

[knn, 2015-10-29 11:23:02]

The CQ bit was checked by knn@chromium.org

[knn, 2015-10-29 11:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, rogerta@chromium.org, pavely@chromium.org,
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/1380103004/#ps280001
(title: "fix iOs")

[commit-bot: I haz the power, 2015-10-29 11:23:10]

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

[commit-bot: I haz the power, 2015-10-29 12:14:20]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-10-29 12:15:09]

Patchset 14 (id:??) landed as
https://crrev.com/3a5f21ae2dfb0b5fa385b1eb75c8e2a66f4fbdb5
Cr-Commit-Position: refs/heads/master@{#356820}