Fix deserialization of unmapped tabs

An unmapped tab can still have an associated |tab_node_id| (e.g. a
foreign tab that has been closed), so the assumption behind the CHECK
prior to this patch was wrong, and the implementation buggy.

According to the reports we've investigated, this scenario occurs for
both local and remote sessions.

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

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -155,7 +155,7 @@ sync_pb::SessionSpecifics CreateTabSpecifics(const std::string& session_tag,
   sync_pb::SessionSpecifics specifics;
   specifics.set_session_tag(session_tag);
   specifics.set_tab_node_id(tab_node_id);
-  specifics.mutable_tab()->add_navigation()->set_virtual_url("http://baz.com/");
+  specifics.mutable_tab()->add_navigation()->set_virtual_url(url);
   specifics.mutable_tab()->set_window_id(window_id);
   specifics.mutable_tab()->set_tab_id(tab_id);
   return specifics;
@@ -735,6 +735,71 @@ TEST_F(SessionSyncBridgeTest, ShouldMergeForeignSession) {
                   {{kForeignWindowId, std::vector<int>{kForeignTabId}}})));
 }
 
+// Regression test for crbug.com/837517: Ensure that the bridge doesn't crash
+// and closed foreign tabs (|kForeignTabId2| in the test) are not exposed after
+// restarting the browser.
+TEST_F(SessionSyncBridgeTest, ShouldNotExposeClosedTabsAfterRestart) {
+  const std::string kForeignSessionTag = "foreignsessiontag";
+  const int kForeignWindowId = 2000001;
+  const int kForeignTabId1 = 2000002;
+  const int kForeignTabId2 = 2000003;
+  const int kForeignTabNodeId1 = 2004;
+  const int kForeignTabNodeId2 = 2005;
+
+  // The header only lists a single tab |kForeignTabId1|, which becomes a mapped
+  // tab.
+  const sync_pb::SessionSpecifics foreign_header =
+      CreateHeaderSpecificsWithOneTab(kForeignSessionTag, kForeignWindowId,
+                                      kForeignTabId1);
+  const sync_pb::SessionSpecifics foreign_tab1 =
+      CreateTabSpecifics(kForeignSessionTag, kForeignWindowId, kForeignTabId1,
+                         kForeignTabNodeId1, "http://foo.com/");
+  // |kForeignTabId2| is not present in the header, leading to an unmapped tab.
+  const sync_pb::SessionSpecifics foreign_tab2 =
+      CreateTabSpecifics(kForeignSessionTag, kForeignWindowId, kForeignTabId2,
+                         kForeignTabNodeId2, "http://bar.com/");
+
+  InitializeBridge();
+  StartSyncing({foreign_header, foreign_tab1, foreign_tab2});
+
+  const std::string local_header_storage_key =
+      SessionStore::GetHeaderStorageKey(kLocalSessionTag);
+  const std::string foreign_header_storage_key =
+      SessionStore::GetHeaderStorageKey(kForeignSessionTag);
+  const std::string foreign_tab_storage_key1 =
+      SessionStore::GetTabStorageKey(kForeignSessionTag, kForeignTabNodeId1);
+  const std::string foreign_tab_storage_key2 =
+      SessionStore::GetTabStorageKey(kForeignSessionTag, kForeignTabNodeId2);
+
+  ASSERT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(local_header_storage_key, _),
+          Pair(foreign_header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(
+                   kForeignSessionTag, {kForeignWindowId}, {kForeignTabId1}))),
+          Pair(foreign_tab_storage_key1,
+               EntityDataHasSpecifics(MatchesTab(
+                   kForeignSessionTag, kForeignWindowId, kForeignTabId1,
+                   kForeignTabNodeId1, {"http://foo.com/"}))),
+          Pair(foreign_tab_storage_key2,
+               EntityDataHasSpecifics(MatchesTab(
+                   kForeignSessionTag, kForeignWindowId, kForeignTabId2,
+                   kForeignTabNodeId2, {"http://bar.com/"})))));
+
+  // Mimic a browser restart, which should restore the very same state (and not
+  // crash!).
+  ShutdownBridge();
+  InitializeBridge();
+  StartSyncing();
+
+  EXPECT_THAT(GetAllData(),
+              UnorderedElementsAre(Pair(local_header_storage_key, _),
+                                   Pair(foreign_header_storage_key, _),
+                                   Pair(foreign_tab_storage_key1, _),
+                                   Pair(foreign_tab_storage_key2, _)));
+}
+
 TEST_F(SessionSyncBridgeTest, ShouldHandleRemoteDeletion) {
   const std::string kForeignSessionTag = "foreignsessiontag";
   const int kForeignWindowId = 2000001;

components/sync_sessions/synced_session_tracker.cc

@@ -769,14 +769,10 @@ void SerializePartialTrackerToSpecifics(
       // Tab entities.
       const SessionID tab_id =
           tracker.LookupTabIdFromTabNodeId(session_tag, tab_node_id);
-      if (tab_id.is_valid()) {
-        // Associated tab node.
-        const sessions::SessionTab* tab =
-            tracker.LookupSessionTab(session_tag, tab_id);
-        // TODO(crbug.com/837517): Replace with DCHECK once the crasher
-        // investigation is finalized.
-        CHECK(tab);
-
+      const sessions::SessionTab* tab =
+          tracker.LookupSessionTab(session_tag, tab_id);
+      if (tab) {
+        // Associated/mapped tab node.
         sync_pb::SessionSpecifics tab_pb;
         tab_pb.set_session_tag(session_tag);
         tab_pb.set_tab_node_id(tab_node_id);
@@ -785,12 +781,12 @@ void SerializePartialTrackerToSpecifics(
         continue;
       }
 
-      // Create dummy tab entities for free nodes (i.e. ones that are known by
-      // the tracker but not associated to a tab ID).
+      // Create entities for unmapped tabs nodes (possibly not even associated
+      // to a tab ID).
       sync_pb::SessionSpecifics tab_pb;
       tab_pb.set_tab_node_id(tab_node_id);
       tab_pb.set_session_tag(session_tag);
-      tab_pb.mutable_tab();
+      tab_pb.mutable_tab()->set_tab_id(tab_id.id());  // Might be invalid.
       output_cb.Run(session->session_name, &tab_pb);
     }
   }

components/sync_sessions/synced_session_tracker_unittest.cc

@@ -1039,4 +1039,30 @@ TEST_F(SyncedSessionTrackerTest, SerializeTrackerToSpecifics) {
       callback.Get());
 }
 
+TEST_F(SyncedSessionTrackerTest, SerializeTrackerToSpecificsWithEmptyHeader) {
+  sync_pb::SessionSpecifics tab;
+  tab.set_session_tag(kTag);
+  tab.set_tab_node_id(kTabNode1);
+  tab.mutable_tab()->set_window_id(kWindow1.id());
+  tab.mutable_tab()->set_tab_id(kTab1.id());
+  UpdateTrackerWithSpecifics(tab, base::Time::Now(), GetTracker());
+
+  sync_pb::SessionSpecifics header;
+  header.set_session_tag(kTag);
+  header.mutable_header()->set_client_name(kSessionName);
+  UpdateTrackerWithSpecifics(header, base::Time::Now(), GetTracker());
+
+  base::MockCallback<base::RepeatingCallback<void(
+      const std::string& session_name, sync_pb::SessionSpecifics* specifics)>>
+      callback;
+  EXPECT_CALL(callback,
+              Run(kSessionName, Pointee(MatchesHeader(kTag, /*window_ids=*/{},
+                                                      /*tab_ids=*/{}))));
+  EXPECT_CALL(
+      callback,
+      Run(kSessionName, Pointee(MatchesTab(kTag, /*window_id=*/0, kTab1.id(),
+                                           kTabNode1, /*urls=*/_))));
+  SerializeTrackerToSpecifics(*GetTracker(), callback.Get());
+}
+
 }  // namespace sync_sessions