Enable USS version of session sync by default

The feature has been running for a while on 50% canary and dev
with only one known issue which is already fixed on trunk.

Hence, let's enable it by default, which also requires updating
a few tests.

The switch exercises a more modern implementation of sessions
sync that should be identical from the users perspective, besides
the performance improvements (most notably, memory consumption
estimated to be reduced by a factor of 70% according to metric
Sync.ModelTypeMemoryKB.SESSION), or ~350 KB for the 95th
percentile.

Bug: 681921
Change-Id: Ida3e676a534d8c3ced174092d5498a793860020b
Reviewed-on: https://chromium-review.googlesource.com/1086945
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568045}

chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/OpenTabsTest.java

@@ -76,13 +76,19 @@ public class OpenTabsTest {
 
     // A container to store OpenTabs information for data verification.
     private static class OpenTabs {
-        public final String headerId;
-        public final List<String> tabIds;
-        public final List<String> urls;
-
-        private OpenTabs(String headerId, List<String> tabIds, List<String> urls) {
-            this.headerId = headerId;
-            this.tabIds = tabIds;
+        public final String headerServerId;
+        public final String headerClientTagHash;
+        public final ArrayList<String> tabServerIds;
+        public final ArrayList<String> tabClientTagHashes;
+        public final ArrayList<String> urls;
+
+        private OpenTabs(String headerServerId, String headerClientTagHash,
+                ArrayList<String> tabServerIds, ArrayList<String> tabClientTagHashes,
+                ArrayList<String> urls) {
+            this.headerServerId = headerServerId;
+            this.headerClientTagHash = headerClientTagHash;
+            this.tabServerIds = tabServerIds;
+            this.tabClientTagHashes = tabClientTagHashes;
             this.urls = urls;
         }
     }
@@ -236,8 +242,6 @@ public class OpenTabsTest {
         mSyncTestRule.getFakeServerHelper().injectUniqueClientEntity(tag, header);
         for (int i = 0; i < urls.length; i++) {
             EntitySpecifics tab = makeTabEntity(tag, urls[i], i);
-            // It is critical that the name here is "<tag> <tabNodeId>", otherwise sync crashes
-            // when it tries to sync due to the use of TabIdToTag in sessions_sync_manager.cc.
             mSyncTestRule.getFakeServerHelper().injectUniqueClientEntity(tag + " " + i, tab);
         }
     }
@@ -282,9 +286,11 @@ public class OpenTabsTest {
 
     private void deleteServerTabsForClient(String clientName) throws JSONException {
         OpenTabs openTabs = getLocalTabsForClient(clientName);
-        mSyncTestRule.getFakeServerHelper().deleteEntity(openTabs.headerId);
-        for (String tabId : openTabs.tabIds) {
-            mSyncTestRule.getFakeServerHelper().deleteEntity(tabId);
+        mSyncTestRule.getFakeServerHelper().deleteEntity(
+                openTabs.headerServerId, openTabs.headerClientTagHash);
+        for (int i = 0; i < openTabs.tabServerIds.size(); i++) {
+            mSyncTestRule.getFakeServerHelper().deleteEntity(
+                    openTabs.tabServerIds.get(i), openTabs.tabClientTagHashes.get(i));
         }
     }
 
@@ -343,11 +349,14 @@ public class OpenTabsTest {
 
     private static class HeaderInfo {
         public final String sessionTag;
-        public final String headerId;
+        public final String headerServerId;
+        public final String headerClientTagHash;
         public final List<String> tabIds;
-        public HeaderInfo(String sessionTag, String headerId, List<String> tabIds) {
+        public HeaderInfo(String sessionTag, String headerServerId, String headerClientTagHash,
+                List<String> tabIds) {
             this.sessionTag = sessionTag;
-            this.headerId = headerId;
+            this.headerServerId = headerServerId;
+            this.headerClientTagHash = headerClientTagHash;
             this.tabIds = tabIds;
         }
     }
@@ -357,35 +366,43 @@ public class OpenTabsTest {
         List<Pair<String, JSONObject>> tabEntities =
                 SyncTestUtil.getLocalData(mSyncTestRule.getTargetContext(), OPEN_TABS_TYPE);
         // Output lists.
-        List<String> urls = new ArrayList<>();
-        List<String> tabEntityIds = new ArrayList<>();
+        ArrayList<String> urls = new ArrayList<>();
+        ArrayList<String> tabServerIds = new ArrayList<>();
+        ArrayList<String> tabClientTagHashes = new ArrayList<>();
         HeaderInfo info = findHeaderInfoForClient(clientName, tabEntities);
         if (info.sessionTag == null) {
             // No client was found. Here we still want to return an empty list of urls.
-            return new OpenTabs("", tabEntityIds, urls);
+            return new OpenTabs("", "", tabServerIds, tabClientTagHashes, urls);
         }
         Map<String, String> tabIdsToUrls = new HashMap<>();
-        Map<String, String> tabIdsToEntityIds = new HashMap<>();
-        findTabMappings(info.sessionTag, tabEntities, tabIdsToUrls, tabIdsToEntityIds);
+        Map<String, String> tabIdsToServerIds = new HashMap<>();
+        Map<String, String> tabIdsToClientTagHashes = new HashMap<>();
+        findTabMappings(info.sessionTag, tabEntities, tabIdsToUrls, tabIdsToServerIds,
+                tabIdsToClientTagHashes);
         // Convert the tabId list to the url list.
         for (String tabId : info.tabIds) {
             urls.add(tabIdsToUrls.get(tabId));
-            tabEntityIds.add(tabIdsToEntityIds.get(tabId));
+            tabServerIds.add(tabIdsToServerIds.get(tabId));
+            tabClientTagHashes.add(tabIdsToClientTagHashes.get(tabId));
         }
-        return new OpenTabs(info.headerId, tabEntityIds, urls);
+        return new OpenTabs(info.headerServerId, info.headerClientTagHash, tabServerIds,
+                tabClientTagHashes, urls);
     }
 
     // Find the header entity for clientName and extract its sessionTag and tabId list.
     private HeaderInfo findHeaderInfoForClient(
             String clientName, List<Pair<String, JSONObject>> tabEntities) throws JSONException {
         String sessionTag = null;
-        String headerId = null;
+        String headerServerId = null;
+        String headerClientTagHash = null;
         List<String> tabIds = new ArrayList<>();
         for (Pair<String, JSONObject> tabEntity : tabEntities) {
             JSONObject header = tabEntity.second.optJSONObject("header");
             if (header != null && header.getString("client_name").equals(clientName)) {
                 sessionTag = tabEntity.second.getString("session_tag");
-                headerId = tabEntity.first;
+                headerClientTagHash =
+                        tabEntity.second.optJSONObject("metadata").getString("client_tag_hash");
+                headerServerId = tabEntity.first;
                 JSONArray windows = header.getJSONArray("window");
                 if (windows.length() == 0) {
                     // The client was found but there are no tabs.
@@ -399,24 +416,26 @@ public class OpenTabsTest {
                 break;
             }
         }
-        return new HeaderInfo(sessionTag, headerId, tabIds);
+        return new HeaderInfo(sessionTag, headerServerId, headerClientTagHash, tabIds);
     }
 
     // Find the associated tabs and record their tabId -> url and entityId mappings.
     private void findTabMappings(String sessionTag, List<Pair<String, JSONObject>> tabEntities,
             // Populating these maps is the output of this function.
-            Map<String, String> tabIdsToUrls, Map<String, String> tabIdsToEntityIds)
-            throws JSONException {
+            Map<String, String> tabIdsToUrls, Map<String, String> tabIdsToServerIds,
+            Map<String, String> tabIdsToClientTagHashes) throws JSONException {
         for (Pair<String, JSONObject> tabEntity : tabEntities) {
             JSONObject json = tabEntity.second;
             if (json.has("tab") && json.getString("session_tag").equals(sessionTag)) {
+                String clientTagHash = json.optJSONObject("metadata").getString("client_tag_hash");
                 JSONObject tab = json.getJSONObject("tab");
                 int i = tab.getInt("current_navigation_index");
                 String tabId = tab.getString("tab_id");
                 String url = tab.getJSONArray("navigation")
                         .getJSONObject(i).getString("virtual_url");
                 tabIdsToUrls.put(tabId, url);
-                tabIdsToEntityIds.put(tabId, tabEntity.first);
+                tabIdsToServerIds.put(tabId, tabEntity.first);
+                tabIdsToClientTagHashes.put(tabId, clientTagHash);
             }
         }
     }

chrome/browser/extensions/api/sessions/sessions_apitest.cc

@@ -13,6 +13,7 @@
 #include "base/path_service.h"
 #include "base/strings/pattern.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/bind_test_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/api/sessions/sessions_api.h"
 #include "chrome/browser/extensions/api/tabs/tabs_api.h"
@@ -28,11 +29,12 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/browser_sync/profile_sync_service_mock.h"
+#include "components/sync/base/hash_util.h"
 #include "components/sync/device_info/local_device_info_provider_mock.h"
 #include "components/sync/driver/sync_api_component_factory_mock.h"
-#include "components/sync/model/fake_sync_change_processor.h"
-#include "components/sync/model/sync_error_factory_mock.h"
-#include "components/sync_sessions/sessions_sync_manager.h"
+#include "components/sync/engine/data_type_activation_request.h"
+#include "components/sync/test/engine/mock_model_type_worker.h"
+#include "components/sync_sessions/session_store.h"
 #include "extensions/browser/api_test_utils.h"
 #include "extensions/common/extension_builder.h"
 
@@ -74,10 +76,11 @@ void BuildWindowSpecifics(int window_id,
 }
 
 void BuildTabSpecifics(const std::string& tag,
-                       int tab_id,
+                       SessionID::id_type tab_id,
+                       int tab_node_id,
                        sync_pb::SessionSpecifics* tab_base) {
   tab_base->set_session_tag(tag);
-  tab_base->set_tab_node_id(0);
+  tab_base->set_tab_node_id(tab_node_id);
   sync_pb::SessionTab* tab = tab_base->mutable_tab();
   tab->set_tab_id(tab_id);
   tab->set_tab_visual_index(1);
@@ -141,6 +144,11 @@ testing::AssertionResult CheckSessionModels(const base::ListValue& devices,
   return testing::AssertionSuccess();
 }
 
+std::string TagHashFromSpecifics(const sync_pb::SessionSpecifics& specifics) {
+  return syncer::GenerateSyncableHash(
+      syncer::SESSIONS, sync_sessions::SessionStore::GetClientTag(specifics));
+}
+
 }  // namespace
 
 class ExtensionSessionsTest : public InProcessBrowserTest {
@@ -249,42 +257,56 @@ void ExtensionSessionsTest::CreateTestExtension() {
 }
 
 void ExtensionSessionsTest::CreateSessionModels() {
+  syncer::DataTypeActivationRequest request;
+  request.error_handler = base::DoNothing();
+  request.cache_guid = "TestCacheGuid";
+  request.authenticated_account_id = "SomeAccountId";
+
+  std::unique_ptr<syncer::DataTypeActivationResponse> activation_response;
+  base::RunLoop loop;
+  ProfileSyncServiceFactory::GetForProfile(browser_->profile())
+      ->GetSessionSyncControllerDelegateOnUIThread()
+      ->OnSyncStarting(
+          request, base::BindLambdaForTesting(
+                       [&](std::unique_ptr<syncer::DataTypeActivationResponse>
+                               response) {
+                         activation_response = std::move(response);
+                         loop.Quit();
+                       }));
+  loop.Run();
+
+  syncer::MockModelTypeWorker worker(sync_pb::ModelTypeState(),
+                                     activation_response->type_processor.get());
+
   syncer::SyncDataList initial_data;
   for (size_t index = 0; index < arraysize(kSessionTags); ++index) {
     // Fill an instance of session specifics with a foreign session's data.
-    sync_pb::SessionSpecifics meta;
-    BuildSessionSpecifics(kSessionTags[index], &meta);
+    sync_pb::EntitySpecifics header_entity;
+    BuildSessionSpecifics(kSessionTags[index], header_entity.mutable_session());
     std::vector<SessionID::id_type> tab_list(kTabIDs,
                                              kTabIDs + arraysize(kTabIDs));
-    BuildWindowSpecifics(index, tab_list, &meta);
+    BuildWindowSpecifics(index, tab_list, header_entity.mutable_session());
     std::vector<sync_pb::SessionSpecifics> tabs(tab_list.size());
     for (size_t i = 0; i < tab_list.size(); ++i) {
-      BuildTabSpecifics(kSessionTags[index], tab_list[i], &tabs[i]);
+      BuildTabSpecifics(kSessionTags[index], tab_list[i], /*tab_node_id=*/i,
+                        &tabs[i]);
     }
 
-    sync_pb::EntitySpecifics entity;
-    entity.mutable_session()->CopyFrom(meta);
-    initial_data.push_back(syncer::SyncData::CreateRemoteData(
-        1, entity, base::Time(),
-        sync_sessions::SessionsSyncManager::TagHashFromSpecifics(
-            entity.session())));
+    worker.UpdateFromServer(TagHashFromSpecifics(header_entity.session()),
+                            header_entity);
+
     for (size_t i = 0; i < tabs.size(); i++) {
-      sync_pb::EntitySpecifics entity;
-      entity.mutable_session()->CopyFrom(tabs[i]);
-      initial_data.push_back(syncer::SyncData::CreateRemoteData(
-          i + 2, entity, base::Time(),
-          sync_sessions::SessionsSyncManager::TagHashFromSpecifics(
-              entity.session())));
+      sync_pb::EntitySpecifics tab_entity;
+      *tab_entity.mutable_session() = tabs[i];
+      worker.UpdateFromServer(TagHashFromSpecifics(tab_entity.session()),
+                              tab_entity);
     }
   }
 
-  ProfileSyncServiceFactory::GetForProfile(browser_->profile())
-      ->GetSessionsSyncableService()
-      ->MergeDataAndStartSyncing(syncer::SESSIONS, initial_data,
-                                 std::unique_ptr<syncer::SyncChangeProcessor>(
-                                     new syncer::FakeSyncChangeProcessor()),
-                                 std::unique_ptr<syncer::SyncErrorFactory>(
-                                     new syncer::SyncErrorFactoryMock()));
+  // Let the processor receive and honor all updates, which requires running
+  // the runloop because there is a ModelTypeProcessorProxy in between, posting
+  // tasks.
+  base::RunLoop().RunUntilIdle();
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionSessionsTest, GetDevices) {

chrome/browser/sync/test/integration/enable_disable_test.cc

@@ -53,6 +53,7 @@ ModelTypeSet UnifiedSyncServiceTypes() {
   ModelTypeSet set;
   set.Put(syncer::AUTOFILL);
   set.Put(syncer::DEVICE_INFO);
+  set.Put(syncer::SESSIONS);
   set.Put(syncer::TYPED_URLS);
   // PRINTERS was the first USS type, and should precede all other USS types.
   // All new types should be USS. This logic is fragile to reordering ModelType.

components/sync/driver/sync_driver_switches.cc

@@ -68,6 +68,6 @@ const base::Feature kSyncUSSBookmarks{"SyncUSSBookmarks",
 
 // Enable USS implementation of sessions.
 const base::Feature kSyncUSSSessions{"SyncUSSSessions",
-                                     base::FEATURE_DISABLED_BY_DEFAULT};
+                                     base::FEATURE_ENABLED_BY_DEFAULT};
 
 }  // namespace switches

components/sync/test/fake_server/android/fake_server_helper_android.cc

@@ -12,7 +12,9 @@
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/logging.h"
+#include "base/time/time.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/base/time.h"
 #include "components/sync/engine/net/network_resources.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/test/fake_server/bookmark_entity_builder.h"
@@ -145,10 +147,11 @@ void FakeServerHelperAndroid::InjectUniqueClientEntity(
   DeserializeEntitySpecifics(env, serialized_entity_specifics,
                              &entity_specifics);
 
+  int64_t now = syncer::TimeToProtoTime(base::Time::Now());
   fake_server_ptr->InjectEntity(
       syncer::PersistentUniqueClientEntity::CreateFromEntitySpecifics(
           base::android::ConvertJavaStringToUTF8(env, name), entity_specifics,
-          12345, 12345));
+          /*creation_time=*/now, /*last_modified_time=*/now));
 }
 
 void FakeServerHelperAndroid::ModifyEntitySpecifics(