Simplify remapping of obsolete session IDs

The new implementation removes some dead code (instead use DCHECKs) and
avoids unnecessary remappings when no tabbed windows exist (e.g.
multiple custom tabs exist without native data).

Bug: 843554
Change-Id: I595516158cc8e5aafb13346a962bf92f70ea6aad
Reviewed-on: https://chromium-review.googlesource.com/1068674
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561036}

components/sync_sessions/local_session_event_handler_impl.cc

@@ -133,7 +133,7 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
     // session; the current tabbed windows are now the source of truth.
     session_tracker_->ResetSessionTracking(current_session_tag_);
     current_session->modified_time = base::Time::Now();
-  } else {
+  } else if (option == RELOAD_TABS) {
     DVLOG(1) << "Found no tabbed windows. Reloading "
              << current_session->windows.size()
              << " windows from previous session.";
@@ -149,12 +149,10 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
     // through and update them to the new SessionIds.
     for (auto& win_iter : current_session->windows) {
       for (auto& tab : win_iter.second->wrapped_window.tabs) {
-        int sync_id = TabNodePool::kInvalidTabNodeID;
-        if (!session_tracker_->GetTabNodeFromLocalTabId(tab->tab_id,
-                                                        &sync_id) ||
-            sync_id == TabNodePool::kInvalidTabNodeID) {
-          continue;
-        }
+        int sync_id = session_tracker_->LookupTabNodeFromTabId(
+            current_session_tag_, tab->tab_id);
+        DCHECK_NE(sync_id, TabNodePool::kInvalidTabNodeID);
+
         DVLOG(1) << "Rewriting tab node " << sync_id << " with tab id "
                  << tab->tab_id.id();
         AppendChangeForExistingTab(sync_id, *tab, batch);

components/sync_sessions/local_session_event_handler_impl_unittest.cc

@@ -497,5 +497,70 @@ TEST_F(LocalSessionEventHandlerImplTest, PropagateNewWindow) {
   AddTab(kWindowId2, kBaz1, kTabId3);
 }
 
+TEST_F(LocalSessionEventHandlerImplTest,
+       PropagateNewNavigationWithoutTabbedWindows) {
+  const int kTabNodeId1 = 0;
+  const int kTabNodeId2 = 1;
+
+  // The tracker is initially restored from persisted state, containing two
+  // custom tabs.
+  sync_pb::SessionSpecifics custom_tab1;
+  custom_tab1.set_session_tag(kSessionTag);
+  custom_tab1.set_tab_node_id(kTabNodeId1);
+  custom_tab1.mutable_tab()->set_window_id(kWindowId1);
+  custom_tab1.mutable_tab()->set_tab_id(kTabId1);
+  session_tracker_.ReassociateLocalTab(kTabNodeId1,
+                                       SessionID::FromSerializedValue(kTabId1));
+  UpdateTrackerWithSpecifics(custom_tab1, base::Time::Now(), &session_tracker_);
+
+  sync_pb::SessionSpecifics custom_tab2;
+  custom_tab2.set_session_tag(kSessionTag);
+  custom_tab2.set_tab_node_id(kTabNodeId2);
+  custom_tab2.mutable_tab()->set_window_id(kWindowId2);
+  custom_tab2.mutable_tab()->set_tab_id(kTabId2);
+  session_tracker_.ReassociateLocalTab(kTabNodeId2,
+                                       SessionID::FromSerializedValue(kTabId2));
+  UpdateTrackerWithSpecifics(custom_tab2, base::Time::Now(), &session_tracker_);
+
+  sync_pb::SessionSpecifics header;
+  header.set_session_tag(kSessionTag);
+  header.mutable_header()->add_window()->set_window_id(kWindowId1);
+  header.mutable_header()->mutable_window(0)->add_tab(kTabId1);
+  header.mutable_header()->add_window()->set_window_id(kWindowId2);
+  header.mutable_header()->mutable_window(1)->add_tab(kTabId2);
+  UpdateTrackerWithSpecifics(header, base::Time::Now(), &session_tracker_);
+
+  ASSERT_THAT(session_tracker_.LookupSession(kSessionTag),
+              MatchesSyncedSession(kSessionTag,
+                                   {{kWindowId1, std::vector<int>{kTabId1}},
+                                    {kWindowId2, std::vector<int>{kTabId2}}}));
+
+  AddWindow(kWindowId1, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  TestSyncedTabDelegate* tab1 = AddTab(kWindowId1, kFoo1, kTabId1);
+  tab1->SetSyncId(kTabNodeId1);
+
+  AddWindow(kWindowId2, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  AddTab(kWindowId2, kBar1, kTabId2)->SetSyncId(kTabNodeId2);
+
+  InitHandler();
+
+  auto update_mock_batch = std::make_unique<StrictMock<MockWriteBatch>>();
+  // Note that the header is reported again, although it hasn't changed. This is
+  // OK because sync will avoid updating an entity with identical content.
+  EXPECT_CALL(*update_mock_batch,
+              DoUpdate(Pointee(MatchesHeader(
+                  kSessionTag, {kWindowId1, kWindowId2}, {kTabId1, kTabId2}))));
+  EXPECT_CALL(
+      *update_mock_batch,
+      DoUpdate(Pointee(MatchesTab(kSessionTag, kWindowId1, kTabId1, kTabNodeId1,
+                                  /*urls=*/{kFoo1, kBaz1}))));
+  EXPECT_CALL(*update_mock_batch, Commit());
+
+  EXPECT_CALL(mock_delegate_, CreateLocalSessionWriteBatch())
+      .WillOnce(Return(ByMove(std::move(update_mock_batch))));
+
+  tab1->Navigate(kBaz1);
+}
+
 }  // namespace
 }  // namespace sync_sessions

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -669,6 +669,107 @@ TEST_F(SessionSyncBridgeTest, ShouldRestoreTabbedDataIfNoWindowsDuringStartup) {
   tab->Navigate("http://bar.com/");
 }
 
+// Ensure that tabbed windows from a previous session are preserved if only
+// a custom tab is present at startup.
+TEST_F(SessionSyncBridgeTest, ShouldPreserveTabbedDataIfCustomTabOnlyFound) {
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
+
+  AddWindow(kWindowId1);
+  AddTab(kWindowId1, "http://foo.com/");
+
+  InitializeBridge();
+  StartSyncing();
+
+  ASSERT_THAT(GetAllData(),
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(
+                              MatchesHeader(kLocalSessionTag, _, _))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, _, _,
+                              /*tab_node_id=*/0, {"http://foo.com/"})))));
+
+  ShutdownBridge();
+
+  // Start the bridge with only a custom tab open.
+  ResetWindows();
+  AddWindow(kWindowId2, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  AddTab(kWindowId2, "http://bar.com/");
+  InitializeBridge();
+  StartSyncing();
+
+  // The previous session should be preserved. The transient window cannot be
+  // synced because we do not have enough local data to ensure that we wouldn't
+  // vend the same sync ID if our persistent storage didn't match upon the last
+  // shutdown.
+  EXPECT_THAT(GetAllData(),
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(
+                              MatchesHeader(kLocalSessionTag, _, _))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, _, _,
+                              /*tab_node_id=*/0, {"http://foo.com/"})))));
+}
+
+// Ensure that tabbed windows from a previous session are preserved and combined
+// with a custom tab that was newly found during startup.
+TEST_F(SessionSyncBridgeTest, ShouldPreserveTabbedDataIfNewCustomTabAlsoFound) {
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
+  const int kTabId1 = 1000003;
+  const int kTabId2 = 1000004;
+
+  AddWindow(kWindowId1);
+  AddTab(kWindowId1, "http://foo.com/", kTabId1);
+
+  InitializeBridge();
+  StartSyncing();
+
+  ASSERT_THAT(GetAllData(),
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                              kLocalSessionTag, {kWindowId1}, {kTabId1}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId1, kTabId1,
+                              /*tab_node_id=*/0, {"http://foo.com/"})))));
+
+  ShutdownBridge();
+
+  // Start the bridge with an additional local custom tab.
+  AddWindow(kWindowId2, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  AddTab(kWindowId2, "http://bar.com/", kTabId2);
+  InitializeBridge();
+  StartSyncing();
+
+  EXPECT_THAT(GetAllData(),
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                              kLocalSessionTag, {kWindowId1, kWindowId2},
+                              {kTabId1, kTabId2}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId1, kTabId1,
+                              /*tab_node_id=*/0, {"http://foo.com/"}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId2, kTabId2,
+                              /*tab_node_id=*/1, {"http://bar.com/"})))));
+}
+
+// Ensure that, in a scenario without prior sync data, encountering a custom
+// tab only ( no tabbed window) does not vend new sync IDs.
+TEST_F(SessionSyncBridgeTest, ShouldIgnoreIfCustomTabOnlyOnStartup) {
+  const int kWindowId = 1000001;
+
+  AddWindow(kWindowId, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  AddTab(kWindowId, "http://foo.com/");
+
+  InitializeBridge();
+  StartSyncing();
+
+  EXPECT_THAT(GetAllData(),
+              UnorderedElementsAre(Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                                               kLocalSessionTag, _, _)))));
+}
+
 TEST_F(SessionSyncBridgeTest, ShouldDisableSyncAndReenable) {
   const int kWindowId = 1000001;
   const int kTabId = 1000002;