Attempt 2 at changing session restores handling of incognito

windows. Previously when closing the last non-incognito window while
an incognito window was open resulted in clearing out the current
session. Now the session is not cleared out.

BUG=31540
TEST=see bug, also covered by tests
R=brettw@chromium.org,kuan@chromium.org

Review URL: http://codereview.chromium.org/6693084

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@80691 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 125 insertions, 78 deletions

diff --git a/chrome/browser/chromeos/tab_closeable_state_watcher.cc b/chrome/browser/chromeos/tab_closeable_state_watcher.cc
index 6ba5ba8..07254a7 100644
--- a/chrome/browser/chromeos/tab_closeable_state_watcher.cc
+++ b/chrome/browser/chromeos/tab_closeable_state_watcher.cc
@@ -67,7 +67,8 @@ TabCloseableStateWatcher::TabCloseableStateWatcher()
       signing_off_(false),
       guest_session_(
           CommandLine::ForCurrentProcess()->HasSwitch(
-              switches::kGuestSession)) {
+              switches::kGuestSession)),
+      waiting_for_browser_(false) {
   BrowserList::AddObserver(this);
   notification_registrar_.Add(this, NotificationType::APP_EXITING,
       NotificationService::AllSources());
@@ -80,7 +81,8 @@ TabCloseableStateWatcher::~TabCloseableStateWatcher() {
 }
 
 bool TabCloseableStateWatcher::CanCloseTab(const Browser* browser) const {
-  return browser->type() != Browser::TYPE_NORMAL ? true : can_close_tab_;
+  return browser->type() != Browser::TYPE_NORMAL ? true :
+      (can_close_tab_ || waiting_for_browser_);
 }
 
 bool TabCloseableStateWatcher::CanCloseBrowser(Browser* browser) {
@@ -109,6 +111,8 @@ void TabCloseableStateWatcher::OnWindowCloseCanceled(Browser* browser) {
 // TabCloseableStateWatcher, BrowserList::Observer implementation:
 
 void TabCloseableStateWatcher::OnBrowserAdded(const Browser* browser) {
+  waiting_for_browser_ = false;
+
   // Only normal browsers may affect closeable state.
   if (browser->type() != Browser::TYPE_NORMAL)
     return;
@@ -156,6 +160,9 @@ void TabCloseableStateWatcher::Observe(NotificationType type,
 
 void TabCloseableStateWatcher::OnTabStripChanged(const Browser* browser,
     bool closing_last_tab) {
+  if (waiting_for_browser_)
+    return;
+
   if (!closing_last_tab) {
     CheckAndUpdateState(browser);
     return;
@@ -175,9 +182,7 @@ void TabCloseableStateWatcher::OnTabStripChanged(const Browser* browser,
 
 void TabCloseableStateWatcher::CheckAndUpdateState(
     const Browser* browser_to_check) {
-  // We shouldn't update state if we're signing off, or there's no normal
-  // browser, or browser is always closeable.
-  if (signing_off_ || tabstrip_watchers_.empty() ||
+  if (waiting_for_browser_ || signing_off_ || tabstrip_watchers_.empty() ||
       (browser_to_check && browser_to_check->type() != Browser::TYPE_NORMAL))
     return;
 
@@ -218,10 +223,15 @@ void TabCloseableStateWatcher::SetCloseableState(bool closeable) {
       Details<bool>(&can_close_tab_));
 }
 
-bool TabCloseableStateWatcher::CanCloseBrowserImpl(const Browser* browser,
-    BrowserActionType* action_type) const {
+bool TabCloseableStateWatcher::CanCloseBrowserImpl(
+    const Browser* browser,
+    BrowserActionType* action_type) {
   *action_type = NONE;
 
+  // If we're waiting for a new browser allow the close.
+  if (waiting_for_browser_)
+    return true;
+
   // Browser is always closeable when signing off.
   if (signing_off_)
     return true;
@@ -234,10 +244,13 @@ bool TabCloseableStateWatcher::CanCloseBrowserImpl(const Browser* browser,
   if (tabstrip_watchers_.size() > 1)
     return true;
 
-  // If last normal browser is incognito, open a non-incognito window,
-  // and allow closing of incognito one (if not guest).
+  // If last normal browser is incognito, open a non-incognito window, and allow
+  // closing of incognito one (if not guest). When this happens we need to wait
+  // for the new browser before doing any other actions as the new browser may
+  // be created by way of session restore, which is async.
   if (browser->profile()->IsOffTheRecord() && !guest_session_) {
     *action_type = OPEN_WINDOW;
+    waiting_for_browser_ = true;
     return true;
   }
 
diff --git a/chrome/browser/chromeos/tab_closeable_state_watcher.h b/chrome/browser/chromeos/tab_closeable_state_watcher.h
index bf74077..38e8ae5 100644
--- a/chrome/browser/chromeos/tab_closeable_state_watcher.h
+++ b/chrome/browser/chromeos/tab_closeable_state_watcher.h
@@ -86,7 +86,7 @@ class TabCloseableStateWatcher : public ::TabCloseableStateWatcher,
   // Returns true if closing of |browser| is permitted.
   // |action_type| is the action to take regardless if browser is closeable.
   bool CanCloseBrowserImpl(const Browser* browser,
-                           BrowserActionType* action_type) const;
+                           BrowserActionType* action_type);
 
   // Data members.
 
@@ -101,6 +101,11 @@ class TabCloseableStateWatcher : public ::TabCloseableStateWatcher,
   // Is in guest session?
   bool guest_session_;
 
+  // Set to true if we're waiting for a new browser to be created. When true we
+  // uncoditionally allow everything as we know a browser is in the process of
+  // being created.
+  bool waiting_for_browser_;
+
   NotificationRegistrar notification_registrar_;
 
   // TabStripWatcher is a TabStripModelObserver that funnels all interesting
diff --git a/chrome/browser/chromeos/tab_closeable_state_watcher_browsertest.cc b/chrome/browser/chromeos/tab_closeable_state_watcher_browsertest.cc
index edee2e7..09243e5 100644
--- a/chrome/browser/chromeos/tab_closeable_state_watcher_browsertest.cc
+++ b/chrome/browser/chromeos/tab_closeable_state_watcher_browsertest.cc
@@ -248,6 +248,8 @@ IN_PROC_BROWSER_TEST_F(TabCloseableStateWatcherTest, SecondIncognitoBrowser) {
 // Tests closing an incognito browser - the incognito browser should close,
 // and a new normal browser opened with a NewTabPage (which is not closeable).
 IN_PROC_BROWSER_TEST_F(TabCloseableStateWatcherTest, CloseIncognitoBrowser) {
+  NavigateToURL(ntp_url_);
+
   // Open an incognito browser.
   Browser* incognito_browser = CreateIncognitoBrowser();
   EXPECT_TRUE(incognito_browser->profile()->IsOffTheRecord());
@@ -262,9 +264,8 @@ IN_PROC_BROWSER_TEST_F(TabCloseableStateWatcherTest, CloseIncognitoBrowser) {
 
   // Close incognito browser.
   incognito_browser->CloseWindow();
-  ui_test_utils::RunAllPendingInMessageLoop();
+  Browser* new_browser = ui_test_utils::WaitForNewBrowser();
   EXPECT_EQ(1u, BrowserList::size());
-  Browser* new_browser = *(BrowserList::begin());
   EXPECT_FALSE(new_browser->profile()->IsOffTheRecord());
   EXPECT_EQ(1, new_browser->tab_count());
   EXPECT_EQ(ntp_url_, new_browser->GetSelectedTabContents()->GetURL());
diff --git a/chrome/browser/sessions/session_restore_browsertest.cc b/chrome/browser/sessions/session_restore_browsertest.cc
index 6ff8980..45e1c09 100644
--- a/chrome/browser/sessions/session_restore_browsertest.cc
+++ b/chrome/browser/sessions/session_restore_browsertest.cc
@@ -1,8 +1,9 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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 "base/file_path.h"
+#include "chrome/browser/browser_list.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
@@ -14,6 +15,48 @@
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/common/page_transition_types.h"
 
+namespace {
+
+// BrowserList::Observer implementation that waits for a browser to be
+// removed.
+class BrowserListObserverImpl : public BrowserList::Observer {
+ public:
+  BrowserListObserverImpl() : did_remove_(false), running_(false) {
+    BrowserList::AddObserver(this);
+  }
+
+  ~BrowserListObserverImpl() {
+    BrowserList::RemoveObserver(this);
+  }
+
+  // Returns when a browser has been removed.
+  void Run() {
+    running_ = true;
+    if (!did_remove_)
+      ui_test_utils::RunMessageLoop();
+  }
+
+  // BrowserList::Observer
+  virtual void OnBrowserAdded(const Browser* browser) OVERRIDE {
+  }
+  virtual void OnBrowserRemoved(const Browser* browser) OVERRIDE {
+    did_remove_ = true;
+    if (running_)
+      MessageLoop::current()->Quit();
+  }
+
+ private:
+  // Was OnBrowserRemoved invoked?
+  bool did_remove_;
+
+  // Was Run invoked?
+  bool running_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserListObserverImpl);
+};
+
+}  // namespace
+
 typedef InProcessBrowserTest SessionRestoreTest;
 
 #if defined(OS_LINUX) && defined(TOOLKIT_VIEWS)
@@ -64,7 +107,6 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest,
   EXPECT_EQ(url, new_browser->GetTabContentsAt(0)->GetURL());
 }
 
-
 IN_PROC_BROWSER_TEST_F(SessionRestoreTest, RestoreIndividualTabFromWindow) {
   GURL url1(ui_test_utils::GetTestUrl(
       FilePath(FilePath::kCurrentDirectory),
@@ -142,3 +184,39 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest, WindowWithOneTab) {
   // Make sure the restore was successful.
   EXPECT_EQ(0U, service->entries().size());
 }
+
+// Verifies we remember the last browser window when closing the last
+// non-incognito window while an incognito window is open.
+IN_PROC_BROWSER_TEST_F(SessionRestoreTest, IncognitotoNonIncognito) {
+  // Turn on session restore.
+  SessionStartupPref pref(SessionStartupPref::LAST);
+  SessionStartupPref::SetStartupPref(browser()->profile(), pref);
+
+  GURL url(ui_test_utils::GetTestUrl(
+      FilePath(FilePath::kCurrentDirectory),
+      FilePath(FILE_PATH_LITERAL("title1.html"))));
+
+  // Add a single tab.
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Create a new incognito window.
+  Browser* incognito_browser = CreateIncognitoBrowser();
+  incognito_browser->AddBlankTab(true);
+  incognito_browser->window()->Show();
+
+  // Close the normal browser. After this we only have the incognito window
+  // open. We wait until the window closes as window closing is async.
+  {
+    BrowserListObserverImpl observer;
+    browser()->window()->Close();
+    observer.Run();
+  }
+
+  // Create a new window, which should trigger session restore.
+  incognito_browser->NewWindow();
+
+  // The first tab should have 'url' as its url.
+  Browser* new_browser = ui_test_utils::WaitForNewBrowser();
+  ASSERT_TRUE(new_browser);
+  EXPECT_EQ(url, new_browser->GetTabContentsAt(0)->GetURL());
+}
diff --git a/chrome/browser/sessions/session_restore_uitest.cc b/chrome/browser/sessions/session_restore_uitest.cc
index 1262e75..9e6e609b 100644
--- a/chrome/browser/sessions/session_restore_uitest.cc
+++ b/chrome/browser/sessions/session_restore_uitest.cc
@@ -335,55 +335,9 @@ TEST_F(SessionRestoreUITest, NormalAndPopup) {
 }
 
 #if !defined(OS_MACOSX)
-// These tests don't apply to the Mac version; see
+// This test doesn't apply to the Mac version; see
 // LaunchAnotherBrowserBlockUntilClosed for details.
 
-// Creates a browser, goes incognito, closes browser, launches and make sure
-// we don't restore.
-//
-TEST_F(SessionRestoreUITest, DontRestoreWhileIncognito) {
-  NavigateToURL(url1_);
-
-  // Make sure we have one window.
-  int initial_window_count;
-  ASSERT_TRUE(automation()->GetBrowserWindowCount(&initial_window_count));
-  ASSERT_EQ(1, initial_window_count);
-
-  scoped_refptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
-  ASSERT_TRUE(browser_proxy.get());
-
-  // Create an incognito window.
-  ASSERT_TRUE(browser_proxy->RunCommand(IDC_NEW_INCOGNITO_WINDOW));
-  int window_count;
-  ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
-  ASSERT_EQ(2, window_count);
-
-  // Close the first window.
-  CloseWindow(0, 2);
-  browser_proxy = NULL;
-
-  // Launch the browser again. Note, this doesn't spawn a new process, instead
-  // it attaches to the current process.
-  include_testing_id_ = false;
-  clear_profile_ = false;
-  launch_arguments_.AppendSwitch(switches::kRestoreLastSession);
-  LaunchAnotherBrowserBlockUntilClosed(launch_arguments_);
-
-  // A new window should appear;
-  ASSERT_TRUE(automation()->WaitForWindowCountToBecome(2));
-
-  // And it shouldn't have url1_ in it.
-  browser_proxy = automation()->GetBrowserWindow(1);
-  ASSERT_TRUE(browser_proxy.get());
-  scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetTab(0));
-  ASSERT_TRUE(tab_proxy.get());
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(
-      TestTimeouts::action_max_timeout_ms()));
-  GURL url;
-  ASSERT_TRUE(tab_proxy->GetCurrentURL(&url));
-  ASSERT_TRUE(url != url1_);
-}
-
 // Launches an app window, closes tabbed browser, launches and makes sure
 // we restore the tabbed browser url.
 // Flaky: http://crbug.com/29110
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index 1a57cdc8..073b2a3 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -1265,14 +1265,12 @@ bool SessionService::IsOnlyOneTabLeft() {
     return false;
   }
 
-  // NOTE: This uses the original profile so that closing the last non-off the
-  // record window while there are open incognito windows resets state).
   int window_count = 0;
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
     const SessionID::id_type window_id = (*i)->session_id().id();
     if (should_track_changes_for_browser_type((*i)->type()) &&
-        (*i)->profile()->GetOriginalProfile() == profile() &&
+        (*i)->profile() == profile() &&
         window_closing_ids_.find(window_id) == window_closing_ids_.end()) {
       if (++window_count > 1)
         return false;
@@ -1291,8 +1289,6 @@ bool SessionService::HasOpenTrackableBrowsers(const SessionID& window_id) {
     return true;
   }
 
-  // NOTE: This uses the original profile so that closing the last non-off the
-  // record window while there are open incognito window resets state).
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
     Browser* browser = *i;
@@ -1300,7 +1296,7 @@ bool SessionService::HasOpenTrackableBrowsers(const SessionID& window_id) {
     if (browser_id != window_id.id() &&
         window_closing_ids_.find(browser_id) == window_closing_ids_.end() &&
         should_track_changes_for_browser_type(browser->type()) &&
-        browser->profile()->GetOriginalProfile() == profile()) {
+        browser->profile() == profile()) {
       return true;
     }
   }
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h
index d7d4aeb..4d09e9b 100644
--- a/chrome/browser/sessions/session_service.h
+++ b/chrome/browser/sessions/session_service.h
@@ -33,12 +33,13 @@ struct SessionWindow;
 // and tabs so that they can be restored at a later date. The state of the
 // currently open browsers is referred to as the current session.
 //
-// SessionService supports restoring from the previous or last session. The
-// previous session typically corresponds to the last run of the browser, but
-// not always. For example, if the user has a tabbed browser and app window
-// running, closes the tabbed browser, then creates a new tabbed browser the
-// current session is made the last session and the current session reset. This
-// is done to provide the illusion that app windows run in separate processes.
+// SessionService supports restoring from the last session. The last session
+// typically corresponds to the last run of the browser, but not always. For
+// example, if the user has a tabbed browser and app window running, closes the
+// tabbed browser, then creates a new tabbed browser the current session is made
+// the last session and the current session reset. This is done to provide the
+// illusion that app windows run in separate processes. Similar behavior occurs
+// with incognito windows.
 //
 // SessionService itself maintains a set of SessionCommands that allow
 // SessionService to rebuild the open state of the browser (as
@@ -240,9 +241,8 @@ class SessionService : public BaseSessionService,
   SessionCommand* CreatePinnedStateCommand(const SessionID& tab_id,
                                            bool is_pinned);
 
-  // Callback from the backend for getting the commands from the previous
-  // or save file. Converts the commands in SessionWindows and notifies
-  // the real callback.
+  // Callback from the backend for getting the commands from the save file.
+  // Converts the commands in SessionWindows and notifies the real callback.
   void OnGotSessionCommands(
       Handle handle,
       scoped_refptr<InternalGetCommandsRequest> request);
@@ -440,8 +440,8 @@ class SessionService : public BaseSessionService,
 
   // If true and a new tabbed browser is created and there are no opened tabbed
   // browser (has_open_trackable_browsers_ is false), then the current session
-  // is made the previous session. See description above class for details on
-  // current/previous session.
+  // is made the last session. See description above class for details on
+  // current/last session.
   bool move_on_new_browser_;
 
   // Used for reporting frequency of session altering operations.