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

chrome/browser/sync/chrome_sync_client.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/chrome_sync_client.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 102 matching lines @@
 #include "chrome/browser/chromeos/arc/arc_util.h"
 #include "chrome/browser/chromeos/printing/printers_manager.h"
 #include "chrome/browser/chromeos/printing/printers_manager_factory.h"
 #include "chrome/browser/chromeos/printing/printers_sync_bridge.h"
 #include "chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h"
 #include "chrome/browser/ui/app_list/arc/arc_package_syncable_service.h"
 #include "components/sync_wifi/wifi_credential_syncable_service.h"
 #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
 // due to possible multiple inheritance issues, wherein ChromeSyncClient might
 // inherit from other interfaces with same methods.
 class SyncSessionsClientImpl : public sync_sessions::SyncSessionsClient {
  public:
   explicit SyncSessionsClientImpl(Profile* profile) : profile_(profile) {
     window_delegates_getter_.reset(
 #if defined(OS_ANDROID)
@@ skipping 144 matching lines @@
 
 favicon::FaviconService* ChromeSyncClient::GetFaviconService() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return FaviconServiceFactory::GetForProfile(
       profile_, ServiceAccessType::IMPLICIT_ACCESS);
 }
 
 history::HistoryService* ChromeSyncClient::GetHistoryService() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return HistoryServiceFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
+      profile_, ServiceAccessType::IMPLICIT_ACCESS);
 }
 
 bool ChromeSyncClient::HasPasswordStore() {
   return password_store_ != nullptr;
 }
 
 autofill::PersonalDataManager* ChromeSyncClient::GetPersonalDataManager() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return autofill::PersonalDataManagerFactory::GetForProfile(profile_);
 }
@@ skipping 29 matching lines @@
 
 scoped_refptr<syncer::ExtensionsActivity>
 ChromeSyncClient::GetExtensionsActivity() {
   return extensions_activity_monitor_.GetExtensionsActivity();
 }
 
 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: {
+    case syncer::HISTORY_DELETE_DIRECTIVES:
+      return WrapInProvider(GetHistoryService());
+    case syncer::TYPED_URLS: {
       history::HistoryService* history = GetHistoryService();
-      return history ? history->AsWeakPtr()
-                     : base::WeakPtr<history::HistoryService>();
-    }
-    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();
+      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>();
+      return WrapInProvider(distiller ? distiller->GetSyncableService()
+                                      : nullptr);
     }
-    case syncer::SESSIONS: {
-      return ProfileSyncServiceFactory::GetForProfile(profile_)->
-          GetSessionsSyncableService()->AsWeakPtr();
-    }
-    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:
       return ProfileSyncServiceFactory::GetForProfile(profile_)
           ->GetDeviceInfoSyncBridge()
           ->AsWeakPtr();
     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());
 #if defined(OS_CHROMEOS)
     case syncer::PRINTERS:
       return chromeos::PrintersManagerFactory::GetForBrowserContext(profile_)
           ->GetSyncBridge()
           ->AsWeakPtr();
 #endif
     default:
       NOTREACHED();
-      return base::WeakPtr<syncer::ModelTypeSyncBridge>();
+      return WeakPtr<syncer::ModelTypeSyncBridge>();
   }
 }
 
 scoped_refptr<syncer::ModelSafeWorker>
 ChromeSyncClient::CreateModelWorkerForGroup(syncer::ModelSafeGroup group) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   switch (group) {
     case syncer::GROUP_DB:
       return new syncer::BrowserThreadModelWorker(
           BrowserThread::GetTaskRunnerForThread(BrowserThread::DB),
@@ skipping 180 matching lines @@
   sync_service->RegisterDataTypeController(
       base::MakeUnique<SupervisedUserSyncDataTypeController>(
           syncer::SUPERVISED_USER_SETTINGS, error_callback, this, profile_));
   sync_service->RegisterDataTypeController(
       base::MakeUnique<SupervisedUserSyncDataTypeController>(
           syncer::SUPERVISED_USER_WHITELISTS, error_callback, this, profile_));
 #endif
 }
 
 }  // namespace browser_sync