Make sure OneClickSigninSyncStarter detects Browser object being closed

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/sync/one_click_signin_sync_starter.h"
 
 #include "base/prefs/pref_service.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 
 #if defined(ENABLE_CONFIGURATION_POLICY)
 #include "chrome/browser/policy/cloud/user_policy_signin_service.h"
 #include "chrome/browser/policy/cloud/user_policy_signin_service_factory.h"
 #endif
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/browser/ui/webui/signin/profile_signin_confirmation_dialog.h"
 #include "chrome/common/url_constants.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
 OneClickSigninSyncStarter::OneClickSigninSyncStarter(
     Profile* profile,
     Browser* browser,
     const std::string& session_index,
     const std::string& email,
     const std::string& password,
     StartSyncMode start_mode,
     bool force_same_tab_navigation,
     ConfirmationRequired confirmation_required)
     : start_mode_(start_mode),
       force_same_tab_navigation_(force_same_tab_navigation),
       confirmation_required_(confirmation_required),
       weak_pointer_factory_(this) {
   DCHECK(profile);
+  BrowserList::AddObserver(this);
+
   Initialize(profile, browser);
 
   // Start the signin process using the cookies in the cookie jar.
   SigninManager* manager = SigninManagerFactory::GetForProfile(profile_);
   SigninManager::OAuthTokenFetchedCallback callback;
   // Policy is enabled, so pass in a callback to do extra policy-related UI
   // before signin completes.
   callback = base::Bind(&OneClickSigninSyncStarter::ConfirmSignin,
                         weak_pointer_factory_.GetWeakPtr());
   manager->StartSignInWithCredentials(session_index, email, password, callback);
 }
 
+void OneClickSigninSyncStarter::OnBrowserRemoved(Browser* browser) {
+  if (browser == browser_)
+    browser_ = NULL;
+}
+
 OneClickSigninSyncStarter::~OneClickSigninSyncStarter() {
+  BrowserList::RemoveObserver(this);
 }
 
 void OneClickSigninSyncStarter::Initialize(Profile* profile, Browser* browser) {
   DCHECK(profile);
   profile_ = profile;
   browser_ = browser;
 
   // Cache the parent desktop for the browser, so we can reuse that same
   // desktop for any UI we want to display.
   if (browser)
@@ skipping 178 matching lines @@
       chrome::startup::IS_PROCESS_STARTUP,
       chrome::startup::IS_FIRST_RUN,
       desktop_type,
       false);
   }
 }
 #endif
 
 void OneClickSigninSyncStarter::ConfirmAndSignin() {
   SigninManager* signin = SigninManagerFactory::GetForProfile(profile_);
-  // browser_ can be null for unit tests.
-  if (browser_ && confirmation_required_ == CONFIRM_UNTRUSTED_SIGNIN) {
+  if (confirmation_required_ == CONFIRM_UNTRUSTED_SIGNIN) {
+    EnsureBrowser();
     // Display a confirmation dialog to the user.
     browser_->window()->ShowOneClickSigninBubble(
         BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_SAML_MODAL_DIALOG,
         UTF8ToUTF16(signin->GetUsernameForAuthInProgress()),
         string16(), // No error message to display.
         base::Bind(&OneClickSigninSyncStarter::UntrustedSigninConfirmed,
                    weak_pointer_factory_.GetWeakPtr()));
   } else {
     // No confirmation required - just sign in the user.
     signin->CompletePendingSignin();
@@ skipping 72 matching lines @@
   browser_->window()->ShowOneClickSigninBubble(
       BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE,
       string16(),  // No email required - this is not a SAML confirmation.
       custom_message,
       // Callback is ignored.
       BrowserWindow::StartSyncCallback());
 }
 
 void OneClickSigninSyncStarter::EnsureBrowser() {
   if (!browser_) {
-    // The user just created a new profile so we need to figure out what
-    // browser to use to display settings. Grab the most recently active
-    // browser or else create a new one.
+    // The user just created a new profile or has closed the browser that
+    // we used previously. Grab the most recently active browser or else
+    // create a new one.
     browser_ = chrome::FindLastActiveWithProfile(profile_, desktop_type_);
     if (!browser_) {
       browser_ = new Browser(Browser::CreateParams(profile_,
                                                    desktop_type_));
+      chrome::AddBlankTabAt(browser_, -1, true);

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

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]

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:04]

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.

     }
     browser_->window()->Show();
   }
 }
 
 void OneClickSigninSyncStarter::ConfigureSync() {
   // Give the user a chance to configure things. We don't clear the
   // ProfileSyncService::setup_in_progress flag because we don't want sync
   // to start up until after the configure UI is displayed (the configure UI
   // will clear the flag when the user is done setting up sync).
@@ skipping 34 matching lines @@
 
 void OneClickSigninSyncStarter::ShowSyncSettingsPageOnSameTab() {
   std::string url = std::string(chrome::kChromeUISettingsURL) +
       chrome::kSyncSetupSubPage;
   chrome::NavigateParams params(
       browser_, GURL(url), content::PAGE_TRANSITION_AUTO_TOPLEVEL);
   params.disposition = CURRENT_TAB;
   params.window_action = chrome::NavigateParams::SHOW_WINDOW;
   chrome::Navigate(&params);
 }