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

[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

[skym, 2017-03-23 05:36:06]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 05:36:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-03-23 05:42:00]

Description was changed from

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

BUG=701326
==========

to

==========
[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
==========

[skym, 2017-03-23 05:42:00]

skym@chromium.org changed reviewers:
+ pavely@chromium.org

[skym, 2017-03-23 05:42:49]

PTAL

[commit-bot: I haz the power, 2017-03-23 05:45:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 05:45:43]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[skym, 2017-03-23 06:13:42]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 06:13:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-03-23 06:14:14]

Some updates to fix compile issues. iOS is likely still broken.

[commit-bot: I haz the power, 2017-03-23 06:23:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 06:23:07]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[skym, 2017-03-24 14:38:24]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 14:39:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 14:42:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 14:42:43]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[skym, 2017-03-24 14:59:47]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 15:00:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-03-24 15:55:41]

Okay, iOS is fixed, PLTA Pavel.

[commit-bot: I haz the power, 2017-03-24 16:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 16:19:01]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-03-27 19:38:26]

lgtm % comment about comment in chrome_sync_client.cc

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/settings_sync_util.cc (right):

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/settings_sync_util.cc:53: // WeakPtr must
be explicitly checked instead of using Bind magic because we
I don't think there is any Bind magic here. If cache was invalid,
cache->GetSyncableService() would DCHECK or SEGFAULT.

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:123: using
autofill::AutocompleteSyncableService;
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.

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:149: template <typename T>
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).

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:485:
dom_distiller::DomDistillerService* distiller =
nit: distiller => distiller_service?

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
File components/sync/driver/shared_change_processor.cc (right):

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
components/sync/driver/shared_change_processor.cc:56: const ServiceProvider&
service_provider,
ServiceProvider looks weird outside of context of sync_client.h. How about
SyncableServiceProvider?

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
File components/sync/driver/sync_client.h (right):

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
components/sync/driver/sync_client.h:106: // Both the provider and the resulting
weak pointer should only be accessed
nit: "should only be accessed" => "will only be accessed" ?
You are describing to implementers guarantees about how this API will be used by
sync engine.

[skym, 2017-03-27 21:45:25]

skym@chromium.org changed reviewers:
+ mathp@chromium.org, rdevlin.cronin@chromium.org, vasilii@chromium.org

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

Updates for Pavel. Adding OWNERs for specific model types, PTAL.

rdevlin.cronin@ - chrome/browser/extensions/api/storage/*
mathp@ - components/autofill/*
vasilii@ - components/search_engines/*

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/settings_sync_util.cc (right):

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/settings_sync_util.cc:53: // WeakPtr must
be explicitly checked instead of using Bind magic because we
On 2017/03/27 19:38:25, pavely wrote:
> I don't think there is any Bind magic here. If cache was invalid,
> cache->GetSyncableService() would DCHECK or SEGFAULT.

Removed the confusing comment.

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:123: using
autofill::AutocompleteSyncableService;
On 2017/03/27 19:38:25, pavely wrote:
> 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.

Done.

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:149: template <typename T>
On 2017/03/27 19:38:25, pavely wrote:
> 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).

Done.

https://codereview.chromium.org/2769113002/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:485:
dom_distiller::DomDistillerService* distiller =
On 2017/03/27 19:38:25, pavely wrote:
> nit: distiller => distiller_service?

Done.

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
File components/sync/driver/shared_change_processor.cc (right):

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
components/sync/driver/shared_change_processor.cc:56: const ServiceProvider&
service_provider,
On 2017/03/27 19:38:25, pavely wrote:
> ServiceProvider looks weird outside of context of sync_client.h. How about
> SyncableServiceProvider?

Removed using instead, hopefully that's okay.

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
File components/sync/driver/sync_client.h (right):

https://codereview.chromium.org/2769113002/diff/80001/components/sync/driver/...
components/sync/driver/sync_client.h:106: // Both the provider and the resulting
weak pointer should only be accessed
On 2017/03/27 19:38:25, pavely wrote:
> nit: "should only be accessed" => "will only be accessed" ?
> You are describing to implementers guarantees about how this API will be used
by
> sync engine.

Done.

[skym, 2017-03-27 21:45:28]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 21:46:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/storage/settings_sync_util.h (right):

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/storage/settings_sync_util.h:55:
syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
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:22]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/storage/settings_sync_util.h (right):

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/storage/settings_sync_util.h:55:
syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
On 2017/03/27 21:54:23, Devlin wrote:
> I'm a little nervous about this - when is the provider going to be called, and
> on which thread?

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.

[commit-bot: I haz the power, 2017-03-27 22:38:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 23:27:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 23:27:30]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-03-27 23:36:23]

autofill lgtm

[Devlin, 2017-03-28 01:17:24]

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/storage/settings_sync_util.h (right):

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/storage/settings_sync_util.h:55:
syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
On 2017/03/27 22:38:47, skym wrote:
> On 2017/03/27 21:54:23, Devlin wrote:
> > I'm a little nervous about this - when is the provider going to be called,
and
> > on which thread?
> 
> 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.

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:20]

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/storage/settings_sync_util.h (right):

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/storage/settings_sync_util.h:55:
syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
On 2017/03/28 01:17:23, Devlin wrote:
> On 2017/03/27 22:38:47, skym wrote:
> > On 2017/03/27 21:54:23, Devlin wrote:
> > > I'm a little nervous about this - when is the provider going to be called,
> and
> > > on which thread?
> > 
> > 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.
> 
> 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. :)

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.

[vasilii, 2017-03-28 09:38:14]

lgtm

components/search_engines/search_engine_data_type_controller_unittest.cc

[skym, 2017-03-28 18:06:10]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:06:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 18:22:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 18:22:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

extensions lgtm.  I think I've convinced myself this is safe.  Ideally, we'd
clean up the threading in this area, but that's a separate issue.

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/storage/settings_sync_util.h (right):

https://codereview.chromium.org/2769113002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/storage/settings_sync_util.h:55:
syncer::SyncClient::ServiceProvider GetSyncableServiceProvider(
On 2017/03/28 05:01:19, skym wrote:
> On 2017/03/28 01:17:23, Devlin wrote:
> > On 2017/03/27 22:38:47, skym wrote:
> > > On 2017/03/27 21:54:23, Devlin wrote:
> > > > I'm a little nervous about this - when is the provider going to be
called,
> > and
> > > > on which thread?
> > > 
> > > 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.
> > 
> > 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. :)
> 
> 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.
> 

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.

[skym, 2017-03-29 05:58:53]

The CQ bit was checked by skym@chromium.org

[skym, 2017-03-29 05:58:54]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
mathp@chromium.org, vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2769113002/#ps140001
(title: "Rebase and removing dependent patch set.")

[commit-bot: I haz the power, 2017-03-29 05:59:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 06:47:06]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490767133840390,
"parent_rev": "41d64776a2936bd014ebed1340d3aaf9e5eeeede", "commit_rev":
"cd6b31d4b9613087e1a68c620af83b6fe1892bdf"}

[commit-bot: I haz the power, 2017-03-29 06:47:54]

Description was changed from

==========
[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
==========

to

==========
[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/+/cd6b31d4b9613087e1a68c620af8...
==========

[commit-bot: I haz the power, 2017-03-29 06:47:55]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cd6b31d4b9613087e1a68c620af8...

[skym, 2017-04-06 17:08:43]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2799653006/ by skym@chromium.org.

The reason for reverting is: 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..