USS: Always commit valid session specifics

A recent bug surfaced the fact that old clients (prior to USS) don't
handle well the case where invalid tab specifics are received over the
sync protocol (namely, with an invalid tab ID).

In the particular case of orphaned foreign tabs, i.e. tabs that have
been overriden -same tab ID- by a different sync entity -different tab
node ID-, the situation is problematic. AFAIK, these entities would only
be submitted by a USS client when custom passphrase is enabled, leading
to crashes in all devices a user is syncing with.

Fortunately, the USS implementation is behind a feature toggle and has
never been enabled or experimented with.

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

components/sync_sessions/session_store.cc

@@ -573,11 +573,9 @@ std::unique_ptr<syncer::DataBatch> SessionStore::GetSessionDataForKeys(
       base::BindRepeating(
           [](syncer::MutableDataBatch* batch, const std::string& session_name,
              sync_pb::SessionSpecifics* specifics) {
+            DCHECK(AreValidSpecifics(*specifics));
             // Local variable used to avoid assuming argument evaluation order.
-            // 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());
+            const std::string storage_key = GetStorageKey(*specifics);
             batch->Put(storage_key, MoveToEntityData(session_name, specifics));
           },
           batch.get()));
@@ -591,11 +589,9 @@ std::unique_ptr<syncer::DataBatch> SessionStore::GetAllSessionData() const {
       base::BindRepeating(
           [](syncer::MutableDataBatch* batch, const std::string& session_name,
              sync_pb::SessionSpecifics* specifics) {
+            DCHECK(AreValidSpecifics(*specifics));
             // Local variable used to avoid assuming argument evaluation order.
-            // 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());
+            const std::string storage_key = GetStorageKey(*specifics);
             batch->Put(storage_key, MoveToEntityData(session_name, specifics));
           },
           batch.get()));

components/sync_sessions/session_store_unittest.cc

@@ -575,11 +575,11 @@ TEST_F(SessionStoreTest, ShouldReturnForeignUnmappedTabs) {
                            /*urls=*/_)))));
 }
 
-TEST_F(SessionStoreTest, ShouldReturnForeignOrphanTabs) {
+TEST_F(SessionStoreTest, ShouldIgnoreForeignOrphanTabs) {
   const std::string kForeignSessionTag = "SomeForeignTag";
   const int kWindowId = 5;
   const int kTabId = 7;
-  // Both tab nodes point to the same tab ID.
+  // Both tab nodes point to the same tab ID, so the second one should prevail.
   const int kTabNodeId1 = 2;
   const int kTabNodeId2 = 3;
 
@@ -587,8 +587,6 @@ TEST_F(SessionStoreTest, ShouldReturnForeignOrphanTabs) {
       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);
 
@@ -610,6 +608,7 @@ TEST_F(SessionStoreTest, ShouldReturnForeignOrphanTabs) {
   tab2.mutable_tab()->set_tab_id(kTabId);
   ASSERT_TRUE(SessionStore::AreValidSpecifics(tab2));
 
+  // Store the two foreign tabs, in order.
   std::unique_ptr<SessionStore::WriteBatch> batch =
       session_store()->CreateWriteBatch(/*error_handler=*/base::DoNothing());
   ASSERT_THAT(batch, NotNull());
@@ -617,6 +616,8 @@ TEST_F(SessionStoreTest, ShouldReturnForeignOrphanTabs) {
   batch->PutAndUpdateTracker(tab2, base::Time::Now());
   SessionStore::WriteBatch::Commit(std::move(batch));
 
+  // The first foreign tab should have been overwritten by the second one,
+  // because they shared a tab ID.
   EXPECT_THAT(BatchToEntityDataMap(session_store()->GetAllSessionData()),
               UnorderedElementsAre(
                   Pair(local_header_storage_key, _),
@@ -624,11 +625,6 @@ TEST_F(SessionStoreTest, ShouldReturnForeignOrphanTabs) {
                        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,

components/sync_sessions/synced_session_tracker.cc

@@ -776,6 +776,16 @@ void SerializePartialTrackerToSpecifics(
       // Tab entities.
       const SessionID tab_id =
           tracker.LookupTabIdFromTabNodeId(session_tag, tab_node_id);
+      if (!tab_id.is_valid()) {
+        // This can be the case for tabs that were unmapped because we received
+        // a new foreign tab with the same tab ID (the last one wins), so we
+        // don't remember the tab ID for the original |tab_node_id|. Instead of
+        // returning partially populated SessionSpecifics (without tab ID), we
+        // simply drop them, because older clients don't handle well such
+        // invalid specifics.
+        continue;
+      }
+
       const sessions::SessionTab* tab =
           tracker.LookupSessionTab(session_tag, tab_id);
       if (tab) {
@@ -788,12 +798,11 @@ void SerializePartialTrackerToSpecifics(
         continue;
       }
 
-      // Create entities for unmapped tabs nodes (possibly not even associated
-      // to a tab ID).
+      // Create entities for unmapped tabs nodes.
       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()->set_tab_id(tab_id.id());  // Might be invalid.
+      tab_pb.mutable_tab()->set_tab_id(tab_id.id());
       output_cb.Run(session->session_name, &tab_pb);
     }
   }

components/sync_sessions/synced_session_tracker_unittest.cc

@@ -1018,10 +1018,6 @@ TEST_F(SyncedSessionTrackerTest, SerializeTrackerToSpecifics) {
       callback,
       Run(kSessionName, Pointee(MatchesTab(kTag, Ne(kWindow1.id()), kTab3.id(),
                                            kTabNode3, /*urls=*/_))));
-  EXPECT_CALL(
-      callback,
-      Run(kSessionName, Pointee(MatchesTab(kTag, /*window_id=*/0, /*tab_id=*/-1,
-                                           kTabNode4, /*urls=*/_))));
 
   SerializeTrackerToSpecifics(*GetTracker(), callback.Get());
   EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(&callback));