Bug 825190  is almost a duplicate of this, but they both have relevant information so let's keep both open.

 Issue 825190  has been merged into this issue.

Most of the work here has happened on  bug 825190 .
There's one remaining usage of SigninManager::SignOut when receiving DISABLE_SYNC_ON_CLIENT from the server. We might eventually be able to just get rid of that, see bug 246839.

Thanks for the update and all the great work here! I've noted down for when we get started on signin/signout that this is the one thing blocking complete removal of SigninManager from sync.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c46597ffd824413e62cc7ec26e5bacfca32d359d

commit c46597ffd824413e62cc7ec26e5bacfca32d359d
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jun 14 16:43:10 2018

Introduce two-phase shutdown of PowerMonitorSource

PowerMonitorBroadcastSource suffers from raciness in that it can
procress incoming Mojo messages on one thread while both it and the
process-wide PowerMonitor instance are being destroyed on another
thread. A previous CL introduced a mitigation for this problem by
shutting down the processing of incoming Mojo messages in the
PowerMonitorBroadcastSource destructor before proceeding with
destruction (https://chromium-review.googlesource.com/1041215). However,
that turns out not to be sufficient to eliminate the race:
the PowerMonitorBroadcastSource instance can end up waiting for the
lock in its destructor while on another thread it is holding the lock
to process an incoming Mojo message. That second thread can then call
back into the process-wide PowerMonitor instance while it is being
torn down, resulting in crashes due to undefined state (specifically,
the PowerMonitor instance has already set its PowerMonitorSource
instance to nullptr, resulting in a crash when that instance is being
accessed -- ironically, the very instance that is currently blocked
in its destructor waiting for the processing of the incoming Mojo
message to finish :).

This CL removes the possibility of that race on shutdown by introducing
a two-phase shutdown process for PowerMonitorSource. In PowerMonitor's
destructor, it first invokes PowerMonitorSource::Shutdown() before
proceeding. The contract of PowerMonitorSource::Shutdown() is that
subclasses must take any necessary action to ensure that on return
from that method, they will no longer call into PowerMonitor.
PowerMonitorBroadcastSource fulfills this contract by stopping the
process of incoming Mojo messages in
PowerMonitorBroadcastSource::Shutdown(). In the above scenario,
the broadcast source will end up waiting for the processing of the
incoming Mojo message to finish *inside*
PowerMonitorBroadcastSource::Shutdown(), which is safe by design as it
is before PowerMonitor has done any teardown.

Note that it is possible that there are still other kinds of
raciness between PowerMonitorBroadcastSource and PowerMonitor; as I
expressed on
https://bugs.chromium.org/p/chromium/issues/detail?id=834312#c33, I am
not confident about the overall threading model here.

TBR=xjz@chromium.org, mmenke@chromium.org

Bug:  809031 
Change-Id: I9d0f291995b8185e16dff1f88564b6e5344439b2
Reviewed-on: https://chromium-review.googlesource.com/1098932
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567302}
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/base/power_monitor/power_monitor.cc
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/base/power_monitor/power_monitor_device_source.cc
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/base/power_monitor/power_monitor_device_source.h
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/base/power_monitor/power_monitor_source.h
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/base/test/power_monitor_test_base.cc
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/base/test/power_monitor_test_base.h
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/media/cast/sender/h264_vt_encoder_unittest.cc
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/services/device/public/cpp/power_monitor/power_monitor_broadcast_source.cc
[modify] https://crrev.com/c46597ffd824413e62cc7ec26e5bacfca32d359d/services/device/public/cpp/power_monitor/power_monitor_broadcast_source.h

I think the above CL went on the wrong bug :)

Comment 8: Yes, I think it was meant for issue 834312.  :)

Chris is bringing up the IdentityManager API for signout now and will look at converting sync to use it.

Changes out for review

https://chromium-review.googlesource.com/c/chromium/src/+/1175911
https://chromium-review.googlesource.com/c/chromium/src/+/1175916

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9641914dd50ae0d1ef1c58d6dc393aad1e8a26b

commit d9641914dd50ae0d1ef1c58d6dc393aad1e8a26b
Author: chcunningham <chcunningham@chromium.org>
Date: Wed Aug 29 19:50:46 2018

Migrate ProfileSyncService and tests to IdentityManager

SigninManager is being deprecated. Long live IdentityManager.

IdentityManager recently added ClearPrimaryAccount() (aka SignOut())
which unblocks migration of this class and its tests.

A follow up CL will remove the SigninWrapper used to initialize
ProfileSyncService. We now just need the IdentityManager.

Bug:  809031 
Change-Id: If7fd07ea1c53ea1ed24daf7ee70b27c67cd7e20c
Reviewed-on: https://chromium-review.googlesource.com/1175911
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587256}
[modify] https://crrev.com/d9641914dd50ae0d1ef1c58d6dc393aad1e8a26b/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/d9641914dd50ae0d1ef1c58d6dc393aad1e8a26b/components/browser_sync/profile_sync_service_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/275f98adf5b67286b3ff167acffeae8379b20b37

commit 275f98adf5b67286b3ff167acffeae8379b20b37
Author: chcunningham <chcunningham@chromium.org>
Date: Thu Aug 30 20:09:46 2018

Delete SigninWrapper from ProfileSyncService init

The wrapper was used to provide references to SigninManager while the
transition to IdentityManager was ongoing. The ancestor of this CL*
will complete that transition, so we can delete this wrapper.

*https://chromium-review.googlesource.com/c/chromium/src/+/1175911

Bug:  809031 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I359d2beebdf76d6620ec46b5bc58748ebb61e544
Reviewed-on: https://chromium-review.googlesource.com/1175916
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587724}
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/chrome/browser/sync/profile_sync_service_factory.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/chrome/browser/sync/profile_sync_test_util.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/components/sync/BUILD.gn
[delete] https://crrev.com/0064f414429476906e2bf268424af45f1d57efd4/components/sync/driver/signin_manager_wrapper.cc
[delete] https://crrev.com/0064f414429476906e2bf268424af45f1d57efd4/components/sync/driver/signin_manager_wrapper.h
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/ios/chrome/browser/sync/profile_sync_service_factory.cc
[modify] https://crrev.com/275f98adf5b67286b3ff167acffeae8379b20b37/ios/web_view/internal/sync/web_view_profile_sync_service_factory.mm

Colin, back to you. My browser_sync/ is converted but I'm not sure if that's all the work that remained for this bug.

This bug is fixed, as there are no more uses of the deprecated classes in ProfileSyncService itself. There are still test uses in //components/browser_sync; I'll track those in a separate bug.