Revert of [Sync] Stop accessing BrowserContextKeyedServiceFactory on …

…non-UI thread. (patchset chromium#8 id:140001 of https://codereview.chromium.org/2769113002/ ) Reason for revert: Caused a memory regression, see crbug.com/708666 , unclear to me why exactly this is. Progress investigating the memory regression seems tricky so far, so reverting to fix more quickly. Original issue's description: > [Sync] Stop accessing BrowserContextKeyedServiceFactory on non-UI thread. > > In the past the AsyncDirectoryTypeController was passing a raw pointer > to a ChromeSyncClient to the model thread, where it had > GetSyncableServiceForType called on it. This was needed for several > model types, which has specific logic around only accessing their > SyncableService on their model threads. However, this was also bad, as > BrowserContextKeyedServiceFactorys were being used with a Profile > object on a non-UI thread, which is against the rules. > > To fix this, GetSyncableServiceForType returns a callback which should > only be run on the model thread, that will return the same > WeakPtr<SyncableService> that we were used to. The various non-UI > thread model types do slightly different things as each type has > different rules and threading guarantees. > > The most involved model type in this CL was extensions, which did not > have sufficient threading constraints to ensure safety. Added weak > pointer support to SyncValueStoreCache to fix this. > > BUG=701326 > > Review-Url: https://codereview.chromium.org/2769113002 > Cr-Commit-Position: refs/heads/master@{#460304} > Committed: https://chromium.googlesource.com/chromium/src/+/cd6b31d4b9613087e1a68c620af83b6fe1892bdf TBR=pavely@chromium.org,rdevlin.cronin@chromium.org,mathp@chromium.org,vasilii@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=701326 Review-Url: https://codereview.chromium.org/2799653006 Cr-Commit-Position: refs/heads/master@{#462578}
Denger-Network · Apr 6, 2017 · a56dd49 · a56dd49
1 parent e4e3e7e
commit a56dd49
Show file tree

Hide file tree

Showing 30 changed files with 235 additions and 338 deletions.
diff --git a/chrome/browser/extensions/api/storage/settings_apitest.cc b/chrome/browser/extensions/api/storage/settings_apitest.cc
@@ -113,10 +113,8 @@ class ExtensionSettingsApiTest : public ExtensionApiTest {
   }
 
   syncer::SyncableService* GetSyncableService() {
-    return settings_sync_util::GetSyncableServiceProvider(browser()->profile(),
-                                                          kModelType)
-        .Run()
-        .get();
+    return settings_sync_util::GetSyncableService(browser()->profile(),
+                                                  kModelType);
   }
 
   void InitSync(syncer::SyncChangeProcessor* sync_processor) {

diff --git a/chrome/browser/extensions/api/storage/settings_sync_util.cc b/chrome/browser/extensions/api/storage/settings_sync_util.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
 
-#include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/storage/sync_value_store_cache.h"
@@ -14,10 +13,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/api/storage/storage_frontend.h"
 
-using base::WeakPtr;
 using content::BrowserThread;
-using syncer::ModelType;
-using syncer::SyncableService;
 
 namespace extensions {
 
@@ -48,19 +44,13 @@ void PopulateAppSettingSpecifics(
       extension_id, key, value, specifics->mutable_extension_setting());
 }
 
-WeakPtr<SyncableService> GetSyncableService(WeakPtr<SyncValueStoreCache> cache,
-                                            ModelType type) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  return cache.get() ? cache->GetSyncableService(type)->AsWeakPtr()
-                     : WeakPtr<SyncableService>();
-}
-
 }  // namespace
 
