Fix issues with token refresh in AccountReconcilor

Fix issues with token refresh in AccountReconcilor

The CL was change a bit as suggested by the reviewers.

This CL now introduces a worklist model in GoogleAutoLoginHelper. Instead of firing all the refresh token events in a row, it queues up all the login requests and insert GAIA cookies one-by-one. This would let accounts to be appended instead of overwriting one another.

This also introduces GoogleAutoLoginHelperTest for some unit testing.

BUG=305249
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238547

[Roger Tawa OOO till Jul 10th, 2013-11-08 13:17:31]

Hi Alan,

See comments below, but I think you should be able to achieve the same goal
without any of the changes to the O2TSs and the AC.  You should write some tests
for new functionality of the helper.

https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
chrome/browser/signin/account_reconcilor.cc:155: }
I don't think its a good idea to declare a new callback method in the observer. 
This now forces all observers to implement this one too.  Also see comment
below.

https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
chrome/browser/signin/account_reconcilor.cc:175: }
If you change the helper so that LogIn(std::string) queues up the merge request
if one is already started, then you don't need the LogIn(std::deque) version.

[acleung1, 2013-11-08 22:21:48]

https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
File chrome/browser/signin/account_reconcilor.cc (right):

https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
chrome/browser/signin/account_reconcilor.cc:175: }
On 2013/11/08 13:17:31, Roger Tawa wrote:
> If you change the helper so that LogIn(std::string) queues up the merge
request
> if one is already started, then you don't need the LogIn(std::deque) version.

In that case, wouldn't I need to do a lot more work making the helper thread
safe?

[Roger Tawa OOO till Jul 10th, 2013-11-09 02:31:39]

On 2013/11/08 22:21:48, acleung1 wrote:
>
https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
> File chrome/browser/signin/account_reconcilor.cc (right):
> 
>
https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
> chrome/browser/signin/account_reconcilor.cc:175: }
> On 2013/11/08 13:17:31, Roger Tawa wrote:
> > If you change the helper so that LogIn(std::string) queues up the merge
> request
> > if one is already started, then you don't need the LogIn(std::deque)
version.
> 
> In that case, wouldn't I need to do a lot more work making the helper thread
> safe?

I don't think so. The helper only works on one thread at a time, although that
thread can be any thread. Much like URL fetcher. At the moment, the AC runs in
the ui thread. When we move it to another thread, we'll need to make sure it
still accesses the helper from one thread.

[acleung1, 2013-11-18 21:17:54]

You are right. The URL fetcher actually guarantee to execute the callback on the
calling thread. Seems like there is not worry that there are multiple threads
calling the helper.

I removed the added events. Please take another look.

Thanks Roger.


On 2013/11/09 02:31:39, Roger Tawa wrote:
> On 2013/11/08 22:21:48, acleung1 wrote:
> >
>
https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
> > File chrome/browser/signin/account_reconcilor.cc (right):
> > 
> >
>
https://codereview.chromium.org/63253003/diff/90001/chrome/browser/signin/acc...
> > chrome/browser/signin/account_reconcilor.cc:175: }
> > On 2013/11/08 13:17:31, Roger Tawa wrote:
> > > If you change the helper so that LogIn(std::string) queues up the merge
> > request
> > > if one is already started, then you don't need the LogIn(std::deque)
> version.
> > 
> > In that case, wouldn't I need to do a lot more work making the helper thread
> > safe?
> 
> I don't think so. The helper only works on one thread at a time, although that
> thread can be any thread. Much like URL fetcher. At the moment, the AC runs in
> the ui thread. When we move it to another thread, we'll need to make sure it
> still accesses the helper from one thread.

[Roger Tawa OOO till Jul 10th, 2013-11-21 16:02:31]

Sorry for late response Alan.  See questions below.  Also would need some unit
tests for new google auth login helper queuing.

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/android/s...
File chrome/browser/android/signin/signin_manager_android.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/android/s...
chrome/browser/android/signin/signin_manager_android.cc:230: }
Given the changes in android_profile_oauth2_token_service.cc, I think it would
it be better to call AO2TS::ValidateAccounts() here instead of duplicating that
code.

Given the necessary java to c++ conversions, maybe add a new function like:

AO2TS::ValidateAccountsImpl(const std::vector<std::string>& accounts, const
std::string& primary_account) 

that is called directly from here, and change AO2TS::ValidateAccounts() to
convert from java to c++ and call ValidateAccountsImpl() too?

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:173:
FireRefreshTokenAvailable(signed_in_account);
Why is it important to fire the primary account first?

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.h:55: const
std::string& account_id) OVERRIDE;
Don't this change if ValidateAccounts() is called LogInSignedInUser().

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:57:
base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
If the |accounts_| list is not empty, should it keep going?  Would this depend
on the kind of error returned?  If we do decide to abort the whole thing, should
the list be cleared out?  Same for error at line 40 above.

[acleung1, 2013-11-26 09:28:31]

Hello Roger

This CL is ready for another look.

-Alan

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/android/s...
File chrome/browser/android/signin/signin_manager_android.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/android/s...
chrome/browser/android/signin/signin_manager_android.cc:230: }
On 2013/11/21 16:02:32, Roger Tawa wrote:
> Given the changes in android_profile_oauth2_token_service.cc, I think it would
> it be better to call AO2TS::ValidateAccounts() here instead of duplicating
that
> code.
> 
> Given the necessary java to c++ conversions, maybe add a new function like:
> 
> AO2TS::ValidateAccountsImpl(const std::vector<std::string>& accounts, const
> std::string& primary_account) 
> 
> that is called directly from here, and change AO2TS::ValidateAccounts() to
> convert from java to c++ and call ValidateAccountsImpl() too?

