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

chrome/browser/extensions/api/storage/settings_sync_util.h

Index: chrome/browser/extensions/api/storage/settings_sync_util.h
diff --git a/chrome/browser/extensions/api/storage/settings_sync_util.h b/chrome/browser/extensions/api/storage/settings_sync_util.h
index 4e23bae06192c33f64f1cefd4804652e3caab65e..12bb7ee7be4336d220a5e5e8b0ff7303865bb8d7 100644
--- a/chrome/browser/extensions/api/storage/settings_sync_util.h
+++ b/chrome/browser/extensions/api/storage/settings_sync_util.h
@@ -5,6 +5,9 @@
 #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"
 
@@ -16,10 +19,6 @@ namespace content {
 class BrowserContext;
 }
 
-namespace syncer {
-class SyncableService;
-}
-
 namespace extensions {
 
 namespace settings_sync_util {
@@ -53,8 +52,9 @@ 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::SyncableService* GetSyncableService(content::BrowserContext* context,
-                                            syncer::ModelType type);
+syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(

[Devlin, 2017/03/27 21:54:23]

I'm a little nervous about this - when is the provider going to be called, and
on which thread?

[skym, 2017/03/27 22:38:47]

FILE thread. Whooops, looks like I didn't put a
DCHECK_CURRENTLY_ON(BrowserThread::FILE); inside ::GetSyncableService(). I'll
add that back.

The idea is that we grab a callback (ServiceProvider) on the UI thread, and then
post a task to the corresponding model thread (in this case FILE), and then run
the callback there. Which means that we need some thread safe object to capture
in the callback that we can post across threads.

Extensions was the trickiest, since there didn't seem to exist anything like
this. Which is why I added a WeakPtr to SyncValueStoreCache.

I'm hoping it's safe to call sync_cache->AsWeakPtr() on the UI thread because
ProfileSyncServiceFactory DependsOn... some extensions thing, that I was kind of
assuming was providing us this safety, but it's possible I'm wrong. 

https://cs.chromium.org/chromium/src/chrome/browser/sync/profile_sync_service...
.

I tried to do something with ValueStoreCache::ShutdownOnUI(), but values are
never removed from StorageFrontend::caches_. I'd love a different solution that
makes this all significantly more elegant.

[Devlin, 2017/03/28 01:17:23]

WeakPtrs are only safe if they are invalidated on the same thread they are
created on, so it sounds like this is unsafe usage.  I'll take another look
tomorrow and see if we can't figure something out. :)

[skym, 2017/03/28 05:01:19]

Hmm, I'm not sure. Reading
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=55 it seems a bit
unclear. I'm definitely only derefing the weak pointers on the FILE thread. The
factory is never derefed per say.

// To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory
// is dereferenced, the factory and its WeakPtrs become bound to the calling
// thread or current SequencedWorkerPool token, and cannot be dereferenced or
// invalidated on any other task runner. Bound WeakPtrs can still be handed
// off to other task runners, e.g. to use to post tasks back to object on the
// bound sequence.

[Devlin, 2017/03/29 01:47:40]

The section right above that makes it more clear, I think:

// Weak pointers may be passed safely between threads, but must always be
// dereferenced and invalidated on the same SequencedTaskRunner otherwise
// checking the pointer would be racey.

https://chromium.googlesource.com/chromium/src/+/95cf7a59de278ede5e772ba75221...

Destroying the WeakPtrFactory invalidates the weak ptrs.  That said, it actually
looks like the SyncValueStore is destroyed on the file thread, despite being
constructed on the UI thread.  That would normally also be racy, except that we
go through the proxy object of the storage frontend, which guarantees that the
value stores are valid for its lifetime.  So actually... I think this is safe,
despite the pattern being typically unsafe.

+    content::BrowserContext* context,
+    syncer::ModelType type);
 
 }  // namespace settings_sync_util