[Android] Fix sign-in crash fix.

[Android] Fix sign-in crash fix.

Sign-in button on sign-in promotion panel can be clicked multiple times,
which can lead to a crash. We should make sure that signing in logic only
happens once.

BUG=477139
Committed: https://crrev.com/44e22fa204b7250b7f261d7098361d8546b7ead8
Cr-Commit-Position: refs/heads/master@{#327200}

[Kibeom Kim (inactive), 2015-04-15 23:26:36]

kkimlabs@chromium.org changed reviewers:
+ newt@chromium.org

[Kibeom Kim (inactive), 2015-04-15 23:27:17]

note: first I thought disabling from UI but noticed that |finishSignIn| can be
called from multiple paths.

[newt (away), 2015-04-15 23:30:03]

newt@chromium.org changed reviewers:
+ maxbogue@chromium.org, nyquist@chromium.org

[newt (away), 2015-04-15 23:30:04]

Deferring to Tommy or Max

[maxbogue, 2015-04-16 16:16:17]

Would you mind writing a regression test for this? Seems like it should just be
as easy as telling SigninManager to sign in twice. Other than that it looks
good, since that class is all on the UI thread.

[nyquist, 2015-04-16 16:42:28]

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

[nyquist, 2015-04-16 16:42:29]

I agree with max about the regression test.

rogert: Do you know what we would do on the native side here? Does this seem
like a reasonable fix to do in finishSignIn?

[Roger Tawa OOO till Jul 10th, 2015-04-16 17:02:14]

On 2015/04/16 16:42:29, nyquist wrote:
> I agree with max about the regression test.
> 
> rogert: Do you know what we would do on the native side here? Does this seem
> like a reasonable fix to do in finishSignIn?

Hi Tommy,

Are other steps before finish that also should not run in this case?  What about
adding the check at line 283 above (startSignIn)?

[nyquist, 2015-04-16 17:09:07]

On 2015/04/16 17:02:14, Roger Tawa wrote:
> On 2015/04/16 16:42:29, nyquist wrote:
> > I agree with max about the regression test.
> > 
> > rogert: Do you know what we would do on the native side here? Does this seem
> > like a reasonable fix to do in finishSignIn?
> 
> Hi Tommy,
> 
> Are other steps before finish that also should not run in this case?  What
about
> adding the check at line 283 above (startSignIn)?

Exactly. I think that would make more sense. At least at a high level from
looking at the code. This code already bails out early before
ChromeSigninController.setSignedInAccountName(...), so I don't think there are
more things that need to happen.

Could there be an issue with doing it so early though that since parts of this
is async, that the fix would in fact not work, as the native state is not
updated yet?

[Kibeom Kim (inactive), 2015-04-16 17:22:13]

On 2015/04/16 17:09:07, nyquist wrote:
> On 2015/04/16 17:02:14, Roger Tawa wrote:
> > On 2015/04/16 16:42:29, nyquist wrote:
> > > I agree with max about the regression test.
> > > 
> > > rogert: Do you know what we would do on the native side here? Does this
seem
> > > like a reasonable fix to do in finishSignIn?
> > 
> > Hi Tommy,
> > 
> > Are other steps before finish that also should not run in this case?  What
> about
> > adding the check at line 283 above (startSignIn)?
> 
> Exactly. I think that would make more sense. At least at a high level from
> looking at the code. This code already bails out early before
> ChromeSigninController.setSignedInAccountName(...), so I don't think there are
> more things that need to happen.
> 

I'm not familiar with the signing in path so unsure if this is an issue, but
also there are

onPolicyCheckedBeforeSignin(...)
onPolicyFetchedBeforeSignin(...)

that eventually calls finishSignin(...), without going through startSignin(...).

> Could there be an issue with doing it so early though that since parts of this
> is async, that the fix would in fact not work, as the native state is not
> updated yet?

Seems so to me...

[Roger Tawa OOO till Jul 10th, 2015-04-16 17:40:25]

On 2015/04/16 17:22:13, Kibeom Kim wrote:
> On 2015/04/16 17:09:07, nyquist wrote:
> > On 2015/04/16 17:02:14, Roger Tawa wrote:
> > > On 2015/04/16 16:42:29, nyquist wrote:
> > > > I agree with max about the regression test.
> > > > 
> > > > rogert: Do you know what we would do on the native side here? Does this
> seem
> > > > like a reasonable fix to do in finishSignIn?
> > > 
> > > Hi Tommy,
> > > 
> > > Are other steps before finish that also should not run in this case?  What
> > about
> > > adding the check at line 283 above (startSignIn)?
> > 
> > Exactly. I think that would make more sense. At least at a high level from
> > looking at the code. This code already bails out early before
> > ChromeSigninController.setSignedInAccountName(...), so I don't think there
are
> > more things that need to happen.
> > 
> 
> I'm not familiar with the signing in path so unsure if this is an issue, but
> also there are
> 
> onPolicyCheckedBeforeSignin(...)
> onPolicyFetchedBeforeSignin(...)
> 
> that eventually calls finishSignin(...), without going through
startSignin(...).
> 
> > Could there be an issue with doing it so early though that since parts of
this
> > is async, that the fix would in fact not work, as the native state is not
> > updated yet?
> 
> Seems so to me...

Those onPolicy functions are private.  Yhey are (eventually) called from
nativeCheckPolicyBeforeSignIn(), which comes from startSignIn().  So I think a
check in startSignIn() covers all bases.

[nyquist, 2015-04-23 00:27:38]

kkimlabs: Any plans for updating this CL?

[Kibeom Kim (inactive), 2015-04-23 00:33:20]

On 2015/04/23 00:27:38, nyquist wrote:
> kkimlabs: Any plans for updating this CL?

just uploaded the latest work. I didn't attempt writing a regression test yet,
will do.

[Kibeom Kim (inactive), 2015-04-23 00:36:43]

On 2015/04/23 00:33:20, Kibeom Kim wrote:
> On 2015/04/23 00:27:38, nyquist wrote:
> > kkimlabs: Any plans for updating this CL?
> 
> just uploaded the latest work. I didn't attempt writing a regression test yet,
> will do.

actually, the last cl seems wrong please ignore :/...

[Roger Tawa OOO till Jul 10th, 2015-04-27 15:21:52]

On 2015/04/23 00:36:43, Kibeom Kim wrote:
> On 2015/04/23 00:33:20, Kibeom Kim wrote:
> > On 2015/04/23 00:27:38, nyquist wrote:
> > > kkimlabs: Any plans for updating this CL?
> > 
> > just uploaded the latest work. I didn't attempt writing a regression test
yet,
> > will do.
> 
> actually, the last cl seems wrong please ignore :/...

Is this related to https://codereview.chromium.org/1073943003/ ?

[Kibeom Kim (inactive), 2015-04-27 17:13:59]

On 2015/04/27 15:21:52, Roger Tawa wrote:
> Is this related to https://codereview.chromium.org/1073943003/ ?

perhaps link copy-paste mistake?

[Roger Tawa OOO till Jul 10th, 2015-04-27 17:23:46]

On 2015/04/27 17:13:59, Kibeom Kim wrote:
> On 2015/04/27 15:21:52, Roger Tawa wrote:
> > Is this related to https://codereview.chromium.org/1073943003/ ?
> 
> perhaps link copy-paste mistake?

Doh, very sorry.  Meant to say: https://codereview.chromium.org/1100983005/

[nyquist, 2015-04-27 23:45:58]

I think it's safe to submit both of these
(https://codereview.chromium.org/1100983005/), so lgtm

[Kibeom Kim (inactive), 2015-04-27 23:49:02]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-04-27 23:51:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073943003/40001

[commit-bot: I haz the power, 2015-04-28 00:51:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-28 00:53:19]

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