Fix race issue in ArcAuthService.

Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked()
can be invoked twice (or more) quickly. This problem will be addressed in another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/f44a3339cce8208c9b308fe95d99bc5faf7fbfc5
Cr-Commit-Position: refs/heads/master@{#436349}

[hidehiko, 2016-12-02 17:51:01]

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

[commit-bot: I haz the power, 2016-12-02 17:51:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hidehiko, 2016-12-02 17:52:12]

hidehiko@chromium.org changed reviewers:
+ lhchavez@chromium.org

[hidehiko, 2016-12-02 17:52:13]

PTAL: lhchavez@. FYI: khmel@, poromov@.

[commit-bot: I haz the power, 2016-12-02 19:16:31]

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

[commit-bot: I haz the power, 2016-12-02 19:16:33]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-12-03 01:27:54]

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:231: // Optionally retrive auth
code in silent mode.
nit: s/retrive/retrieve/

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/auth/arc_auth_code_fetcher.h (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/auth/arc_auth_code_fetcher.h:22: virtual void
Fetch(const FetchCallback& callback) = 0;
Maybe mention that it's not legal to call Fetch() more than once per instance.

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc:66:
FetchInternal();
Don't we want to transition to another page at this point such that even if the
user clicks "TRY AGAIN" not too quickly, it is possible to have several such
events in the queue and some of them being interleaved with the callback from
OnContextPrepared? (thus working around the check in L64).

[Luis Héctor Chávez, 2016-12-03 14:24:26]

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc:66:
FetchInternal();
On 2016/12/03 01:27:54, Luis Héctor Chávez wrote:
> Don't we want to transition to another page at this point such that even if
the
> user clicks "TRY AGAIN" not too quickly, it is possible to have several such
> events in the queue and some of them being interleaved with the callback from
> OnContextPrepared? (thus working around the check in L64).

i thought about this a bit more: that would still be susceptible to races. how
about you disable the button once it's pressed from js so that it's impossible
for this scenario to happen? you can still transition to the "just one sec..."
page here just to give feedback in case the context creation takes a bit longer
than expected.

[hidehiko, 2016-12-05 05:55:58]

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

[commit-bot: I haz the power, 2016-12-05 05:56:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hidehiko, 2016-12-05 06:00:31]

Thank you for reivew. PTAL.

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:231: // Optionally retrive auth
code in silent mode.
On 2016/12/03 01:27:54, Luis Héctor Chávez wrote:
> nit: s/retrive/retrieve/

Done.

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/auth/arc_auth_code_fetcher.h (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/auth/arc_auth_code_fetcher.h:22: virtual void
Fetch(const FetchCallback& callback) = 0;
On 2016/12/03 01:27:54, Luis Héctor Chávez wrote:
> Maybe mention that it's not legal to call Fetch() more than once per instance.

Done.

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc (right):

https://codereview.chromium.org/2547073002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/auth/arc_manual_auth_code_fetcher.cc:66:
FetchInternal();
On 2016/12/03 14:24:26, Luis Héctor Chávez wrote:
> On 2016/12/03 01:27:54, Luis Héctor Chávez wrote:
> > Don't we want to transition to another page at this point such that even if
> the
> > user clicks "TRY AGAIN" not too quickly, it is possible to have several such
> > events in the queue and some of them being interleaved with the callback
from
> > OnContextPrepared? (thus working around the check in L64).
> 
> i thought about this a bit more: that would still be susceptible to races. how
> about you disable the button once it's pressed from js so that it's impossible
> for this scenario to happen? you can still transition to the "just one sec..."
> page here just to give feedback in case the context creation takes a bit
longer
> than expected.

SGTM.

Can I work on the UI change in another CL, because 1) the change would have
slightly different scope from this CL's main goal, 2) it is not regression by
this change IIUC, and 3) it conflicts with the M56 bug fix, unfortunately.

I removed the |preparing_context_| and related checks.

[commit-bot: I haz the power, 2016-12-05 06:51:16]

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

[commit-bot: I haz the power, 2016-12-05 06:51:16]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-12-05 16:19:05]

lgtm just change the commit summary to reflect that this does not fully fix the
race.

[hidehiko, 2016-12-05 18:04:02]

Description was changed from

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked() can be
invoked twice (or more) quickly. This problem will be addressed in another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

[hidehiko, 2016-12-05 18:04:17]

Description was changed from

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked() can be
invoked twice (or more) quickly. This problem will be addressed in another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked()
can be invoked twice (or more) quickly. This problem will be addressed in
another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

[hidehiko, 2016-12-05 18:06:11]

Updated commit message. Thank you for review! Submitting.

[hidehiko, 2016-12-05 18:06:18]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2016-12-05 18:06:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 18:11:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480961178283760,
"parent_rev": "6b413df5b0651370073c1e73ff1d34ad410dc61b", "commit_rev":
"386055b6e3d87c29e01acb8ed8ef8e16cedfc6f3"}

[commit-bot: I haz the power, 2016-12-05 18:13:02]

Description was changed from

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked()
can be invoked twice (or more) quickly. This problem will be addressed in
another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked()
can be invoked twice (or more) quickly. This problem will be addressed in
another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

[commit-bot: I haz the power, 2016-12-05 18:13:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-05 18:16:59]

Description was changed from

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked()
can be invoked twice (or more) quickly. This problem will be addressed in
another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Fix race issue in ArcAuthService.

ArcAuthService had an race issue, around manual "SIGN IN"
click flow.
This CL fixes it by moving everyting into ArcManualAuthCodeFetcher
implementation, and make it cancellable by deleting.

For that purpose, this CL introduces ArcAuthFetcher interface,
and has (old) ArcAuthCodeFetcher and ArcRobotAuth implement it, as well.
Along the change, these are renamed into ArcBackgroundAuthCodeFetcher
and ArcRobotAuthCodeFetchere respectively.

Note that, on error page, there still remains a race issue that
a user can click "TRY AGAIN" button quickly, so that OnRetryClicked()
can be invoked twice (or more) quickly. This problem will be addressed in
another CL.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/f44a3339cce8208c9b308fe95d99bc5faf7fbfc5
Cr-Commit-Position: refs/heads/master@{#436349}
==========

[commit-bot: I haz the power, 2016-12-05 18:17:00]

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