Fix: Prerendering was confusing SessionService to not save sessionStorage.

chrome/browser/sessions/session_service.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/sessions/session_service.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 #include <vector>
@@ skipping 202 matching lines @@
       has_open_trackable_browsers_(false),
       move_on_new_browser_(false),
       save_delay_in_millis_(base::TimeDelta::FromMilliseconds(2500)),
       save_delay_in_mins_(base::TimeDelta::FromMinutes(10)),
       save_delay_in_hrs_(base::TimeDelta::FromHours(8)),
       force_browser_not_alive_with_no_windows_(false)  {
   Init();
 }
 
 SessionService::~SessionService() {
+  for (BrowserList::const_iterator iter = BrowserList::begin();
+       iter != BrowserList::end(); ++iter) {
+    OnBrowserRemoved(*iter);
+  }
+  BrowserList::RemoveObserver(this);
   Save();
 }
 
 bool SessionService::RestoreIfNecessary(const std::vector<GURL>& urls_to_open) {
   return RestoreIfNecessary(urls_to_open, NULL);
 }
 
 void SessionService::ResetFromCurrentBrowsers() {
   ScheduleReset();
 }
@@ skipping 281 matching lines @@
                                      &last_updated_save_time_);
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_SESSION_SERVICE_SAVED,
         content::Source<Profile>(profile()),
         content::NotificationService::NoDetails());
   }
 }
 
 void SessionService::Init() {
   // Register for the notifications we're interested in.
-  registrar_.Add(this, chrome::NOTIFICATION_TAB_PARENTED,
-                 content::NotificationService::AllSources());
-  registrar_.Add(this, chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
-                 content::NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_NAV_LIST_PRUNED,
                  content::NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_CHANGED,
                  content::NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                  content::NotificationService::AllSources());
   // Wait for NOTIFICATION_BROWSER_WINDOW_READY so that is_app() is set.
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_WINDOW_READY,
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(
       this, chrome::NOTIFICATION_TAB_CONTENTS_APPLICATION_EXTENSION_CHANGED,
       content::NotificationService::AllSources());
+  BrowserList::AddObserver(this);
+  for (BrowserList::const_iterator iter = BrowserList::begin();
+       iter != BrowserList::end(); ++iter) {
+    OnBrowserAdded(*iter);
+  }
 }
 
 bool SessionService::ShouldNewWindowStartSession() {
   // ChromeOS and OSX have different ideas of application lifetime than
   // the other platforms.
   // On ChromeOS opening a new window should never start a new session.
 #if defined(OS_CHROMEOS)
   if (!force_browser_not_alive_with_no_windows_)
     return false;
 #endif
@@ skipping 45 matching lines @@
       if (browser->profile() != profile() ||
           !should_track_changes_for_browser_type(browser->type(), app_type))
         return;
 
       RestoreIfNecessary(std::vector<GURL>(), browser);
       SetWindowType(browser->session_id(), browser->type(), app_type);
       SetWindowAppName(browser->session_id(), browser->app_name());
       break;
     }
 
-    case chrome::NOTIFICATION_TAB_PARENTED: {
-      WebContents* web_contents = content::Source<WebContents>(source).ptr();
-      if (web_contents->GetBrowserContext() != profile())
-        return;
-      SessionTabHelper* session_tab_helper =
-          SessionTabHelper::FromWebContents(web_contents);
-      SetTabWindow(session_tab_helper->window_id(),
-                   session_tab_helper->session_id());
-      extensions::TabHelper* extensions_tab_helper =
-          extensions::TabHelper::FromWebContents(web_contents);
-      if (extensions_tab_helper &&
-          extensions_tab_helper->extension_app()) {
-        SetTabExtensionAppID(
-            session_tab_helper->window_id(),
-            session_tab_helper->session_id(),
-            extensions_tab_helper->extension_app()->id());
-      }
-
-      // Record the association between the SessionStorageNamespace and the
-      // tab.
-      //
-      // TODO(ajwong): This should be processing the whole map rather than
-      // just the default. This in particular will not work for tabs with only
-      // isolated apps which won't have a default partition.
-      content::SessionStorageNamespace* session_storage_namespace =
-          web_contents->GetController().GetDefaultSessionStorageNamespace();
-      ScheduleCommand(CreateSessionStorageAssociatedCommand(
-          session_tab_helper->session_id(),
-          session_storage_namespace->persistent_id()));
-      session_storage_namespace->SetShouldPersist(true);
-      break;
-    }
-
-    case chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED: {
-      TabContents* tab = content::Source<TabContents>(source).ptr();
-      if (!tab || tab->profile() != profile())
-        return;
-      // Allow the associated sessionStorage to get deleted; it won't be needed
-      // in the session restore.
-      content::SessionStorageNamespace* session_storage_namespace =
-          tab->web_contents()->GetController().
-              GetDefaultSessionStorageNamespace();
-      session_storage_namespace->SetShouldPersist(false);
-      SessionTabHelper* session_tab_helper =
-          SessionTabHelper::FromWebContents(tab->web_contents());
-      TabClosed(session_tab_helper->window_id(),
-                session_tab_helper->session_id(),
-                tab->web_contents()->GetClosedByUserGesture());
-      RecordSessionUpdateHistogramData(type, &last_updated_tab_closed_time_);
-      break;
-    }
-
     case content::NOTIFICATION_NAV_LIST_PRUNED: {
       WebContents* web_contents =
           content::Source<content::NavigationController>(source).ptr()->
               GetWebContents();
       SessionTabHelper* session_tab_helper =
           SessionTabHelper::FromWebContents(web_contents);
       if (!session_tab_helper || web_contents->GetBrowserContext() != profile())
         return;
       content::Details<content::PrunedDetails> pruned_details(details);
       if (pruned_details->from_front) {
@@ skipping 1089 matching lines @@
       50);
   if (use_long_period) {
     std::string long_name_("SessionRestore.SaveLongPeriod");
     UMA_HISTOGRAM_CUSTOM_TIMES(long_name_,
         delta,
         save_delay_in_mins_,
         save_delay_in_hrs_,
         50);
   }
 }
