Fix a crash in SigninManager.finishSignIn()

Fix a crash in SigninManager.finishSignIn()

No test because SigninManager is low on my list of things to write tests
for. I've filed http://crbug.com/481099 to track it.

BUG=478597
Committed: https://crrev.com/fec395b4f42a58b59bf6e695a526ad82cc465681
Cr-Commit-Position: refs/heads/master@{#327182}

[maxbogue, 2015-04-24 18:02:27]

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

[maxbogue, 2015-04-24 18:02:28]

Tommy, please review this quick NPE fix.

[nyquist, 2015-04-24 18:22:45]

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

[nyquist, 2015-04-24 18:22:45]

rogerta: Any thoughts on this?

https://codereview.chromium.org/1100983005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/1100983005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:398:
// If signin was canceled, abort.
How does one cancel a signin? I'm not sure I understand how this can happen
(even in a monkey test). Does it mean that two sign-ins where triggered somehow,
and when one of them succeeded, they set mSignInAccount to null?

[maxbogue, 2015-04-24 21:35:34]

https://codereview.chromium.org/1100983005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/1100983005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:398:
// If signin was canceled, abort.
On 2015/04/24 18:22:45, nyquist wrote:
> How does one cancel a signin? I'm not sure I understand how this can happen
> (even in a monkey test). Does it mean that two sign-ins where triggered
somehow,
> and when one of them succeeded, they set mSignInAccount to null?

I was thinking something called the cancelSignIn() method on this class, but yes
the double-call seems just as if not more likely. In either case, this check
should fix the problem.

[nyquist, 2015-04-24 22:23:13]

lgtm, but please hold off for rogerta@'s thoughts in case there is some extra
cleanup that need to happen in that case.

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

lgtm since it will fix a crash, but I'm sure exactly what the cause could be.

[nyquist, 2015-04-27 17:26:22]

maxbogue: Could you look at https://codereview.chromium.org/1073943003/ and talk
to kkimlabs@ before landing this? The issues seem similar to me.

[Kibeom Kim (inactive), 2015-04-27 17:44:39]

On 2015/04/27 17:26:22, nyquist wrote:
> maxbogue: Could you look at https://codereview.chromium.org/1073943003/ and
talk
> to kkimlabs@ before landing this? The issues seem similar to me.

Yes it's the same, and he and my fix is the same, which is good I guess. :).
Please ignore my comment #15 at https://codereview.chromium.org/1073943003/ I
looked again just now, and I think I was just mistaken.

[maxbogue, 2015-04-27 19:52:44]

On 2015/04/27 17:44:39, Kibeom Kim wrote:
> On 2015/04/27 17:26:22, nyquist wrote:
> > maxbogue: Could you look at https://codereview.chromium.org/1073943003/ and
> talk
> > to kkimlabs@ before landing this? The issues seem similar to me.
> 
> Yes it's the same, and he and my fix is the same, which is good I guess. :).
> Please ignore my comment #15 at https://codereview.chromium.org/1073943003/ I
> looked again just now, and I think I was just mistaken.

I think both fixes are a good idea. I'm not convinced it's impossible for
cancelSignIn() to be called between doSignIn() and finishSignIn(), which
is a case that the other fix would not address.

[Kibeom Kim (inactive), 2015-04-27 23:01:36]

On 2015/04/27 19:52:44, maxbogue wrote:
> On 2015/04/27 17:44:39, Kibeom Kim wrote:
> > On 2015/04/27 17:26:22, nyquist wrote:
> > > maxbogue: Could you look at https://codereview.chromium.org/1073943003/
and
> > talk
> > > to kkimlabs@ before landing this? The issues seem similar to me.
> > 
> > Yes it's the same, and he and my fix is the same, which is good I guess. :).
> > Please ignore my comment #15 at https://codereview.chromium.org/1073943003/
I
> > looked again just now, and I think I was just mistaken.
> 
> I think both fixes are a good idea. I'm not convinced it's impossible for
> cancelSignIn() to be called between doSignIn() and finishSignIn(), which
> is a case that the other fix would not address.

Yeah you're right. lgtm .

[maxbogue, 2015-04-27 23:35:07]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-04-27 23:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1100983005/#ps20001
(title: "Add log warning.")

[commit-bot: I haz the power, 2015-04-27 23:35:32]

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

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

Committed patchset #2 (id:20001)

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

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