[Sync] Handle reassociation of tabs where the old tab is already mapped.

components/sync_sessions/synced_session_tracker.cc

Index: components/sync_sessions/synced_session_tracker.cc
diff --git a/components/sync_sessions/synced_session_tracker.cc b/components/sync_sessions/synced_session_tracker.cc
index 1b61004e0e268172c836f41b17d55fe47fc5fb94..be1db02c9744ee0ee97e8c7e8b2f7407ab86d1b0 100644
--- a/components/sync_sessions/synced_session_tracker.cc
+++ b/components/sync_sessions/synced_session_tracker.cc
@@ -397,33 +397,69 @@ void SyncedSessionTracker::ReassociateLocalTab(int tab_node_id,
 
   SessionID::id_type old_tab_id =
       local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id);
+  if (new_tab_id == old_tab_id) {
+    return;
+  }
+
   local_tab_pool_.ReassociateTabNode(tab_node_id, new_tab_id);
 
   sessions::SessionTab* tab_ptr = nullptr;
 
+  auto new_tab_iter = synced_tab_map_[local_session_tag_].find(new_tab_id);
   auto old_tab_iter = synced_tab_map_[local_session_tag_].find(old_tab_id);
   if (old_tab_id != kInvalidTabID &&
       old_tab_iter != synced_tab_map_[local_session_tag_].end()) {
     tab_ptr = old_tab_iter->second;

[skym, 2017/05/04 20:07:20]

Is it possible this assigns a nullptr? Cause the new code sure makes it seem
like this is possible. If it shouldn't happen, I like the old way better, of 

else {
  tab_ptr = GetTab(dklsgfjalds);
}

Could also throw a DCHECK up here. But if we are worried old_tab_iter->second
holds a nullptr, feel free to disregard.

[Nicolas Zea, 2017/05/04 23:36:15]

Added DCHECK

+
     // Remove the tab from the synced tab map under the old id.
     synced_tab_map_[local_session_tag_].erase(old_tab_iter);
-  } else {
+
+    if (new_tab_iter != synced_tab_map_[local_session_tag_].end()) {
+      // If both the old and the new tab already exist, delete the new tab
+      // and use the old tab in its place.
+      auto unmapped_tabs_iter =
+          unmapped_tabs_[local_session_tag_].find(new_tab_id);
+      if (unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
+        unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
+      } else {
+        sessions::SessionTab* new_tab_ptr = new_tab_iter->second;
+        for (auto& window_iter_pair : GetSession(local_session_tag_)->windows) {

[skym, 2017/05/04 20:07:20]

Sure would have been nice if things had parent pointers.

[Nicolas Zea, 2017/05/04 23:36:15]

Yeah, this really motivates moving away from the SessionTab/SessionWindow
objects and onto our own entirely new structures.

+          auto tab_iter = std::find_if(

[skym, 2017/05/04 20:07:20]

Did you consider remove_if?

[Nicolas Zea, 2017/05/04 23:36:15]

I think that would prevent me from breaking out?

+              window_iter_pair.second->wrapped_window.tabs.begin(),

[skym, 2017/05/04 20:07:20]

Did you consider assigning window_iter_pair.second->wrapped_window.tabs to a
temp variable?

[Nicolas Zea, 2017/05/04 23:36:15]

Done.

+              window_iter_pair.second->wrapped_window.tabs.end(),
+              [&new_tab_ptr](const std::unique_ptr<sessions::SessionTab>& tab) {
+                return tab.get() == new_tab_ptr;
+              });
+          if (tab_iter != window_iter_pair.second->wrapped_window.tabs.end()) {
+            window_iter_pair.second->wrapped_window.tabs.erase(tab_iter);
+            break;
+          }
+        }
+      }
+
+      synced_tab_map_[local_session_tag_].erase(new_tab_iter);
+    }
+
+    // If the old tab is unmapped, update the tab id under which it is
+    // indexed.
+    auto unmapped_tabs_iter =
+        unmapped_tabs_[local_session_tag_].find(old_tab_id);
+    if (old_tab_id != kInvalidTabID &&
+        unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
+      std::unique_ptr<sessions::SessionTab> tab =
+          std::move(unmapped_tabs_iter->second);
+      DCHECK_EQ(tab_ptr, tab.get());
+      unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
+      unmapped_tabs_[local_session_tag_][new_tab_id] = std::move(tab);
+    }
+  }
+
+  if (tab_ptr == nullptr) {
     // It's possible a placeholder is already in place for the new tab. If so,
     // reuse it, otherwise create a new one (which will default to unmapped).
     tab_ptr = GetTab(local_session_tag_, new_tab_id);
   }
 
-  // If the old tab is unmapped, update the tab id under which it is indexed.
-  auto unmapped_tabs_iter = unmapped_tabs_[local_session_tag_].find(old_tab_id);
-  if (old_tab_id != kInvalidTabID &&
-      unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
-    std::unique_ptr<sessions::SessionTab> tab =
-        std::move(unmapped_tabs_iter->second);
-    DCHECK_EQ(tab_ptr, tab.get());
-    unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
-    unmapped_tabs_[local_session_tag_][new_tab_id] = std::move(tab);
-  }
-
   // Update the tab id.
   if (old_tab_id != kInvalidTabID) {
     DVLOG(1) << "Remapped tab " << old_tab_id << " with node " << tab_node_id