Avoid potential DCHECK failures in SessionStore

With DCHECKs enabled, GetStorageKey() might potentially fail in some
codepaths that are updated in this patch.

In addition, we introduce a CHECK for a scenario that is subject to
investigation.

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

components/sync_sessions/session_store.cc

@@ -574,7 +574,10 @@ std::unique_ptr<syncer::DataBatch> SessionStore::GetSessionDataForKeys(
           [](syncer::MutableDataBatch* batch, const std::string& session_name,
              sync_pb::SessionSpecifics* specifics) {
             // Local variable used to avoid assuming argument evaluation order.
-            const std::string storage_key = GetStorageKey(*specifics);
+            // We also avoid GetStorageKey() because specifics might not be
+            // valid according to AreValidSpecifics().
+            const std::string storage_key = EncodeStorageKey(
+                specifics->session_tag(), specifics->tab_node_id());
             batch->Put(storage_key, MoveToEntityData(session_name, specifics));
           },
           batch.get()));
@@ -589,7 +592,10 @@ std::unique_ptr<syncer::DataBatch> SessionStore::GetAllSessionData() const {
           [](syncer::MutableDataBatch* batch, const std::string& session_name,
              sync_pb::SessionSpecifics* specifics) {
             // Local variable used to avoid assuming argument evaluation order.
-            const std::string storage_key = GetStorageKey(*specifics);
+            // We also avoid GetStorageKey() because specifics might not be
+            // valid according to AreValidSpecifics().
+            const std::string storage_key = EncodeStorageKey(
+                specifics->session_tag(), specifics->tab_node_id());
             batch->Put(storage_key, MoveToEntityData(session_name, specifics));
           },
           batch.get()));

components/sync_sessions/session_store_unittest.cc

@@ -575,5 +575,65 @@ TEST_F(SessionStoreTest, ShouldReturnForeignUnmappedTabs) {
                            /*urls=*/_)))));
 }
 
+TEST_F(SessionStoreTest, ShouldReturnForeignOrphanTabs) {
+  const std::string kForeignSessionTag = "SomeForeignTag";
+  const int kWindowId = 5;
+  const int kTabId = 7;
+  // Both tab nodes point to the same tab ID.
+  const int kTabNodeId1 = 2;
+  const int kTabNodeId2 = 3;
+
+  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, kTabNodeId1);
+  const std::string foreign_tab_storage_key2 =
+      SessionStore::GetTabStorageKey(kForeignSessionTag, kTabNodeId2);
+
+  // Local header entity is present initially.
+  ASSERT_THAT(BatchToEntityDataMap(session_store()->GetAllSessionData()),
+              ElementsAre(Pair(local_header_storage_key, _)));
+
+  SessionSpecifics tab1;
+  tab1.set_session_tag(kForeignSessionTag);
+  tab1.set_tab_node_id(kTabNodeId1);
+  tab1.mutable_tab()->set_window_id(kWindowId);
+  tab1.mutable_tab()->set_tab_id(kTabId);
+  ASSERT_TRUE(SessionStore::AreValidSpecifics(tab1));
+
+  SessionSpecifics tab2;
+  tab2.set_session_tag(kForeignSessionTag);
+  tab2.set_tab_node_id(kTabNodeId2);
+  tab2.mutable_tab()->set_window_id(kWindowId);
+  tab2.mutable_tab()->set_tab_id(kTabId);
+  ASSERT_TRUE(SessionStore::AreValidSpecifics(tab2));
+
+  std::unique_ptr<SessionStore::WriteBatch> batch =
+      session_store()->CreateWriteBatch(/*error_handler=*/base::DoNothing());
+  ASSERT_THAT(batch, NotNull());
+  batch->PutAndUpdateTracker(tab1, base::Time::Now());
+  batch->PutAndUpdateTracker(tab2, base::Time::Now());
+  SessionStore::WriteBatch::Commit(std::move(batch));
+
+  EXPECT_THAT(BatchToEntityDataMap(session_store()->GetAllSessionData()),
+              UnorderedElementsAre(
+                  Pair(local_header_storage_key, _),
+                  Pair(foreign_header_storage_key,
+                       EntityDataHasSpecifics(MatchesHeader(kForeignSessionTag,
+                                                            /*window_ids=*/{},
+                                                            /*tab_ids=*/{}))),
+                  Pair(foreign_tab_storage_key1,
+                       EntityDataHasSpecifics(
+                           MatchesTab(kForeignSessionTag, /*window_id=*/0,
+                                      /*tab_id=*/-1, kTabNodeId1,
+                                      /*urls=*/_))),
+                  Pair(foreign_tab_storage_key2,
+                       EntityDataHasSpecifics(MatchesTab(
+                           kForeignSessionTag, kWindowId, kTabId, kTabNodeId2,
+                           /*urls=*/_)))));
+}
+
 }  // namespace
 }  // namespace sync_sessions

components/sync_sessions/synced_session_tracker.cc

@@ -773,7 +773,9 @@ void SerializePartialTrackerToSpecifics(
         // Associated tab node.
         const sessions::SessionTab* tab =
             tracker.LookupSessionTab(session_tag, tab_id);
-        DCHECK(tab);
+        // TODO(crbug.com/837517): Replace with DCHECK once the crasher
+        // investigation is finalized.
+        CHECK(tab);
 
         sync_pb::SessionSpecifics tab_pb;
         tab_pb.set_session_tag(session_tag);