Delay sending TabDetachedAt and TabClosingAt observer notifications.

Previously, the notifications were sent during the tab-close-loop [if multiple
tabs were being closed]. This made reentrancy particularly dangerous.

The logic in Browser::TabDetachedAt and Browser::TabInsertedAt relied on the
|index| parameter to determine whether the index of the active tab has changed
[for which there is no notification]. This CL changes those methods to always
call SessionService::SetSelectedTabInWindow, and modifies
SessionService::SetSelectedTabInWindow to be idempotent.

Bug: 842194
Change-Id: I2fc54749e495ab9c925bdba89ef857c4ea3537d9
Reviewed-on: https://chromium-review.googlesource.com/1055732Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558530}

chrome/browser/sessions/session_service.cc

@@ -292,6 +292,7 @@ void SessionService::WindowClosed(const SessionID& window_id) {
   }
 
   windows_tracking_.erase(window_id);
+  last_selected_tab_in_window_.erase(window_id);
 
   if (window_closing_ids_.find(window_id) != window_closing_ids_.end()) {
     window_closing_ids_.erase(window_id);
@@ -469,6 +470,11 @@ void SessionService::SetSelectedTabInWindow(const SessionID& window_id,
   if (!ShouldTrackChangesToWindow(window_id))
     return;
 
+  auto it = last_selected_tab_in_window_.find(window_id);
+  if (it != last_selected_tab_in_window_.end() && it->second == index)
+    return;
+  last_selected_tab_in_window_[window_id] = index;
+
   ScheduleCommand(
       sessions::CreateSetSelectedTabInWindowCommand(window_id, index));
 }
@@ -757,6 +763,7 @@ void SessionService::ScheduleResetCommands() {
   base_session_service_->ClearPendingCommands();
   tab_to_available_range_.clear();
   windows_tracking_.clear();
+  last_selected_tab_in_window_.clear();
   rebuild_on_next_save_ = false;
   BuildCommandsFromBrowsers(&tab_to_available_range_,
                             &windows_tracking_);

chrome/browser/sessions/session_service.h

@@ -384,6 +384,10 @@ class SessionService : public sessions::BaseSessionServiceDelegate,
   // Force session commands to be rebuild before next save event.
   bool rebuild_on_next_save_;
 
+  // Don't send duplicate SetSelectedTabInWindow commands when the selected
+  // tab's index hasn't changed.
+  std::map<SessionID, int> last_selected_tab_in_window_;
+
   base::WeakPtrFactory<SessionService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SessionService);

chrome/browser/ui/browser.cc

@@ -1027,15 +1027,12 @@ void Browser::TabClosingAt(TabStripModel* tab_strip_model,
 }
 
 void Browser::TabDetachedAt(WebContents* contents, int index, bool was_active) {
-  // TabDetachedAt is called before TabStripModel has updated the
-  // active index.
-  int old_active_index = tab_strip_model_->active_index();
-  if (index < old_active_index && !tab_strip_model_->closing_all()) {
+  if (!tab_strip_model_->closing_all()) {
     SessionService* session_service =
         SessionServiceFactory::GetForProfileIfExisting(profile_);
     if (session_service) {
       session_service->SetSelectedTabInWindow(session_id(),
-                                              old_active_index - 1);
+                                              tab_strip_model_->active_index());
     }
   }
 

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -199,7 +199,7 @@ struct TabStripModel::DetachNotifications {
 
   // The WebContents that were recently detached. Observers need to be notified
   // about these. These must be updated after construction.
-  std::vector<DetachedWebContents> detached_web_contents;
+  std::vector<std::unique_ptr<DetachedWebContents>> detached_web_contents;
 
   // The selection model prior to any tabs being detached.
   const ui::ListSelectionModel selection_model;
@@ -346,13 +346,14 @@ std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsAt(
 
   DetachNotifications notifications(initially_active_web_contents,
                                     selection_model_, /*will_delete=*/false);
-  DetachedWebContents dwc(
-      index, index,
-      DetachWebContentsImpl(index, /*create_historical_tab=*/false,
-                            /*will_delete=*/false));
+  std::unique_ptr<DetachedWebContents> dwc =
+      std::make_unique<DetachedWebContents>(
+          index, index,
+          DetachWebContentsImpl(index, /*create_historical_tab=*/false,
+                                /*will_delete=*/false));
   notifications.detached_web_contents.push_back(std::move(dwc));
   SendDetachWebContentsNotifications(&notifications);
-  return std::move(notifications.detached_web_contents[0].contents);
+  return std::move(notifications.detached_web_contents[0]->contents);
 }
 
 std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsImpl(
@@ -376,18 +377,8 @@ std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsImpl(
   std::unique_ptr<WebContentsData> old_data = std::move(contents_data_[index]);
   contents_data_.erase(contents_data_.begin() + index);
 
-  if (will_delete) {
-    for (auto& observer : observers_)
-      observer.TabClosingAt(this, raw_web_contents, index);
-  }
-
-  if (empty())
-    closing_all_ = true;
-
-  for (auto& observer : observers_)
-    observer.TabDetachedAt(raw_web_contents, index, index == active_index());
-
   if (empty()) {
+    closing_all_ = true;
     selection_model_.Clear();
   } else {
     int old_active = active_index();
@@ -414,14 +405,45 @@ void TabStripModel::SendDetachWebContentsNotifications(
     DetachNotifications* notifications) {
   bool was_any_tab_selected = false;
 
+  // Sort the DetachedWebContents in decreasing order of
+  // |index_before_any_removals|. This is because |index_before_any_removals| is
+  // used by observers to update their own copy of TabStripModel state, and each
+  // removal affects subsequent removals of higher index.
+  std::sort(notifications->detached_web_contents.begin(),
+            notifications->detached_web_contents.end(),
+            [](const std::unique_ptr<DetachedWebContents>& dwc1,
+               const std::unique_ptr<DetachedWebContents>& dwc2) {
+              return dwc1->index_before_any_removals >
+                     dwc2->index_before_any_removals;
+            });
+  for (auto& dwc : notifications->detached_web_contents) {
+    // TabClosingAt() must be sent before TabDetachedAt(), since some observers
+    // use the former to change the behavior of the latter.
+    // TODO(erikchen): Combine these notifications. https://crbug.com/842194.
+    if (notifications->will_delete) {
+      for (auto& observer : observers_) {
+        observer.TabClosingAt(this, dwc->contents.get(),
+                              dwc->index_before_any_removals);
+      }
+    }
+
+    // TabDetachedAt() allows observers that keep their own model of
+    // |contents_data_| to keep that model in sync.
+    for (auto& observer : observers_) {
+      observer.TabDetachedAt(
+          dwc->contents.get(), dwc->index_before_any_removals,
+          notifications->initially_active_web_contents == dwc->contents.get());
+    }
+  }
+
   for (auto& dwc : notifications->detached_web_contents) {
     if (notifications->selection_model.IsSelected(
-            dwc.index_before_any_removals)) {
+            dwc->index_before_any_removals)) {
       was_any_tab_selected = true;
     }
 
     if (notifications->initially_active_web_contents &&
-        dwc.contents.get() == notifications->initially_active_web_contents) {
+        dwc->contents.get() == notifications->initially_active_web_contents) {
       for (auto& observer : observers_)
         observer.TabDeactivated(notifications->initially_active_web_contents);
 
@@ -435,7 +457,7 @@ void TabStripModel::SendDetachWebContentsNotifications(
     if (notifications->will_delete) {
       // This destroys the WebContents, which will also send
       // WebContentsDestroyed notifications.
-      dwc.contents.reset();
+      dwc->contents.reset();
     }
   }
 
@@ -1332,11 +1354,12 @@ bool TabStripModel::CloseWebContentses(
       continue;
     }
 
-    DetachedWebContents dwc(
-        original_indices[i], current_index,
-        DetachWebContentsImpl(current_index,
-                              close_types & CLOSE_CREATE_HISTORICAL_TAB,
-                              /*will_delete=*/true));
+    std::unique_ptr<DetachedWebContents> dwc =
+        std::make_unique<DetachedWebContents>(
+            original_indices[i], current_index,
+            DetachWebContentsImpl(current_index,
+                                  close_types & CLOSE_CREATE_HISTORICAL_TAB,
+                                  /*will_delete=*/true));
     notifications.detached_web_contents.push_back(std::move(dwc));
   }