[Sync] Stop accessing BrowserContextKeyedServiceFactory on non-UI thread.

chrome/browser/sync/chrome_sync_client.cc

Index: chrome/browser/sync/chrome_sync_client.cc
diff --git a/chrome/browser/sync/chrome_sync_client.cc b/chrome/browser/sync/chrome_sync_client.cc
index 16c7aad4bd1386ecf3f7cf4a0b77531dce8cb03e..31e498c48bfe89da500b6b9b16c7271bc411a7fb 100644
--- a/chrome/browser/sync/chrome_sync_client.cc
+++ b/chrome/browser/sync/chrome_sync_client.cc
@@ -120,21 +120,60 @@
 #include "components/sync_wifi/wifi_credential_syncable_service_factory.h"
 #endif
 
-using content::BrowserThread;
+using autofill::AutocompleteSyncableService;

[pavely, 2017/03/27 19:38:25]

nit: autofill related classes are only mentioned in this file once. Does it make
sense to add them here? One issue is that when name usage gets removed from
file, "using ..." directive often gets left behind.

[skym, 2017/03/27 21:45:26]

Done.

+using autofill::AutofillProfileSyncableService;
+using autofill::AutofillWalletMetadataSyncableService;
+using autofill::AutofillWalletSyncableService;
+using base::Callback;
+using base::WeakPtr;
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 using browser_sync::ExtensionDataTypeController;
 using browser_sync::ExtensionSettingDataTypeController;
 #endif
 using browser_sync::SearchEngineDataTypeController;
+using content::BrowserThread;
 using syncer::AsyncDirectoryTypeController;