+
+void SessionService::OnBrowserAdded(Browser* browser) {

[sky, 2012/09/21 16:26:31]

Can we move the implementation of observing BrowserList and TabStripModel into a
separate class that invokes newly exposed methods on SessionService? That way
SessionService doesn't have as many dependencies.

[marja, 2012/09/24 09:23:09]

Ok, it seems that Browser already has this logic to monitor the tab creation,
and it was also calling SessionService funcs to notify it e.g., about window
closing, so I think notifying SessionService about tab creation / closing fits
in, too. (See the latest patch set.) Wdyt?

+  if (!profile() || !profile()->IsSameProfile(browser->profile()))
+    return;
+  browser->tab_strip_model()->AddObserver(this);
+}
+
+void SessionService::OnBrowserRemoved(Browser* browser) {
+  if (!profile() || !profile()->IsSameProfile(browser->profile()))
+    return;
+  browser->tab_strip_model()->RemoveObserver(this);
+}
+
+void SessionService::TabInsertedAt(TabContents* contents,
+                                   int index,
+                                   bool foreground) {
+  if (contents->profile() != profile())
+    return;
+  WebContents* web_contents = contents->web_contents();
+  SessionTabHelper* session_tab_helper =
+      SessionTabHelper::FromWebContents(web_contents);
+  SetTabWindow(session_tab_helper->window_id(),
+               session_tab_helper->session_id());
+  extensions::TabHelper* extensions_tab_helper =
+      extensions::TabHelper::FromWebContents(web_contents);
+  if (extensions_tab_helper &&
+      extensions_tab_helper->extension_app()) {
+    SetTabExtensionAppID(
+        session_tab_helper->window_id(),
+        session_tab_helper->session_id(),
+        extensions_tab_helper->extension_app()->id());
+  }
+
+  // Record the association between the SessionStorageNamespace and the
+  // tab.
+  //
+  // TODO(ajwong): This should be processing the whole map rather than
+  // just the default. This in particular will not work for tabs with only
+  // isolated apps which won't have a default partition.
+  content::SessionStorageNamespace* session_storage_namespace =
+      web_contents->GetController().GetDefaultSessionStorageNamespace();
+  ScheduleCommand(CreateSessionStorageAssociatedCommand(
+      session_tab_helper->session_id(),
+      session_storage_namespace->persistent_id()));
+  session_storage_namespace->SetShouldPersist(true);
+}
+
+void SessionService::TabClosingAt(TabStripModel* tab_strip_model,
+                                  TabContents* contents,
+                                  int index) {
+  if (!contents || contents->profile() != profile())
+    return;
+  WebContents* web_contents = contents->web_contents();
+  // Allow the associated sessionStorage to get deleted; it won't be needed
+  // in the session restore.
+  content::SessionStorageNamespace* session_storage_namespace =
+    web_contents->GetController().GetDefaultSessionStorageNamespace();
+  session_storage_namespace->SetShouldPersist(false);
+  SessionTabHelper* session_tab_helper =
+    SessionTabHelper::FromWebContents(web_contents);
+  TabClosed(session_tab_helper->window_id(),
+            session_tab_helper->session_id(),
+            web_contents->GetClosedByUserGesture());
+  RecordSessionUpdateHistogramData(chrome::NOTIFICATION_TAB_CONTENTS_DESTROYED,
+                                   &last_updated_tab_closed_time_);
+}
+
+void SessionService::TabReplacedAt(TabStripModel* tab_strip_model,
+                                   TabContents* old_contents,
+                                   TabContents* new_contents,
+                                   int index) {
+  TabClosingAt(tab_strip_model, old_contents, index);
+  TabInsertedAt(new_contents, index, true /* foreground is not used */);
+}