Make TabRestoreService::Entry noncopyable and fix up surrounding code.

components/sessions/core/tab_restore_service_helper.cc

Index: components/sessions/core/tab_restore_service_helper.cc
diff --git a/components/sessions/core/tab_restore_service_helper.cc b/components/sessions/core/tab_restore_service_helper.cc
index 73a4c72f6530d3dd4003e016178feb103c6fffc6..cc3f2ae80af6260108bdefebf3fcb4a74d5119bc 100644
--- a/components/sessions/core/tab_restore_service_helper.cc
+++ b/components/sessions/core/tab_restore_service_helper.cc
@@ -10,6 +10,7 @@
 #include <iterator>
 
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "components/sessions/core/live_tab.h"
@@ -52,7 +53,6 @@ TabRestoreServiceHelper::TabRestoreServiceHelper(
 TabRestoreServiceHelper::~TabRestoreServiceHelper() {
   FOR_EACH_OBSERVER(TabRestoreServiceObserver, observer_list_,
                     TabRestoreServiceDestroyed(tab_restore_service_));
-  STLDeleteElements(&entries_);
 }
 
 void TabRestoreServiceHelper::AddObserver(
@@ -74,48 +74,39 @@ void TabRestoreServiceHelper::CreateHistoricalTab(LiveTab* live_tab,
   if (closing_contexts_.find(context) != closing_contexts_.end())
     return;
 
-  std::unique_ptr<Tab> local_tab(new Tab());
+  auto local_tab = base::MakeUnique<Tab>();
   PopulateTab(local_tab.get(), index, context, live_tab);
   if (local_tab->navigations.empty())
     return;
 
-  AddEntry(local_tab.release(), true, true);
+  AddEntry(std::move(local_tab), true, true);
 }
 
 void TabRestoreServiceHelper::BrowserClosing(LiveTabContext* context) {
   closing_contexts_.insert(context);
 
-  std::unique_ptr<Window> window(new Window());
+  auto window = base::MakeUnique<Window>();
   window->selected_tab_index = context->GetSelectedIndex();
   window->timestamp = TimeNow();
   window->app_name = context->GetAppName();
 
-  // Don't use std::vector::resize() because it will push copies of an empty tab
-  // into the vector, which will give all tabs in a window the same ID.
-  for (int i = 0; i < context->GetTabCount(); ++i) {
-    window->tabs.push_back(Tab());
-  }
-  size_t entry_index = 0;
   for (int tab_index = 0; tab_index < context->GetTabCount(); ++tab_index) {
-    PopulateTab(&(window->tabs[entry_index]), tab_index, context,
+    auto tab = base::MakeUnique<Tab>();
+    PopulateTab(tab.get(), tab_index, context,
                 context->GetLiveTabAt(tab_index));
-    if (window->tabs[entry_index].navigations.empty()) {
-      window->tabs.erase(window->tabs.begin() + entry_index);
-    } else {
-      window->tabs[entry_index].browser_id = context->GetSessionID().id();
-      entry_index++;
+    if (!tab->navigations.empty()) {
+      tab->browser_id = context->GetSessionID().id();
+      window->tabs.push_back(std::move(tab));
     }
   }
   if (window->tabs.size() == 1 && window->app_name.empty()) {
     // Short-circuit creating a Window if only 1 tab was present. This fixes
-    // http://crbug.com/56744. Copy the Tab because it's owned by an object on
-    // the stack.
-    AddEntry(new Tab(window->tabs[0]), true, true);
+    // http://crbug.com/56744.
+    AddEntry(std::move(window->tabs[0]), true, true);
   } else if (!window->tabs.empty()) {
-    window->selected_tab_index =
-        std::min(static_cast<int>(window->tabs.size() - 1),
-                 window->selected_tab_index);
-    AddEntry(window.release(), true, true);
+    window->selected_tab_index = std::min(
+        static_cast<int>(window->tabs.size() - 1), window->selected_tab_index);
+    AddEntry(std::move(window), true, true);
   }
 }
 
@@ -126,7 +117,7 @@ void TabRestoreServiceHelper::BrowserClosed(LiveTabContext* context) {
 void TabRestoreServiceHelper::ClearEntries() {
   if (observer_)
     observer_->OnClearEntries();
-  STLDeleteElements(&entries_);
+  entries_.clear();
   NotifyTabsChanged();
 }
 
@@ -141,17 +132,16 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreMostRecentEntry(
   return RestoreEntryById(context, entries_.front()->id, UNKNOWN);
 }
 
-TabRestoreService::Tab* TabRestoreServiceHelper::RemoveTabEntryById(
-    SessionID::id_type id) {
-  Entries::iterator it = GetEntryIteratorById(id);
+std::unique_ptr<TabRestoreService::Tab>
+TabRestoreServiceHelper::RemoveTabEntryById(SessionID::id_type id) {
+  auto it = GetEntryIteratorById(id);
   if (it == entries_.end())
     return nullptr;
 
-  Entry* entry = *it;
-  if (entry->type != TabRestoreService::TAB)
+  if ((*it)->type != TabRestoreService::TAB)
     return nullptr;
 
-  Tab* tab = static_cast<Tab*>(entry);
+  auto tab = std::unique_ptr<Tab>(static_cast<Tab*>(it->release()));
   entries_.erase(it);
   return tab;
 }
@@ -169,97 +159,99 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
   if (observer_)
     observer_->OnRestoreEntryById(id, entry_iterator);
   restoring_ = true;
-  Entry* entry = *entry_iterator;
+  auto& entry = **entry_iterator;
 
   // If the entry's ID does not match the ID that is being restored, then the
   // entry is a window from which a single tab will be restored.
-  bool restoring_tab_in_window = entry->id != id;
-
-  if (!restoring_tab_in_window) {
-    entries_.erase(entry_iterator);
-    entry_iterator = entries_.end();
-  }
+  bool restoring_tab_in_window = entry.id != id;
 
   // |context| will be NULL in cases where one isn't already available (eg,
   // when invoked on Mac OS X with no windows open). In this case, create a
   // new browser into which we restore the tabs.
   std::vector<LiveTab*> live_tabs;
-  if (entry->type == TabRestoreService::TAB) {
-    Tab* tab = static_cast<Tab*>(entry);
-    LiveTab* restored_tab = nullptr;
-    context = RestoreTab(*tab, context, disposition, &restored_tab);
-    live_tabs.push_back(restored_tab);
-    context->ShowBrowserWindow();
-  } else if (entry->type == TabRestoreService::WINDOW) {
-    LiveTabContext* current_context = context;
-    Window* window = static_cast<Window*>(entry);
-
-    // When restoring a window, either the entire window can be restored, or a
-    // single tab within it. If the entry's ID matches the one to restore, then
-    // the entire window will be restored.
-    if (!restoring_tab_in_window) {
-      context = client_->CreateLiveTabContext(window->app_name);
-      for (size_t tab_i = 0; tab_i < window->tabs.size(); ++tab_i) {
-        const Tab& tab = window->tabs[tab_i];
-        LiveTab* restored_tab = context->AddRestoredTab(
-            tab.navigations, context->GetTabCount(),
-            tab.current_navigation_index, tab.extension_app_id,
-            static_cast<int>(tab_i) == window->selected_tab_index, tab.pinned,
-            tab.from_last_session, tab.platform_data.get(),
-            tab.user_agent_override);
-        if (restored_tab) {
-          restored_tab->LoadIfNecessary();
-          client_->OnTabRestored(
-              tab.navigations.at(tab.current_navigation_index).virtual_url());
-          live_tabs.push_back(restored_tab);
+  switch (entry.type) {
+    case TabRestoreService::TAB: {
+      auto& tab = static_cast<const Tab&>(entry);
+      LiveTab* restored_tab = nullptr;
+      context = RestoreTab(tab, context, disposition, &restored_tab);
+      live_tabs.push_back(restored_tab);
+      context->ShowBrowserWindow();
+      break;
+    }
+    case TabRestoreService::WINDOW: {
+      LiveTabContext* current_context = context;
+      auto& window = static_cast<Window&>(entry);
+
+      // When restoring a window, either the entire window can be restored, or a
+      // single tab within it. If the entry's ID matches the one to restore,
+      // then the entire window will be restored.
+      if (!restoring_tab_in_window) {
+        context = client_->CreateLiveTabContext(window.app_name);
+        for (size_t tab_i = 0; tab_i < window.tabs.size(); ++tab_i) {
+          const Tab& tab = *window.tabs[tab_i];
+          LiveTab* restored_tab = context->AddRestoredTab(
+              tab.navigations, context->GetTabCount(),
+              tab.current_navigation_index, tab.extension_app_id,
+              static_cast<int>(tab_i) == window.selected_tab_index, tab.pinned,
+              tab.from_last_session, tab.platform_data.get(),
+              tab.user_agent_override);
+          if (restored_tab) {
+            restored_tab->LoadIfNecessary();
+            client_->OnTabRestored(
+                tab.navigations.at(tab.current_navigation_index).virtual_url());
+            live_tabs.push_back(restored_tab);
+          }
         }
-      }
-      // All the window's tabs had the same former browser_id.
-      if (window->tabs[0].has_browser()) {
-        UpdateTabBrowserIDs(window->tabs[0].browser_id,
-                            context->GetSessionID().id());
-      }
-    } else {
-      // Restore a single tab from the window. Find the tab that matches the ID
-      // in the window and restore it.
-      for (std::vector<Tab>::iterator tab_i = window->tabs.begin();
-           tab_i != window->tabs.end(); ++tab_i) {
-        const Tab& tab = *tab_i;
-        if (tab.id != id)
-          continue;
-
-        LiveTab* restored_tab = nullptr;
-        context = RestoreTab(tab, context, disposition, &restored_tab);
-        live_tabs.push_back(restored_tab);
-        window->tabs.erase(tab_i);
-        // If restoring the tab leaves the window with nothing else, delete it
-        // as well.
-        if (window->tabs.empty()) {
-          entries_.erase(entry_iterator);
-          delete entry;
-        } else {
-          // Update the browser ID of the rest of the tabs in the window so if
-          // any one is restored, it goes into the same window as the tab
-          // being restored now.
-          UpdateTabBrowserIDs(tab.browser_id, context->GetSessionID().id());
-          for (Tab& tab_j : window->tabs)
-            tab_j.browser_id = context->GetSessionID().id();
+        // All the window's tabs had the same former browser_id.
+        if (auto browser_id = window.tabs[0]->browser_id) {
+          UpdateTabBrowserIDs(browser_id, context->GetSessionID().id());
+        }
+      } else {
+        // Restore a single tab from the window. Find the tab that matches the
+        // ID
+        // in the window and restore it.
+        for (auto tab_i = window.tabs.begin(); tab_i != window.tabs.end();
+             ++tab_i) {
+          SessionID::id_type restored_tab_browser_id;
+          {
+            const Tab& tab = **tab_i;
+            if (tab.id != id)
+              continue;
+
+            restored_tab_browser_id = tab.browser_id;
+            LiveTab* restored_tab = nullptr;
+            context = RestoreTab(tab, context, disposition, &restored_tab);
+            live_tabs.push_back(restored_tab);
+            window.tabs.erase(tab_i);
+          }

[Sidney San Martín, 2016/08/04 04:24:57]

I had a use-after free here, so I'm using a block to take `tab` out of scope
when it's erased from `window.tabs`. `tab_i` also becomes invalid here, but I
feel like it should already get more scrutiny. Right after I uploaded this patch
set, rsesek@ noted that a comment might be sufficient, but said to leave it up
to you.

+          // If restoring the tab leaves the window with nothing else, delete it
+          // as well.
+          if (window.tabs.empty()) {
+            entries_.erase(entry_iterator);
+          } else {
+            // Update the browser ID of the rest of the tabs in the window so if
+            // any one is restored, it goes into the same window as the tab
+            // being restored now.
+            UpdateTabBrowserIDs(restored_tab_browser_id,
+                                context->GetSessionID().id());
+            for (auto& tab_j : window.tabs)
+              tab_j->browser_id = context->GetSessionID().id();
+          }
+          break;
         }
-        break;
       }
-    }
-    context->ShowBrowserWindow();
+      context->ShowBrowserWindow();
 
-    if (disposition == CURRENT_TAB && current_context &&
-        current_context->GetActiveLiveTab()) {
-      current_context->CloseTab();
+      if (disposition == CURRENT_TAB && current_context &&
+          current_context->GetActiveLiveTab()) {
+        current_context->CloseTab();
+      }
+      break;
     }
-  } else {
-    NOTREACHED();
   }
 
   if (!restoring_tab_in_window) {
-    delete entry;
+    entries_.erase(entry_iterator);
   }
 
   restoring_ = false;
@@ -277,18 +269,17 @@ void TabRestoreServiceHelper::NotifyLoaded() {
                     TabRestoreServiceLoaded(tab_restore_service_));
 }
 
-void TabRestoreServiceHelper::AddEntry(Entry* entry,
+void TabRestoreServiceHelper::AddEntry(std::unique_ptr<Entry> entry,
                                        bool notify,
                                        bool to_front) {
-  if (!FilterEntry(entry) || (entries_.size() >= kMaxEntries && !to_front)) {
-    delete entry;
+  if (!FilterEntry(*entry) || (entries_.size() >= kMaxEntries && !to_front)) {
     return;
   }
 
   if (to_front)
-    entries_.push_front(entry);
+    entries_.push_front(std::move(entry));
   else
-    entries_.push_back(entry);
+    entries_.push_back(std::move(entry));
 
   PruneEntries();
 
@@ -302,19 +293,13 @@ void TabRestoreServiceHelper::AddEntry(Entry* entry,
 void TabRestoreServiceHelper::PruneEntries() {
   Entries new_entries;
 
-  for (TabRestoreService::Entries::const_iterator iter = entries_.begin();
-       iter != entries_.end(); ++iter) {
-    TabRestoreService::Entry* entry = *iter;
-
-    if (FilterEntry(entry) &&
-        new_entries.size() < kMaxEntries) {
-      new_entries.push_back(entry);
-    } else {
-      delete entry;
+  for (auto& entry : entries_) {
+    if (FilterEntry(*entry) && new_entries.size() < kMaxEntries) {
+      new_entries.push_back(std::move(entry));
     }
   }
 
-  entries_ = new_entries;
+  entries_ = std::move(new_entries);
 }
 
 TabRestoreService::Entries::iterator
@@ -326,10 +311,9 @@ TabRestoreServiceHelper::GetEntryIteratorById(SessionID::id_type id) {
     // For Window entries, see if the ID matches a tab. If so, report the window
     // as the Entry.
     if ((*i)->type == TabRestoreService::WINDOW) {
-      std::vector<Tab>& tabs = static_cast<Window*>(*i)->tabs;
-      for (std::vector<Tab>::iterator j = tabs.begin();
-           j != tabs.end(); ++j) {
-        if ((*j).id == id) {
+      auto& window = static_cast<const Window&>(**i);
+      for (const auto& tab : window.tabs) {
+        if (tab->id == id) {
           return i;
         }
       }
@@ -339,13 +323,13 @@ TabRestoreServiceHelper::GetEntryIteratorById(SessionID::id_type id) {
 }
 
 // static
-bool TabRestoreServiceHelper::ValidateEntry(Entry* entry) {
-  if (entry->type == TabRestoreService::TAB)
-    return ValidateTab(static_cast<Tab*>(entry));
-
-  if (entry->type == TabRestoreService::WINDOW)
-    return ValidateWindow(static_cast<Window*>(entry));
-
+bool TabRestoreServiceHelper::ValidateEntry(const Entry& entry) {
+  switch (entry.type) {
+    case TabRestoreService::TAB:
+      return ValidateTab(static_cast<const Tab&>(entry));
+    case TabRestoreService::WINDOW:
+      return ValidateWindow(static_cast<const Window&>(entry));
+  }
   NOTREACHED();
   return false;
 }
@@ -368,8 +352,6 @@ void TabRestoreServiceHelper::PopulateTab(Tab* tab,
   }
   tab->timestamp = TimeNow();
   tab->current_navigation_index = live_tab->GetCurrentEntryIndex();
-  if (tab->current_navigation_index == -1 && entry_count > 0)
-    tab->current_navigation_index = 0;
   tab->tabstrip_index = index;
 
   tab->extension_app_id = client_->GetExtensionAppIDForTab(live_tab);
@@ -397,7 +379,7 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTab(
         tab.extension_app_id, tab.platform_data.get(), tab.user_agent_override);
   } else {
     // We only respsect the tab's original browser if there's no disposition.
-    if (disposition == UNKNOWN && tab.has_browser()) {
+    if (disposition == UNKNOWN && tab.browser_id) {
       context = client_->FindLiveTabContextWithID(tab.browser_id);
     }
 
@@ -410,7 +392,7 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTab(
       tab_index = tab.tabstrip_index;
     } else {
       context = client_->CreateLiveTabContext(std::string());
-      if (tab.has_browser())
+      if (tab.browser_id)
         UpdateTabBrowserIDs(tab.browser_id, context->GetSessionID().id());
     }
 
@@ -436,86 +418,68 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTab(
   return context;
 }
 
+bool TabRestoreServiceHelper::ValidateTab(const Tab& tab) {
+  return !tab.navigations.empty() &&
+         static_cast<size_t>(tab.current_navigation_index) <
+             tab.navigations.size();
+}
 
-bool TabRestoreServiceHelper::ValidateTab(Tab* tab) {
-  if (tab->navigations.empty())
+bool TabRestoreServiceHelper::ValidateWindow(const Window& window) {
+  if (static_cast<size_t>(window.selected_tab_index) >= window.tabs.size()) {
     return false;
+  }
 
-  tab->current_navigation_index =
-      std::max(0, std::min(tab->current_navigation_index,
-                           static_cast<int>(tab->navigations.size()) - 1));
-
-  return true;
-}
-
-bool TabRestoreServiceHelper::ValidateWindow(Window* window) {
-  window->selected_tab_index =
-      std::max(0, std::min(window->selected_tab_index,
-                           static_cast<int>(window->tabs.size() - 1)));
-
-  int i = 0;
-  for (std::vector<Tab>::iterator tab_i = window->tabs.begin();
-       tab_i != window->tabs.end();) {
-    if (!ValidateTab(&(*tab_i))) {
-      tab_i = window->tabs.erase(tab_i);
-      if (i < window->selected_tab_index)
-        window->selected_tab_index--;
-      else if (i == window->selected_tab_index)
-        window->selected_tab_index = 0;
-    } else {
-      ++tab_i;
-      ++i;
+  for (const auto& tab : window.tabs) {
+    if (!ValidateTab(*tab)) {
+      return false;
     }
   }
 
-  if (window->tabs.empty())
-    return false;
-
   return true;
 }
 
-bool TabRestoreServiceHelper::IsTabInteresting(const Tab* tab) {
-  if (tab->navigations.empty())
+bool TabRestoreServiceHelper::IsTabInteresting(const Tab& tab) {
+  if (tab.navigations.empty())
     return false;
 
-  if (tab->navigations.size() > 1)
+  if (tab.navigations.size() > 1)
     return true;
 
-  return tab->pinned ||
-         tab->navigations.at(0).virtual_url() != client_->GetNewTabURL();
+  return tab.pinned ||
+         tab.navigations.at(0).virtual_url() != client_->GetNewTabURL();
 }
 
-bool TabRestoreServiceHelper::IsWindowInteresting(const Window* window) {
-  if (window->tabs.empty())
+bool TabRestoreServiceHelper::IsWindowInteresting(const Window& window) {
+  if (window.tabs.empty())
     return false;
 
-  if (window->tabs.size() > 1)
+  if (window.tabs.size() > 1)
     return true;
 
-  return IsTabInteresting(&window->tabs[0]);
+  return IsTabInteresting(*window.tabs[0]);
 }
 
-bool TabRestoreServiceHelper::FilterEntry(Entry* entry) {
+bool TabRestoreServiceHelper::FilterEntry(const Entry& entry) {
   if (!ValidateEntry(entry))
     return false;
 
-  if (entry->type == TabRestoreService::TAB)
-    return IsTabInteresting(static_cast<Tab*>(entry));
-  else if (entry->type == TabRestoreService::WINDOW)
-    return IsWindowInteresting(static_cast<Window*>(entry));
-
+  switch (entry.type) {
+    case TabRestoreService::TAB:
+      return IsTabInteresting(static_cast<const Tab&>(entry));
+    case TabRestoreService::WINDOW:
+      return IsWindowInteresting(static_cast<const Window&>(entry));
+  }
   NOTREACHED();
   return false;
 }
 
 void TabRestoreServiceHelper::UpdateTabBrowserIDs(SessionID::id_type old_id,
                                                   SessionID::id_type new_id) {
-  for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
-    Entry* entry = *i;
+  for (const auto& entry : entries_) {
     if (entry->type == TabRestoreService::TAB) {
-      Tab* tab = static_cast<Tab*>(entry);
-      if (tab->browser_id == old_id)
-        tab->browser_id = new_id;
+      auto& tab = static_cast<Tab&>(*entry);
+      if (tab.browser_id == old_id)
+        tab.browser_id = new_id;
     }
   }
 }