[sync] Adopt cache GUIDs as session tags

The general idea is to adopt sync cache GUID to identify the local
session, making this datatype more similar to others. This also allows
the adoption techniques like IsRecentLocalCacheGuid() to filter out
copies of the local device and avoid listing them as remote devices.

The old tag, stored in preferences, is kept around (marked as legacy in
code) to avoid a one-off discrepancy that would run into a large traffic
to servers and possibly degraded user experience. The new method is
hence used for *newly* created sessions.

This new logic is introduced behind feature toggle (enabled by default)
in case it turns out to be problematic.

Change-Id: I18b71291ea8ffc6991ce0c2ea4e388c048a880c5
Bug: 1159455
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595372
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838136}

chrome/browser/sync/session_sync_service_factory.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/device_info_sync_service_factory.h"
 #include "chrome/browser/sync/glue/sync_start_util.h"
 #include "chrome/browser/sync/model_type_store_service_factory.h"
 #include "chrome/browser/sync/sessions/sync_sessions_web_contents_router.h"
@@ -20,6 +21,8 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/sync/model/model_type_store_service.h"
+#include "components/sync_device_info/device_info_sync_service.h"
+#include "components/sync_device_info/device_info_tracker.h"
 #include "components/sync_sessions/session_sync_prefs.h"
 #include "components/sync_sessions/session_sync_service_impl.h"
 #include "components/sync_sessions/sync_sessions_client.h"
@@ -84,6 +87,12 @@ class SyncSessionsClientImpl : public sync_sessions::SyncSessionsClient {
     return ShouldSyncURLImpl(url);
   }
 