-syncer::SyncData CreateData(const std::string& extension_id,
-                            const std::string& key,
-                            const base::Value& value,
-                            ModelType type) {
+syncer::SyncData CreateData(
+    const std::string& extension_id,
+    const std::string& key,
+    const base::Value& value,
+    syncer::ModelType type) {
   sync_pb::EntitySpecifics specifics;
   switch (type) {
     case syncer::EXTENSION_SETTINGS:
@@ -87,47 +77,47 @@ syncer::SyncData CreateData(const std::string& extension_id,
       extension_id + "/" + key, key, specifics);
 }
 
-syncer::SyncChange CreateAdd(const std::string& extension_id,
-                             const std::string& key,
-                             const base::Value& value,
-                             ModelType type) {
+syncer::SyncChange CreateAdd(
+    const std::string& extension_id,
+    const std::string& key,
+    const base::Value& value,
+    syncer::ModelType type) {
   return syncer::SyncChange(
       FROM_HERE,
       syncer::SyncChange::ACTION_ADD,
       CreateData(extension_id, key, value, type));
 }
 
-syncer::SyncChange CreateUpdate(const std::string& extension_id,
-                                const std::string& key,
-                                const base::Value& value,
-                                ModelType type) {
+syncer::SyncChange CreateUpdate(
+    const std::string& extension_id,
+    const std::string& key,
+    const base::Value& value,
+    syncer::ModelType type) {
   return syncer::SyncChange(
       FROM_HERE,
       syncer::SyncChange::ACTION_UPDATE,
       CreateData(extension_id, key, value, type));
 }
 
-syncer::SyncChange CreateDelete(const std::string& extension_id,
-                                const std::string& key,
-                                ModelType type) {
+syncer::SyncChange CreateDelete(
+    const std::string& extension_id,
+    const std::string& key,
+    syncer::ModelType type) {
   base::DictionaryValue no_value;
   return syncer::SyncChange(
       FROM_HERE,
       syncer::SyncChange::ACTION_DELETE,
       CreateData(extension_id, key, no_value, type));
 }
 
-syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
-    content::BrowserContext* context,
-    ModelType type) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+syncer::SyncableService* GetSyncableService(content::BrowserContext* context,
+                                            syncer::ModelType type) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   DCHECK(type == syncer::APP_SETTINGS || type == syncer::EXTENSION_SETTINGS);
   StorageFrontend* frontend = StorageFrontend::Get(context);
   SyncValueStoreCache* sync_cache = static_cast<SyncValueStoreCache*>(
       frontend->GetValueStoreCache(settings_namespace::SYNC));
-  // We must rely on our caller to guarantee that sync_cache->AsWeakPtr() is a
-  // valid call right now, and that shutdown has not begun.
-  return base::Bind(&GetSyncableService, sync_cache->AsWeakPtr(), type);
+  return sync_cache->GetSyncableService(type);
 }
 
 }  // namespace settings_sync_util

diff --git a/chrome/browser/extensions/api/storage/settings_sync_util.h b/chrome/browser/extensions/api/storage/settings_sync_util.h
@@ -5,9 +5,6 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_STORAGE_SETTINGS_SYNC_UTIL_H_
 #define CHROME_BROWSER_EXTENSIONS_API_STORAGE_SETTINGS_SYNC_UTIL_H_
 
-#include <string>
-
-#include "components/sync/driver/sync_client.h"
 #include "components/sync/model/sync_change.h"
 #include "components/sync/model/sync_data.h"
 
@@ -19,6 +16,10 @@ namespace content {
 class BrowserContext;
 }
 
+namespace syncer {
+class SyncableService;
+}
+
 namespace extensions {
 
 namespace settings_sync_util {
@@ -52,9 +53,8 @@ syncer::SyncChange CreateDelete(
 
 // Returns the sync service for settings. Must be called on the FILE thread.
 // |type| must be either APP_SETTINGS or EXTENSION_SETTINGS.
-syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
-    content::BrowserContext* context,
-    syncer::ModelType type);
+syncer::SyncableService* GetSyncableService(content::BrowserContext* context,
+                                            syncer::ModelType type);
 
 }  // namespace settings_sync_util
 

diff --git a/chrome/browser/extensions/api/storage/sync_value_store_cache.cc b/chrome/browser/extensions/api/storage/sync_value_store_cache.cc
@@ -35,7 +35,7 @@ SyncValueStoreCache::SyncValueStoreCache(
     const scoped_refptr<ValueStoreFactory>& factory,
     const scoped_refptr<SettingsObserverList>& observers,
     const base::FilePath& profile_path)
-    : initialized_(false), weak_ptr_factory_(this) {
+    : initialized_(false) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // This post is safe since the destructor can only be invoked from the
@@ -52,10 +52,6 @@ SyncValueStoreCache::~SyncValueStoreCache() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 }
 
-base::WeakPtr<SyncValueStoreCache> SyncValueStoreCache::AsWeakPtr() {
-  return weak_ptr_factory_.GetWeakPtr();
-}
-
 syncer::SyncableService* SyncValueStoreCache::GetSyncableService(
     syncer::ModelType type) const {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);

diff --git a/chrome/browser/extensions/api/storage/sync_value_store_cache.h b/chrome/browser/extensions/api/storage/sync_value_store_cache.h
@@ -6,12 +6,10 @@
 #define CHROME_BROWSER_EXTENSIONS_API_STORAGE_SYNC_VALUE_STORE_CACHE_H_
 
 #include <memory>
-#include <string>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "components/sync/model/syncable_service.h"
 #include "extensions/browser/api/storage/settings_observer.h"
 #include "extensions/browser/api/storage/value_store_cache.h"
@@ -38,8 +36,6 @@ class SyncValueStoreCache : public ValueStoreCache {
                       const base::FilePath& profile_path);
   ~SyncValueStoreCache() override;
 
-  base::WeakPtr<SyncValueStoreCache> AsWeakPtr();
-
   syncer::SyncableService* GetSyncableService(syncer::ModelType type) const;
 
   // ValueStoreCache implementation:
@@ -56,7 +52,6 @@ class SyncValueStoreCache : public ValueStoreCache {
   bool initialized_;
   std::unique_ptr<SyncStorageBackend> app_backend_;
   std::unique_ptr<SyncStorageBackend> extension_backend_;
-  base::WeakPtrFactory<SyncValueStoreCache> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncValueStoreCache);
 };