Make sure OneClickSigninSyncStarter detects Browser object being closed

Make sure OneClickSigninSyncStarter detects Browser object being closed

Before this fix, OneClickSigninSyncStarter didn't detect when the Browser object it points to was closed. It could then cause a crash by making calls on the destroyed Browser object. OneClickSigninSyncStarter now observes the browser list to detect when the Browser it points to is closed.

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

[fdoray, 2013-05-30 16:40:46]

https://codereview.chromium.org/16092009/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/16092009/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:254: if
(confirmation_required_ == CONFIRM_UNTRUSTED_SIGNIN) {
The only unit test that involves this method is
OneClickSigninHelperTest.SigninFromWebstoreWithConfigSyncfirst. In this unit
test, confirmation_required_ == NO_CONFIRMATION, so I don't think we need need
the special case that we had before.

[Roger Tawa OOO till Jul 10th, 2013-05-30 18:14:49]

lgtm

https://codereview.chromium.org/16092009/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/16092009/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:254: if
(confirmation_required_ == CONFIRM_UNTRUSTED_SIGNIN) {
On 2013/05/30 16:40:46, fdoray wrote:
> The only unit test that involves this method is
> OneClickSigninHelperTest.SigninFromWebstoreWithConfigSyncfirst. In this unit
> test, confirmation_required_ == NO_CONFIRMATION, so I don't think we need need
> the special case that we had before.

OK.  Sounds like the OneClickSigninHelper unit tests should actually be browser
tests.  There is a bug to implement browser tests for that code
(crbug.com/142487), so this should be fine.

https://codereview.chromium.org/16092009/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/16092009/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:70: virtual void
OnBrowserRemoved(Browser* browser);
Add OVERRIDE macro.

[noms (inactive), 2013-05-31 17:26:13]

lgtm

[bcwhite, 2013-06-03 14:06:23]

lgtm

https://codereview.chromium.org/16092009/diff/6001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/16092009/diff/6001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:69: //
chrome::BrowserListObserver override.
Just

// BrowserListObserver:

[commit-bot: I haz the power, 2013-06-03 18:08:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/16092009/12001

[commit-bot: I haz the power, 2013-06-04 09:36:04]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-04 13:44:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/16092009/30001

[commit-bot: I haz the power, 2013-06-04 20:02:33]

Change committed as 204021

[Andrew T Wilson (Slow), 2013-06-05 08:43:51]

https://codereview.chromium.org/16092009/diff/30001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/16092009/diff/30001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:377:
chrome::AddBlankTabAt(browser_, -1, true);
Why is this necessary? This code was working properly before in the case where
we create a new profile.

[(NOT FOR CODE REVIEWS), 2013-06-05 14:11:34]

https://codereview.chromium.org/16092009/diff/30001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/16092009/diff/30001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:377:
chrome::AddBlankTabAt(browser_, -1, true);
On 2013/06/05 08:43:51, Andrew T Wilson wrote:
> Why is this necessary? This code was working properly before in the case where
> we create a new profile.

Maybe something changed since the time you looked at this?  Francois showed me
that without this line, he would see a new window with only the "New tab" button
shown, no actual tab.

[fdoray, 2013-06-05 15:09:03]

https://codereview.chromium.org/16092009/diff/30001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/16092009/diff/30001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:377:
chrome::AddBlankTabAt(browser_, -1, true);
On 2013/06/05 14:11:34, Roger Tawa (Google) wrote:
> On 2013/06/05 08:43:51, Andrew T Wilson wrote:
> > Why is this necessary? This code was working properly before in the case
where
> > we create a new profile.
> 
> Maybe something changed since the time you looked at this?  Francois showed me
> that without this line, he would see a new window with only the "New tab"
button
> shown, no actual tab.

When you create a new profile as part of the sign in process, a new browser with
a new tab page is created before EnsureBrowser is called.
chrome::FindLastActiveWithProfile returns this browser and the line I added is
not executed. That's why the code seemed to work before. As Roger said, if I
remove the call to chrome::AddBlankTabAt, you get a window with no tab when the
"if" block is actually executed.