Less duplicated code. I like that. Done.

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:173:
FireRefreshTokenAvailable(signed_in_account);
On 2013/11/21 16:02:32, Roger Tawa wrote:
> Why is it important to fire the primary account first?

Because that becomes the first account in the gaia cookie which I think is
probably what the user would like to see.

Do think it should just be the order which android account manager returns us?

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.h:55: const
std::string& account_id) OVERRIDE;
On 2013/11/21 16:02:32, Roger Tawa wrote:
> Don't this change if ValidateAccounts() is called LogInSignedInUser().

Yes. Since I have refactored ValidateAccounts just now, we can avoid making this
change.

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:57:
base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
On 2013/11/21 16:02:32, Roger Tawa wrote:
> If the |accounts_| list is not empty, should it keep going?  Would this depend
> on the kind of error returned?  If we do decide to abort the whole thing,
should
> the list be cleared out?  Same for error at line 40 above.

Not completely sure. I think continuing makes more sense. Let say you have 4
accounts and one of them fail to get auth token for various reason, you should
probably still see 3 on gmail.

I added a TODO instead.

[Roger Tawa OOO till Jul 10th, 2013-11-27 18:45:42]

lgtm, a few nits below.

For the TODO, I created a new bug so that this does not fall between the cracks:
crbug.com/323996

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
File chrome/browser/signin/android_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/63253003/diff/270001/chrome/browser/signin/an...
chrome/browser/signin/android_profile_oauth2_token_service.cc:173:
FireRefreshTokenAvailable(signed_in_account);
On 2013/11/26 09:28:31, acleung1 wrote:
> On 2013/11/21 16:02:32, Roger Tawa wrote:
> > Why is it important to fire the primary account first?
> 
> Because that becomes the first account in the gaia cookie which I think is
> probably what the user would like to see.
> 
> Do think it should just be the order which android account manager returns us?


No, you are correct Alan.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper.cc (right):

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:15:
GoogleAutoLoginHelper::~GoogleAutoLoginHelper() {}
Should DCHECK that |accounts_| is empty.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:18:
uber_token_fetcher_.reset(CreateNewUbertokenFetcher());
Add a DCHECK that |accounts_| is empty.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:32: VLOG(1) <<
"GoogleAutoLoginHelper::OnUbertokenSuccess" << uber_token;
Nit: put a space after "Success" in string?

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:59: // take different actions
instead of just throw our hands up.
sgtm, but for now please clear out the |accounts_| member, since we are ignoring
them any way.  Clear out at line 40 above too.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper_unittest.cc (right):

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper_unittest.cc:46: virtual
UbertokenFetcher* CreateNewUbertokenFetcher() {
missing OVERRIDE?

[acleung1, 2013-11-28 00:52:44]

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper.cc (right):

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:15:
GoogleAutoLoginHelper::~GoogleAutoLoginHelper() {}
On 2013/11/27 18:45:43, Roger Tawa wrote:
> Should DCHECK that |accounts_| is empty.

Done.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:18:
uber_token_fetcher_.reset(CreateNewUbertokenFetcher());
On 2013/11/27 18:45:43, Roger Tawa wrote:
> Add a DCHECK that |accounts_| is empty.

Done.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:32: VLOG(1) <<
"GoogleAutoLoginHelper::OnUbertokenSuccess" << uber_token;
On 2013/11/27 18:45:43, Roger Tawa wrote:
> Nit: put a space after "Success" in string?

Good point. Actually, the uber token's too noisy. I am removing it instead.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper.cc:59: // take different actions
instead of just throw our hands up.
On 2013/11/27 18:45:43, Roger Tawa wrote:
> sgtm, but for now please clear out the |accounts_| member, since we are
ignoring
> them any way.  Clear out at line 40 above too.

Done.

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
File chrome/browser/signin/google_auto_login_helper_unittest.cc (right):

https://codereview.chromium.org/63253003/diff/430001/chrome/browser/signin/go...
chrome/browser/signin/google_auto_login_helper_unittest.cc:46: virtual
UbertokenFetcher* CreateNewUbertokenFetcher() {
On 2013/11/27 18:45:43, Roger Tawa wrote:
> missing OVERRIDE?

Done.

[acleung1, 2013-11-28 00:54:38]

Hey Tommy, can I get a lgtm on the chrome/browser/android/signin changes?

Thanks.

[commit-bot: I haz the power, 2013-11-28 01:17:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/450001

[commit-bot: I haz the power, 2013-11-28 01:59:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/450001

[commit-bot: I haz the power, 2013-11-28 02:20:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/450001

[commit-bot: I haz the power, 2013-11-28 02:27:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/450001

[commit-bot: I haz the power, 2013-11-28 02:58:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/450001

[commit-bot: I haz the power, 2013-11-28 03:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/450001

[commit-bot: I haz the power, 2013-11-28 03:57:22]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[nyquist, 2013-12-02 22:47:01]

chrome/browser/android/signin/ lgtm

[commit-bot: I haz the power, 2013-12-02 23:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/550001

[commit-bot: I haz the power, 2013-12-03 23:35:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/63253003/590001

[commit-bot: I haz the power, 2013-12-04 03:33:25]

Change committed as 238547