Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(497)

Unified Diff: components/sessions/core/tab_restore_service_helper.cc

Issue 2200993004: Make TabRestoreService::Entry noncopyable and fix up surrounding code. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@tab-test-cleanup
Patch Set: Eliminate a use-after-free, Windows build fix Created 4 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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
+ // 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;
}
}
}

Powered by Google App Engine
This is Rietveld 408576698