+using syncer::SyncableService;
+
+using ServiceProvider = syncer::SyncClient::ServiceProvider;
 
 namespace browser_sync {
 
 namespace {
+
 #if defined(OS_WIN)
 const base::FilePath::CharType kLoopbackServerBackendFilename[] =
     FILE_PATH_LITERAL("profile.pb");
 #endif
+
+template <typename T>

[pavely, 2017/03/27 19:38:25]

Could you add comment explaining how these functions are used. The goal is to
explain to new datatype developer which function to use. Call out two typical
usage patterns (getting reference to SyncableService on UI thread vs getting
some intermediate object on UI thread and then getting SyncableService out of it
on model thread).

[skym, 2017/03/27 21:45:26]

Done.

+T Trampoline(T arg) {
+  return arg;
+}
+
+template <typename T>
+Callback<T()> WrapInCallback(T arg) {
+  return base::Bind(&Trampoline<T>, arg);
+}
+
+WeakPtr<SyncableService> ServiceAsWeakPtr(SyncableService* ptr) {
+  return ptr ? ptr->AsWeakPtr() : WeakPtr<SyncableService>();
+}
+
+ServiceProvider WrapInProvider(SyncableService* service) {
+  return WrapInCallback(ServiceAsWeakPtr(service));
+}
+
+template <typename T>
+WeakPtr<SyncableService> CallbackResultAsWeakPtr(Callback<T()> callback) {
+  return ServiceAsWeakPtr(callback.Run());
+}
+
+template <typename T>
+ServiceProvider WrapInWeakPtrCallback(Callback<T()> callback) {
+  return base::Bind(&CallbackResultAsWeakPtr<T>, callback);
+}
+
 }  // namespace
 
 // Chrome implementation of SyncSessionsClient. Needs to be in a separate class
@@ -299,7 +338,7 @@ favicon::FaviconService* ChromeSyncClient::GetFaviconService() {
 history::HistoryService* ChromeSyncClient::GetHistoryService() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return HistoryServiceFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
+      profile_, ServiceAccessType::IMPLICIT_ACCESS);
 }
 
 bool ChromeSyncClient::HasPasswordStore() {
@@ -349,144 +388,133 @@ sync_sessions::SyncSessionsClient* ChromeSyncClient::GetSyncSessionsClient() {
   return sync_sessions_client_.get();
 }
 
-base::WeakPtr<syncer::SyncableService>
-ChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
+ServiceProvider ChromeSyncClient::GetSyncableServiceForType(
+    syncer::ModelType type) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!profile_) {  // For tests.
-     return base::WeakPtr<syncer::SyncableService>();
+    return WrapInProvider(nullptr);
   }
   switch (type) {
     case syncer::DEVICE_INFO:
-      return ProfileSyncServiceFactory::GetForProfile(profile_)
-          ->GetDeviceInfoSyncableService()
-          ->AsWeakPtr();
+      return WrapInProvider(ProfileSyncServiceFactory::GetForProfile(profile_)
+                                ->GetDeviceInfoSyncableService());
     case syncer::PREFERENCES:
-      return PrefServiceSyncableFromProfile(profile_)
-          ->GetSyncableService(syncer::PREFERENCES)
-          ->AsWeakPtr();
     case syncer::PRIORITY_PREFERENCES:
-      return PrefServiceSyncableFromProfile(profile_)
-          ->GetSyncableService(syncer::PRIORITY_PREFERENCES)
-          ->AsWeakPtr();
+      return WrapInProvider(
+          PrefServiceSyncableFromProfile(profile_)->GetSyncableService(type));
     case syncer::AUTOFILL:
     case syncer::AUTOFILL_PROFILE:
     case syncer::AUTOFILL_WALLET_DATA:
     case syncer::AUTOFILL_WALLET_METADATA: {
-      if (!web_data_service_)
-        return base::WeakPtr<syncer::SyncableService>();
-      if (type == syncer::AUTOFILL) {
-        return autofill::AutocompleteSyncableService::FromWebDataService(
-            web_data_service_.get())->AsWeakPtr();
+      if (!web_data_service_) {
+        return WrapInProvider(nullptr);
+      } else if (type == syncer::AUTOFILL) {
+        return WrapInWeakPtrCallback(
+            base::Bind(&AutocompleteSyncableService::FromWebDataService,
+                       base::RetainedRef(web_data_service_)));
       } else if (type == syncer::AUTOFILL_PROFILE) {
-        return autofill::AutofillProfileSyncableService::FromWebDataService(
-            web_data_service_.get())->AsWeakPtr();
+        return WrapInWeakPtrCallback(
+            base::Bind(&AutofillProfileSyncableService::FromWebDataService,
+                       base::RetainedRef(web_data_service_)));
       } else if (type == syncer::AUTOFILL_WALLET_METADATA) {
-        return autofill::AutofillWalletMetadataSyncableService::
-            FromWebDataService(web_data_service_.get())->AsWeakPtr();
+        return WrapInWeakPtrCallback(base::Bind(
+            &AutofillWalletMetadataSyncableService::FromWebDataService,
+            base::RetainedRef(web_data_service_)));
+      } else {
+        return WrapInWeakPtrCallback(
+            base::Bind(&AutofillWalletSyncableService::FromWebDataService,
+                       base::RetainedRef(web_data_service_)));
       }
-      return autofill::AutofillWalletSyncableService::FromWebDataService(
-          web_data_service_.get())->AsWeakPtr();
     }
     case syncer::SEARCH_ENGINES:
-      return TemplateURLServiceFactory::GetForProfile(profile_)->AsWeakPtr();
+      return WrapInProvider(TemplateURLServiceFactory::GetForProfile(profile_));
 #if BUILDFLAG(ENABLE_EXTENSIONS)
     case syncer::APPS:
     case syncer::EXTENSIONS:
-      return ExtensionSyncService::Get(profile_)->AsWeakPtr();
+      return WrapInProvider(ExtensionSyncService::Get(profile_));
     case syncer::APP_SETTINGS:
     case syncer::EXTENSION_SETTINGS:
-      return extensions::settings_sync_util::GetSyncableService(profile_, type)
-          ->AsWeakPtr();
+      return extensions::settings_sync_util::GetSyncableServiceProvider(
+          profile_, type);
 #endif
 #if BUILDFLAG(ENABLE_APP_LIST)
     case syncer::APP_LIST:
-      return app_list::AppListSyncableServiceFactory::GetForProfile(profile_)->
-          AsWeakPtr();
+      return WrapInProvider(
+          app_list::AppListSyncableServiceFactory::GetForProfile(profile_));
 #endif
 #if !defined(OS_ANDROID)
     case syncer::THEMES:
-      return ThemeServiceFactory::GetForProfile(profile_)->
-          GetThemeSyncableService()->AsWeakPtr();
+      return WrapInProvider(ThemeServiceFactory::GetForProfile(profile_)
+                                ->GetThemeSyncableService());
 #endif
-    case syncer::HISTORY_DELETE_DIRECTIVES: {
-      history::HistoryService* history = GetHistoryService();
-      return history ? history->AsWeakPtr()
-                     : base::WeakPtr<history::HistoryService>();
-    }
+    case syncer::HISTORY_DELETE_DIRECTIVES:
+      return WrapInProvider(GetHistoryService());
     case syncer::TYPED_URLS: {
-      // We request history service with explicit access here because this
-      // codepath is executed on backend thread while HistoryServiceFactory
-      // checks preference value in implicit mode and PrefService expectes calls
-      // only from UI thread.
-      history::HistoryService* history = HistoryServiceFactory::GetForProfile(
-          profile_, ServiceAccessType::EXPLICIT_ACCESS);
-      if (!history)
-        return base::WeakPtr<history::TypedUrlSyncableService>();
-      return history->GetTypedUrlSyncableService()->AsWeakPtr();
+      history::HistoryService* history = GetHistoryService();
+      return WrapInProvider(history ? history->GetTypedUrlSyncableService()
+                                    : nullptr);
     }
 #if BUILDFLAG(ENABLE_SPELLCHECK)
     case syncer::DICTIONARY:
-      return SpellcheckServiceFactory::GetForContext(profile_)->
-          GetCustomDictionary()->AsWeakPtr();
+      return WrapInProvider(SpellcheckServiceFactory::GetForContext(profile_)
+                                ->GetCustomDictionary());
 #endif
     case syncer::FAVICON_IMAGES:
-    case syncer::FAVICON_TRACKING: {
-      sync_sessions::FaviconCache* favicons =
-          ProfileSyncServiceFactory::GetForProfile(profile_)->GetFaviconCache();
-      return favicons ? favicons->AsWeakPtr()
-                      : base::WeakPtr<syncer::SyncableService>();
-    }
+    case syncer::FAVICON_TRACKING:
+      return WrapInProvider(ProfileSyncServiceFactory::GetForProfile(profile_)
+                                ->GetFaviconCache());
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
     case syncer::SUPERVISED_USER_SETTINGS:
-      return SupervisedUserSettingsServiceFactory::GetForProfile(profile_)->
-          AsWeakPtr();
+      return WrapInProvider(
+          SupervisedUserSettingsServiceFactory::GetForProfile(profile_));
 #if !defined(OS_ANDROID)
     case syncer::SUPERVISED_USERS:
-      return SupervisedUserSyncServiceFactory::GetForProfile(profile_)->
-          AsWeakPtr();
+      return WrapInProvider(
+          SupervisedUserSyncServiceFactory::GetForProfile(profile_));
     case syncer::SUPERVISED_USER_SHARED_SETTINGS:
-      return SupervisedUserSharedSettingsServiceFactory::GetForBrowserContext(
-          profile_)->AsWeakPtr();
+      return WrapInProvider(
+          SupervisedUserSharedSettingsServiceFactory::GetForBrowserContext(
+              profile_));
 #endif
     case syncer::SUPERVISED_USER_WHITELISTS:
-      return SupervisedUserServiceFactory::GetForProfile(profile_)
-          ->GetWhitelistService()
-          ->AsWeakPtr();
+      return WrapInProvider(
+          SupervisedUserServiceFactory::GetForProfile(profile_)
+              ->GetWhitelistService());
 #endif
     case syncer::ARTICLES: {
-      dom_distiller::DomDistillerService* service =
+      dom_distiller::DomDistillerService* distiller =

[pavely, 2017/03/27 19:38:25]

nit: distiller => distiller_service?

[skym, 2017/03/27 21:45:26]

Done.

           dom_distiller::DomDistillerServiceFactory::GetForBrowserContext(
               profile_);
-      if (service)
-        return service->GetSyncableService()->AsWeakPtr();
-      return base::WeakPtr<syncer::SyncableService>();
-    }
-    case syncer::SESSIONS: {
-      return ProfileSyncServiceFactory::GetForProfile(profile_)->
-          GetSessionsSyncableService()->AsWeakPtr();
+      return WrapInProvider(distiller ? distiller->GetSyncableService()
+                                      : nullptr);
     }
-    case syncer::PASSWORDS: {
+    case syncer::SESSIONS:
+      return WrapInProvider(ProfileSyncServiceFactory::GetForProfile(profile_)
+                                ->GetSessionsSyncableService());
+    case syncer::PASSWORDS:
       return password_store_.get()
-                 ? password_store_->GetPasswordSyncableService()
-                 : base::WeakPtr<syncer::SyncableService>();
-    }
+                 ? base::Bind(&password_manager::PasswordStore::
+                                  GetPasswordSyncableService,
+                              base::RetainedRef(password_store_))
+                 : WrapInProvider(nullptr);
 #if defined(OS_CHROMEOS)
     case syncer::WIFI_CREDENTIALS:
-      return sync_wifi::WifiCredentialSyncableServiceFactory::
-          GetForBrowserContext(profile_)
-              ->AsWeakPtr();
+      return WrapInProvider(
+          sync_wifi::WifiCredentialSyncableServiceFactory::GetForBrowserContext(
+              profile_));
     case syncer::ARC_PACKAGE:
-      return arc::ArcPackageSyncableService::Get(profile_)->AsWeakPtr();
+      return WrapInProvider(arc::ArcPackageSyncableService::Get(profile_));
 #endif
     default:
       // The following datatypes still need to be transitioned to the
       // syncer::SyncableService API:
       // Bookmarks
       NOTREACHED();
-      return base::WeakPtr<syncer::SyncableService>();
+      return WrapInProvider(nullptr);
   }
 }
 
-base::WeakPtr<syncer::ModelTypeSyncBridge>
+WeakPtr<syncer::ModelTypeSyncBridge>
 ChromeSyncClient::GetSyncBridgeForModelType(syncer::ModelType type) {
   switch (type) {
     case syncer::DEVICE_INFO:
@@ -496,7 +524,7 @@ ChromeSyncClient::GetSyncBridgeForModelType(syncer::ModelType type) {
     case syncer::READING_LIST:
       // Reading List is only supported on iOS at the moment.
       NOTREACHED();
-      return base::WeakPtr<syncer::ModelTypeSyncBridge>();
+      return WeakPtr<syncer::ModelTypeSyncBridge>();
     case syncer::AUTOFILL:
       return autofill::AutocompleteSyncBridge::FromWebDataService(
           web_data_service_.get());
@@ -508,7 +536,7 @@ ChromeSyncClient::GetSyncBridgeForModelType(syncer::ModelType type) {
 #endif
     default:
       NOTREACHED();
-      return base::WeakPtr<syncer::ModelTypeSyncBridge>();
+      return WeakPtr<syncer::ModelTypeSyncBridge>();
   }
 }