+  bool IsRecentLocalCacheGuid(const std::string& cache_guid) const override {
+    return DeviceInfoSyncServiceFactory::GetForProfile(profile_)
+        ->GetDeviceInfoTracker()
+        ->IsRecentLocalCacheGuid(cache_guid);
+  }
+
   sync_sessions::SyncedWindowDelegatesGetter* GetSyncedWindowDelegatesGetter()
       override {
     return window_delegates_getter_.get();

components/sync/protocol/session_specifics.proto

@@ -19,7 +19,8 @@ package sync_pb;
 import "components/sync/protocol/sync_enums.proto";
 
 message SessionSpecifics {
-  // Unique id for the client.
+  // Unique id for the client. M89 and higher use sync's cache GUID (client ID)
+  // to populate this tag for *new* sessions.
   optional string session_tag = 1;
   optional SessionHeader header = 2;
   optional SessionTab tab = 3;

components/sync_sessions/mock_sync_sessions_client.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_SYNC_SESSIONS_MOCK_SYNC_SESSIONS_CLIENT_H_
 #define COMPONENTS_SYNC_SESSIONS_MOCK_SYNC_SESSIONS_CLIENT_H_
 
+#include <string>
+
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
@@ -23,6 +25,10 @@ class MockSyncSessionsClient : public SyncSessionsClient {
               (override));
   MOCK_METHOD(void, ClearAllOnDemandFavicons, (), (override));
   MOCK_METHOD(bool, ShouldSyncURL, (const GURL& url), (const override));
+  MOCK_METHOD(bool,
+              IsRecentLocalCacheGuid,
+              (const std::string& cache_guid),
+              (const override));
   MOCK_METHOD(SyncedWindowDelegatesGetter*,
               GetSyncedWindowDelegatesGetter,
               (),

components/sync_sessions/session_store.cc

@@ -12,6 +12,7 @@
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
+#include "base/feature_list.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ -26,6 +27,7 @@
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync_device_info/local_device_info_util.h"
 #include "components/sync_sessions/session_sync_prefs.h"
+#include "components/sync_sessions/switches.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 
 namespace sync_sessions {
@@ -80,16 +82,24 @@ std::unique_ptr<syncer::EntityData> MoveToEntityData(
 std::string GetSessionTagWithPrefs(const std::string& cache_guid,
                                    SessionSyncPrefs* sync_prefs) {
   DCHECK(sync_prefs);
-  const std::string persisted_guid = sync_prefs->GetSyncSessionsGUID();
+
+  // If a legacy GUID exists, keep honoring it.
+  const std::string persisted_guid = sync_prefs->GetLegacySyncSessionsGUID();
   if (!persisted_guid.empty()) {
     DVLOG(1) << "Restoring persisted session sync guid: " << persisted_guid;
     return persisted_guid;
   }
 
+  if (base::FeatureList::IsEnabled(
+          switches::kSyncUseCacheGuidAsSyncSessionTag)) {
+    DVLOG(1) << "Using sync cache guid as session sync guid: " << cache_guid;
+    return cache_guid;
+  }
+
   const std::string new_guid =
       base::StringPrintf("session_sync%s", cache_guid.c_str());
   DVLOG(1) << "Creating session sync guid: " << new_guid;
-  sync_prefs->SetSyncSessionsGUID(new_guid);
+  sync_prefs->SetLegacySyncSessionsGUID(new_guid);
   return new_guid;
 }
 
@@ -407,6 +417,7 @@ SessionStore::SessionStore(
     SyncSessionsClient* sessions_client)
     : local_session_info_(local_session_info),
       store_(std::move(underlying_store)),
+      sessions_client_(sessions_client),
       session_tracker_(sessions_client) {
   session_tracker_.InitLocalSession(local_session_info_.session_tag,
                                     local_session_info_.client_name,
@@ -542,6 +553,11 @@ void SessionStore::DeleteAllDataAndMetadata() {
   session_tracker_.Clear();
   store_->DeleteAllDataAndMetadata(base::DoNothing());
 
+  if (base::FeatureList::IsEnabled(
+          switches::kSyncUseCacheGuidAsSyncSessionTag)) {
+    sessions_client_->GetSessionSyncPrefs()->ClearLegacySyncSessionsGUID();
+  }
+
   // At all times, the local session must be tracked.
   session_tracker_.InitLocalSession(local_session_info_.session_tag,
                                     local_session_info_.client_name,

components/sync_sessions/session_store.h

@@ -162,6 +162,8 @@ class SessionStore {
   // In charge of actually persisting changes to disk.
   const std::unique_ptr<syncer::ModelTypeStore> store_;
 
+  SyncSessionsClient* const sessions_client_;
+
   SyncedSessionTracker session_tracker_;
 
   base::WeakPtrFactory<SessionStore> weak_ptr_factory_{this};

components/sync_sessions/session_store_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/cancelable_callback.h"
 #include "base/run_loop.h"
 #include "base/test/mock_callback.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/sync/base/hash_util.h"
@@ -22,6 +23,7 @@
 #include "components/sync_device_info/local_device_info_util.h"
 #include "components/sync_sessions/mock_sync_sessions_client.h"
 #include "components/sync_sessions/session_sync_prefs.h"
+#include "components/sync_sessions/switches.h"
 #include "components/sync_sessions/test_matchers.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -185,7 +187,11 @@ class SessionStoreOpenTest : public ::testing::Test {
 };
 
 TEST_F(SessionStoreOpenTest, ShouldCreateStore) {
-  ASSERT_THAT(session_sync_prefs_.GetSyncSessionsGUID(), IsEmpty());
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(
+      switches::kSyncUseCacheGuidAsSyncSessionTag);
+
+  ASSERT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(), IsEmpty());
 
   MockOpenCallback completion;
   EXPECT_CALL(completion, Run(NoModelError(), /*store=*/NotNull(),
@@ -196,13 +202,36 @@ TEST_F(SessionStoreOpenTest, ShouldCreateStore) {
   ASSERT_THAT(completion.GetResult(), NotNull());
   EXPECT_THAT(completion.GetResult()->local_session_info().client_name,
               Eq(syncer::GetPersonalizableDeviceNameBlocking()));
-  EXPECT_THAT(session_sync_prefs_.GetSyncSessionsGUID(),
+  EXPECT_THAT(completion.GetResult()->local_session_info().session_tag,
+              Eq(std::string("session_sync") + kCacheGuid));
+  EXPECT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(),
               Eq(std::string("session_sync") + kCacheGuid));
 }
 
+TEST_F(SessionStoreOpenTest, ShouldCreateStoreWithoutTagPrefix) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      switches::kSyncUseCacheGuidAsSyncSessionTag);
+
+  ASSERT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(), IsEmpty());
+
+  MockOpenCallback completion;
+  EXPECT_CALL(completion, Run(NoModelError(), /*store=*/NotNull(),
+                              MetadataBatchContains(_, IsEmpty())));
+  SessionStore::Open(kCacheGuid, mock_sync_sessions_client_.get(),
+                     completion.Get());
+  completion.Wait();
+  ASSERT_THAT(completion.GetResult(), NotNull());
+  EXPECT_THAT(completion.GetResult()->local_session_info().client_name,
+              Eq(syncer::GetPersonalizableDeviceNameBlocking()));
+  EXPECT_THAT(completion.GetResult()->local_session_info().session_tag,
+              Eq(kCacheGuid));
+  EXPECT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(), IsEmpty());
+}
+
 TEST_F(SessionStoreOpenTest, ShouldReadSessionsGuidFromPrefs) {
   const std::string kCachedGuid = "cachedguid1";
-  session_sync_prefs_.SetSyncSessionsGUID(kCachedGuid);
+  session_sync_prefs_.SetLegacySyncSessionsGUID(kCachedGuid);
 
   NiceMock<MockOpenCallback> completion;
   SessionStore::Open(kCacheGuid, mock_sync_sessions_client_.get(),
@@ -211,7 +240,7 @@ TEST_F(SessionStoreOpenTest, ShouldReadSessionsGuidFromPrefs) {
   ASSERT_THAT(completion.GetResult(), NotNull());
   EXPECT_THAT(completion.GetResult()->local_session_info().session_tag,
               Eq(kCachedGuid));
-  EXPECT_THAT(session_sync_prefs_.GetSyncSessionsGUID(), Eq(kCachedGuid));
+  EXPECT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(), Eq(kCachedGuid));
 }
 
 TEST_F(SessionStoreOpenTest, ShouldNotUseClientIfCancelled) {
@@ -257,7 +286,7 @@ class SessionStoreTest : public SessionStoreOpenTest {
   const std::string kLocalSessionTag = "localsessiontag";
 
   SessionStoreTest() {
-    session_sync_prefs_.SetSyncSessionsGUID(kLocalSessionTag);
+    session_sync_prefs_.SetLegacySyncSessionsGUID(kLocalSessionTag);
     session_store_ = CreateSessionStore();
   }
 
@@ -276,6 +305,16 @@ class SessionStoreTest : public SessionStoreOpenTest {
   std::unique_ptr<SessionStore> session_store_;
 };
 
+TEST_F(SessionStoreTest, ShouldClearSessionsGuidFromPrefs) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      switches::kSyncUseCacheGuidAsSyncSessionTag);
+  ASSERT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(),
+              Eq(kLocalSessionTag));
+  session_store()->DeleteAllDataAndMetadata();
+  EXPECT_THAT(session_sync_prefs_.GetLegacySyncSessionsGUID(), IsEmpty());
+}
+
 TEST_F(SessionStoreTest, ShouldCreateLocalSession) {
   const std::string header_storage_key =
       SessionStore::GetHeaderStorageKey(kLocalSessionTag);

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -177,7 +177,7 @@ class SessionSyncBridgeTest : public ::testing::Test {
     ON_CALL(mock_sync_sessions_client_, GetLocalSessionEventRouter())
         .WillByDefault(Return(window_getter_.router()));
 
-    session_sync_prefs_.SetSyncSessionsGUID(kLocalSessionTag);
+    session_sync_prefs_.SetLegacySyncSessionsGUID(kLocalSessionTag);
 
     // Even if we use NiceMock, let's be strict about errors and let tests
     // explicitly list them.

components/sync_sessions/session_sync_prefs.cc

@@ -10,15 +10,15 @@
 namespace sync_sessions {
 namespace {
 
-// The GUID session sync will use to identify this client, even across sync
-// disable/enable events.
-const char kSyncSessionsGUID[] = "sync.session_sync_guid";
+// Legacy GUID to identify this client, no longer newly populated by modern
+// clients but honored if present.
+const char kLegacySyncSessionsGUID[] = "sync.session_sync_guid";
 
 }  // namespace
 
 // static
 void SessionSyncPrefs::RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterStringPref(kSyncSessionsGUID, std::string());
+  registry->RegisterStringPref(kLegacySyncSessionsGUID, std::string());
 }
 
 SessionSyncPrefs::SessionSyncPrefs(PrefService* pref_service)
@@ -28,12 +28,16 @@ SessionSyncPrefs::SessionSyncPrefs(PrefService* pref_service)
 
 SessionSyncPrefs::~SessionSyncPrefs() {}
 
-std::string SessionSyncPrefs::GetSyncSessionsGUID() const {
-  return pref_service_->GetString(kSyncSessionsGUID);
+std::string SessionSyncPrefs::GetLegacySyncSessionsGUID() const {
+  return pref_service_->GetString(kLegacySyncSessionsGUID);
 }
 
-void SessionSyncPrefs::SetSyncSessionsGUID(const std::string& guid) {
-  pref_service_->SetString(kSyncSessionsGUID, guid);
+void SessionSyncPrefs::SetLegacySyncSessionsGUID(const std::string& guid) {
+  pref_service_->SetString(kLegacySyncSessionsGUID, guid);
+}
+
+void SessionSyncPrefs::ClearLegacySyncSessionsGUID() {
+  pref_service_->ClearPref(kLegacySyncSessionsGUID);
 }
 
 }  // namespace sync_sessions

components/sync_sessions/session_sync_prefs.h

@@ -22,8 +22,9 @@ class SessionSyncPrefs {
   explicit SessionSyncPrefs(PrefService* pref_service);
   ~SessionSyncPrefs();
 
-  std::string GetSyncSessionsGUID() const;
-  void SetSyncSessionsGUID(const std::string& guid);
+  std::string GetLegacySyncSessionsGUID() const;
+  void SetLegacySyncSessionsGUID(const std::string& guid);
+  void ClearLegacySyncSessionsGUID();
 
  private:
   PrefService* const pref_service_;

components/sync_sessions/session_sync_service.h

@@ -51,7 +51,9 @@ class SessionSyncService : public KeyedService {
   virtual void ProxyTabsStateChanged(
       syncer::DataTypeController::State state) = 0;
 
-  // Used on Android only, to override the machine tag.
+  // Used on Android only, to override the session tag. This call may be ignored
+  // depending on feature toggles.
+  // TODO(crbug.com/1159455): Delete code when the feature toggle gets deleted.
   virtual void SetSyncSessionsGUID(const std::string& guid) = 0;
 
  private:

components/sync_sessions/session_sync_service_impl.cc

@@ -8,10 +8,12 @@
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
+#include "base/feature_list.h"
 #include "components/sync/base/report_unrecoverable_error.h"
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
 #include "components/sync_sessions/session_sync_bridge.h"
 #include "components/sync_sessions/session_sync_prefs.h"
+#include "components/sync_sessions/switches.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 
 namespace sync_sessions {
@@ -65,7 +67,12 @@ void SessionSyncServiceImpl::ProxyTabsStateChanged(
 }
 
 void SessionSyncServiceImpl::SetSyncSessionsGUID(const std::string& guid) {
-  sessions_client_->GetSessionSyncPrefs()->SetSyncSessionsGUID(guid);
+  if (base::FeatureList::IsEnabled(
+          switches::kSyncUseCacheGuidAsSyncSessionTag)) {
+    return;
+  }
+
+  sessions_client_->GetSessionSyncPrefs()->SetLegacySyncSessionsGUID(guid);
 }
 
 OpenTabsUIDelegate*

components/sync_sessions/switches.cc

@@ -11,4 +11,7 @@ namespace switches {
 const base::Feature kSyncConsiderEmptyWindowsSyncable{
     "SyncConsiderEmptyWindowsSyncable", base::FEATURE_ENABLED_BY_DEFAULT};
 
+const base::Feature kSyncUseCacheGuidAsSyncSessionTag{
+    "SyncUseCacheGuidAsSyncSessionTag", base::FEATURE_ENABLED_BY_DEFAULT};
+
 }  // namespace switches

components/sync_sessions/switches.h

@@ -11,6 +11,8 @@ namespace switches {
 
 extern const base::Feature kSyncConsiderEmptyWindowsSyncable;
 
+extern const base::Feature kSyncUseCacheGuidAsSyncSessionTag;
+
 }  // namespace switches
 
 #endif  // COMPONENTS_SYNC_SESSIONS_SWITCHES_H_

components/sync_sessions/sync_sessions_client.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_SYNC_SESSIONS_SYNC_SESSIONS_CLIENT_H_
 
 #include <memory>
+#include <string>
 
 #include "base/macros.h"
 #include "components/sync/model/model_type_store.h"
@@ -39,6 +40,10 @@ class SyncSessionsClient {
   // componentized.
   virtual bool ShouldSyncURL(const GURL& url) const = 0;
 
+  // Returns if the provided |cache_guid| is the local device's current cache\
+  // GUID or is known to have been used in the past as local device GUID.
+  virtual bool IsRecentLocalCacheGuid(const std::string& cache_guid) const = 0;
+
   // Returns the SyncedWindowDelegatesGetter for this client.
   virtual SyncedWindowDelegatesGetter* GetSyncedWindowDelegatesGetter() = 0;
 

components/sync_sessions/synced_session_tracker.cc

@@ -326,7 +326,13 @@ std::vector<const SyncedSession*> SyncedSessionTracker::LookupSessions(
     if (lookup == PRESENTABLE && !IsPresentable(sessions_client_, session)) {
       continue;
     }
-    if (exclude_local_session && session_pair.first == local_session_tag_) {
+    // The comparison against |local_session_tag_| deals with the currently
+    // active sync session (cache GUID or legacy session tag) but in addition
+    // IsRecentLocalCacheGuid() is used to filter out older values of the
+    // local cache GUID.
+    if (exclude_local_session &&
+        (session_pair.first == local_session_tag_ ||
+         sessions_client_->IsRecentLocalCacheGuid(session_pair.first))) {
       continue;
     }
     sessions.push_back(&session);

components/sync_sessions/synced_session_tracker_unittest.cc

@@ -243,6 +243,7 @@ TEST_F(SyncedSessionTrackerTest, LookupAllForeignSessions) {
   tab->navigations.push_back(
       sessions::SerializedNavigationEntryTestHelper::CreateNavigationForTest());
   tab->navigations.back().set_virtual_url(GURL(kInvalidUrl));
+
   // Only the session with a valid window and tab gets returned.
   EXPECT_THAT(
       tracker_.LookupAllForeignSessions(SyncedSessionTracker::PRESENTABLE),
@@ -250,6 +251,15 @@ TEST_F(SyncedSessionTrackerTest, LookupAllForeignSessions) {
   EXPECT_THAT(tracker_.LookupAllForeignSessions(SyncedSessionTracker::RAW),
               ElementsAre(HasSessionTag(kTag), HasSessionTag(kTag2),
                           HasSessionTag(kTag3)));
+
+  // Annotate kTag as local session.
+  ON_CALL(sessions_client_, IsRecentLocalCacheGuid(kTag))
+      .WillByDefault(Return(true));
+  EXPECT_THAT(
+      tracker_.LookupAllForeignSessions(SyncedSessionTracker::PRESENTABLE),
+      IsEmpty());
+  EXPECT_THAT(tracker_.LookupAllForeignSessions(SyncedSessionTracker::RAW),
+              ElementsAre(HasSessionTag(kTag2), HasSessionTag(kTag3)));
 }
 
 TEST_F(SyncedSessionTrackerTest, LookupSessionWindows) {

ios/chrome/browser/sync/session_sync_service_factory.mm

@@ -15,6 +15,8 @@
 #include "components/keyed_service/ios/browser_state_dependency_manager.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/model/model_type_store_service.h"
+#include "components/sync_device_info/device_info_sync_service.h"
+#include "components/sync_device_info/device_info_tracker.h"
 #include "components/sync_sessions/local_session_event_router.h"
 #include "components/sync_sessions/session_sync_prefs.h"
 #include "components/sync_sessions/session_sync_service_impl.h"
@@ -24,6 +26,7 @@
 #include "ios/chrome/browser/chrome_url_constants.h"
 #include "ios/chrome/browser/favicon/favicon_service_factory.h"
 #include "ios/chrome/browser/history/history_service_factory.h"
+#include "ios/chrome/browser/sync/device_info_sync_service_factory.h"
 #include "ios/chrome/browser/sync/glue/sync_start_util.h"
 #include "ios/chrome/browser/sync/model_type_store_service_factory.h"
 #import "ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.h"
@@ -95,6 +98,12 @@ class SyncSessionsClientImpl : public sync_sessions::SyncSessionsClient {
     return ShouldSyncURLImpl(url);
   }
 
+  bool IsRecentLocalCacheGuid(const std::string& cache_guid) const override {
+    return DeviceInfoSyncServiceFactory::GetForBrowserState(browser_state_)
+        ->GetDeviceInfoTracker()
+        ->IsRecentLocalCacheGuid(cache_guid);
+  }
+
   sync_sessions::SyncedWindowDelegatesGetter* GetSyncedWindowDelegatesGetter()
       override {
     return window_delegates_getter_.get();