Do not observe MessageLoop destruction from ModelSafeWorker.

Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=650723
Committed: https://crrev.com/ea7a7eca75b39bfc9cbf72aed8a4dffa1d77f1e2
Cr-Commit-Position: refs/heads/master@{#430293}

[fdoray, 2016-11-02 19:45:09]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers.

BUG=653916
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to send work to their thread.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify the parent SyncBackendRegistrar. When all
   ModelSafeWorkers have notified the SyncBackendRegistrar that
   their MessageLoop was destroyed, the SyncBackendRegistrar
   destroys itself.

-> There is no reason to keep the SyncBackendRegistrar
   alive after both SyncManager and SyncBackendHostImpl have
   released their reference to it. Code is easier to understand
   when a delete task is posted from
   SyncBackendHostImpl::Shutdown() than when SyncBackendRegistrar
   deletes itself after receiving destruction notifications from
   all its ModelSafeWorkers.

BUG=653916
==========

[fdoray, 2016-11-02 19:45:26]

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

[commit-bot: I haz the power, 2016-11-02 19:46:03]

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

[fdoray, 2016-11-02 19:49:54]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to send work to their thread.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify the parent SyncBackendRegistrar. When all
   ModelSafeWorkers have notified the SyncBackendRegistrar that
   their MessageLoop was destroyed, the SyncBackendRegistrar
   destroys itself.

-> There is no reason to keep the SyncBackendRegistrar
   alive after both SyncManager and SyncBackendHostImpl have
   released their reference to it. Code is easier to understand
   when a delete task is posted from
   SyncBackendHostImpl::Shutdown() than when SyncBackendRegistrar
   deletes itself after receiving destruction notifications from
   all its ModelSafeWorkers.

BUG=653916
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to send work to their thread.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify the parent SyncBackendRegistrar. When all
   ModelSafeWorkers have notified the SyncBackendRegistrar that
   their MessageLoop was destroyed, the SyncBackendRegistrar
   destroys itself.

-> There is no reason to keep the SyncBackendRegistrar
   alive after both SyncManager and SyncBackendHostImpl have
   released their reference to it. SyncBackendRegistrar can
   be destroyed by a DeleteSoon task posted by
   SyncBackendHostImpl::Shutdown().

-> ModelSafeWorkers are ref-counted. Releasing references
   when SyncBackendRegistrar is deleted is not dangerous since
   any code that needs to use a ModelSafeWorker should have its
   own reference to it.

BUG=653916
==========

[commit-bot: I haz the power, 2016-11-02 20:01:25]

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

[commit-bot: I haz the power, 2016-11-02 20:01:26]

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...)

[fdoray, 2016-11-02 20:21:54]

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

[commit-bot: I haz the power, 2016-11-02 20:22:44]

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, 2016-11-02 22:01:32]

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

[commit-bot: I haz the power, 2016-11-02 22:01:33]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-11-03 12:13:14]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to send work to their thread.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify the parent SyncBackendRegistrar. When all
   ModelSafeWorkers have notified the SyncBackendRegistrar that
   their MessageLoop was destroyed, the SyncBackendRegistrar
   destroys itself.

-> There is no reason to keep the SyncBackendRegistrar
   alive after both SyncManager and SyncBackendHostImpl have
   released their reference to it. SyncBackendRegistrar can
   be destroyed by a DeleteSoon task posted by
   SyncBackendHostImpl::Shutdown().

-> ModelSafeWorkers are ref-counted. Releasing references
   when SyncBackendRegistrar is deleted is not dangerous since
   any code that needs to use a ModelSafeWorker should have its
   own reference to it.

BUG=653916
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to send work to their thread.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have released
   their reference to it. With this CL, SyncBackendRegistrar is
   deleted by a DeleteSoon task posted from
   SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=653916
==========

[fdoray, 2016-11-03 12:13:48]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to send work to their thread.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have released
   their reference to it. With this CL, SyncBackendRegistrar is
   deleted by a DeleteSoon task posted from
   SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=653916
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have released
   their reference to it. With this CL, SyncBackendRegistrar is
   deleted by a DeleteSoon task posted from
   SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=653916
==========

[fdoray, 2016-11-03 12:14:48]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have released
   their reference to it. With this CL, SyncBackendRegistrar is
   deleted by a DeleteSoon task posted from
   SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=653916
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=653916
==========

[fdoray, 2016-11-03 12:19:56]

fdoray@chromium.org changed reviewers:
+ zea@chromium.org

[fdoray, 2016-11-03 12:19:57]

zea@: PTAL

Disclaimer: I spent a lot of time to analyze the
ModelSafeWorker/SyncBackendRegistrar code and I'm pretty sure that this CL is
correct. However, I'm not a sync expert. Don't hesitate to tell me if I said
something wrong in the CL description.

[maxbogue, 2016-11-03 17:05:30]

maxbogue@chromium.org changed reviewers:
+ maxbogue@chromium.org

[maxbogue, 2016-11-03 17:05:31]

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
So my understanding of the current weird structure regarding
SyncBackendRegistrar keeping ownership of the sync thread is that we do not want
to block browser shutdown on it. If you return it here, its ownership gets
passed to ProfileSyncService, which will try to destroy it when it gets
destroyed and block while it joins, which (again, AFAIK) can take a while if
tasks are running on it.

Can you convince me this isn't a problem? I've wanted to move the sync thread
out of the SBR for a long time, but the only solution I've come up with is to
just leak it on shutdown from PSS.

[maxbogue, 2016-11-03 17:10:23]

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
On 2016/11/03 17:05:31, maxbogue wrote:
> So my understanding of the current weird structure regarding
> SyncBackendRegistrar keeping ownership of the sync thread is that we do not
want
> to block browser shutdown on it. If you return it here, its ownership gets
> passed to ProfileSyncService, which will try to destroy it when it gets
> destroyed and block while it joins, which (again, AFAIK) can take a while if
> tasks are running on it.
> 
> Can you convince me this isn't a problem? I've wanted to move the sync thread
> out of the SBR for a long time, but the only solution I've come up with is to
> just leak it on shutdown from PSS.

Additionally note that I'm not actually positive the current structure works,
since it seems like it will just result in the sync thread being destroyed on an
arbitrary message loop later on...

[maxbogue, 2016-11-03 17:29:11]

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
On 2016/11/03 17:10:23, maxbogue wrote:
> On 2016/11/03 17:05:31, maxbogue wrote:
> > So my understanding of the current weird structure regarding
> > SyncBackendRegistrar keeping ownership of the sync thread is that we do not
> want
> > to block browser shutdown on it. If you return it here, its ownership gets
> > passed to ProfileSyncService, which will try to destroy it when it gets
> > destroyed and block while it joins, which (again, AFAIK) can take a while if
> > tasks are running on it.
> > 
> > Can you convince me this isn't a problem? I've wanted to move the sync
thread
> > out of the SBR for a long time, but the only solution I've come up with is
to
> > just leak it on shutdown from PSS.
> 
> Additionally note that I'm not actually positive the current structure works,
> since it seems like it will just result in the sync thread being destroyed on
an
> arbitrary message loop later on...

Update: it seems that we currently don't block the /window/ closing on the sync
thread but the actual process exiting /does/ block on the sync thread, so I
guess this will at least not be a regression.

[fdoray, 2016-11-03 19:11:06]

maxbogue@: PTAnL

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
On 2016/11/03 17:29:11, maxbogue wrote:
> On 2016/11/03 17:10:23, maxbogue wrote:
> > On 2016/11/03 17:05:31, maxbogue wrote:
> > > So my understanding of the current weird structure regarding
> > > SyncBackendRegistrar keeping ownership of the sync thread is that we do
not
> > want
> > > to block browser shutdown on it. If you return it here, its ownership gets
> > > passed to ProfileSyncService, which will try to destroy it when it gets
> > > destroyed and block while it joins, which (again, AFAIK) can take a while
if
> > > tasks are running on it.
> > > 

Before AND after this CL, SyncBackendHostImpl::Shutdown joins the sync thread
when |reason| is BROWSER_SHUTDOWN and transfers ownership to the caller when
|reason| != BROWSER_SHUTDOWN. The only thing that changes with this CL is when
SyncBackendRegistrar is destroyed. Therefore, I don't believe that this CL will
regress shutdown time.

> > > Can you convince me this isn't a problem? I've wanted to move the sync
> thread
> > > out of the SBR for a long time, but the only solution I've come up with is
> to
> > > just leak it on shutdown from PSS.
> > 

We plan to migrate the sync thread to TaskScheduler, whose threads are not
joined on shutdown [1]. Therefore, the sync thread blocking shutdown will soon
stop being an issue.

[1] We wait until all BLOCK_SHUTDOWN tasks have completed their execution. After
that, the process may be terminated even if there are pending tasks.

> > Additionally note that I'm not actually positive the current structure
works,
> > since it seems like it will just result in the sync thread being destroyed
on
> an
> > arbitrary message loop later on...
> 

When SyncBackendHostImpl::Shutdown() is called from
ProfileSyncService::Shutdown(), |reason| is BROWSER_SHUTDOWN.
SyncBackendHostImpl::Shutdown() joins and deletes the sync thread.

When SyncBackendHostImpl::Shutdown() is called outside of
ProfileSyncService::Shutdown(), |reason| is *not* BROWSER_SHUTDOWN.
SyncBackendHostImpl::Shutdown() transfers ownership of the sync thread to
ProfileSyncService which joins it delete it later from
ProfileSyncService::Shutdown().

tl;dr: The sync thread was and is still joined and deleted as part of
ProfileSyncService::Shutdown().

> Update: it seems that we currently don't block the /window/ closing on the
sync
> thread but the actual process exiting /does/ block on the sync thread, so I
> guess this will at least not be a regression.

[maxbogue, 2016-11-03 23:48:33]

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
On 2016/11/03 19:11:05, fdoray wrote:
> Before AND after this CL, SyncBackendHostImpl::Shutdown joins the sync thread
> when |reason| is BROWSER_SHUTDOWN and transfers ownership to the caller when
> |reason| != BROWSER_SHUTDOWN. The only thing that changes with this CL is when
> SyncBackendRegistrar is destroyed. Therefore, I don't believe that this CL
will
> regress shutdown time.

Prior to this CL, the sync thread was joined whenever the released
SyncBackendRegistrar was eventually destroyed, after the workers got destroyed
(asynchronously). Now it's synchronously joined at the end of the method. I'd
actually prefer you just always pass the sync thread back up to to PSS so it can
be destroyed whenever that is.

[fdoray, 2016-11-04 12:24:01]

maxbogue@: PTAL

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
On 2016/11/03 23:48:33, maxbogue wrote:
> On 2016/11/03 19:11:05, fdoray wrote:
> > Before AND after this CL, SyncBackendHostImpl::Shutdown joins the sync
thread
> > when |reason| is BROWSER_SHUTDOWN and transfers ownership to the caller when
> > |reason| != BROWSER_SHUTDOWN. The only thing that changes with this CL is
when
> > SyncBackendRegistrar is destroyed. Therefore, I don't believe that this CL
> will
> > regress shutdown time.
> 
> Prior to this CL, the sync thread was joined whenever the released
> SyncBackendRegistrar was eventually destroyed, after the workers got destroyed
> (asynchronously). Now it's synchronously joined at the end of the method. I'd
> actually prefer you just always pass the sync thread back up to to PSS so it
can
> be destroyed whenever that is.

Done. You're right. I didn't read this code correctly.

With my latest patch set, SyncBackendHostImpl::Shutdown() always transfers
ownership of the sync thread to ProfileSyncService which joins it here
https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser....
This code runs after the browser window is closed.

Is that what you wanted?

I'm not too concerned about the impact of joining the sync thread on shutdown
time since the sync thread will soon be migrated to TaskScheduler.

[maxbogue, 2016-11-04 17:34:58]

lgtm! After this lands I'll do a cleanup CL to completely remove the sync thread
from the registrar, so PSS just always has ownership.

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2471183003/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:272: return
released_sync_thread;
On 2016/11/04 12:24:01, fdoray wrote:
> On 2016/11/03 23:48:33, maxbogue wrote:
> > On 2016/11/03 19:11:05, fdoray wrote:
> > > Before AND after this CL, SyncBackendHostImpl::Shutdown joins the sync
> thread
> > > when |reason| is BROWSER_SHUTDOWN and transfers ownership to the caller
when
> > > |reason| != BROWSER_SHUTDOWN. The only thing that changes with this CL is
> when
> > > SyncBackendRegistrar is destroyed. Therefore, I don't believe that this CL
> > will
> > > regress shutdown time.
> > 
> > Prior to this CL, the sync thread was joined whenever the released
> > SyncBackendRegistrar was eventually destroyed, after the workers got
destroyed
> > (asynchronously). Now it's synchronously joined at the end of the method.
I'd
> > actually prefer you just always pass the sync thread back up to to PSS so it
> can
> > be destroyed whenever that is.
> 
> Done. You're right. I didn't read this code correctly.
> 
> With my latest patch set, SyncBackendHostImpl::Shutdown() always transfers
> ownership of the sync thread to ProfileSyncService which joins it here
>
https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser....
> This code runs after the browser window is closed.
> 
> Is that what you wanted?
> 
> I'm not too concerned about the impact of joining the sync thread on shutdown
> time since the sync thread will soon be migrated to TaskScheduler.
> 

Ah, didn't realize that PSS joins it there, I thought it would just happen on
destruction. That's fine I suppose.

https://codereview.chromium.org/2471183003/diff/40001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_registrar.h (right):

https://codereview.chromium.org/2471183003/diff/40001/components/sync/driver/...
components/sync/driver/glue/sync_backend_registrar.h:54: //   5) If the
ShutdownReason is not BROWSER_SHUTDOWN, transfer ownership of
You can just remove 5)

https://codereview.chromium.org/2471183003/diff/40001/components/sync/engine/...
File components/sync/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/2471183003/diff/40001/components/sync/engine/...
components/sync/engine/model_safe_worker.cc:72:
ModelSafeWorker::ModelSafeWorker() = default;
Is there some reason "= default;" is preferable to "{}"?

[maxbogue, 2016-11-04 17:42:21]

maxbogue@chromium.org changed required reviewers:
+ zea@chromium.org

[maxbogue, 2016-11-04 17:42:21]

Whoops, forgot to say: please wait for Nicolas to review as well, as he's more
familiar with the subtleties of the ModelSafeWorker code than I am.

[fdoray, 2016-11-04 17:56:49]

fdoray@chromium.org changed reviewers:
+ isherman@chromium.org, sky@chromium.org

[fdoray, 2016-11-04 17:56:50]

zea@: PTAL

sky@: Please review changes in components/history
isherman@: Please review changes in components/password_manager

https://codereview.chromium.org/2471183003/diff/40001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_registrar.h (right):

https://codereview.chromium.org/2471183003/diff/40001/components/sync/driver/...
components/sync/driver/glue/sync_backend_registrar.h:54: //   5) If the
ShutdownReason is not BROWSER_SHUTDOWN, transfer ownership of
On 2016/11/04 17:34:57, maxbogue wrote:
> You can just remove 5)

Done.

https://codereview.chromium.org/2471183003/diff/40001/components/sync/engine/...
File components/sync/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/2471183003/diff/40001/components/sync/engine/...
components/sync/engine/model_safe_worker.cc:72:
ModelSafeWorker::ModelSafeWorker() = default;
On 2016/11/04 17:34:57, maxbogue wrote:
> Is there some reason "= default;" is preferable to "{}"?

For copy/move constructor yes, but not for default constructor. Changed it back
to {}.

[Nicolas Zea, 2016-11-04 18:00:00]

lgtm

[fdoray, 2016-11-04 18:05:18]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=653916
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=650723
==========

[sky, 2016-11-04 20:23:53]

LGTM

[Ilya Sherman, 2016-11-05 01:35:42]

LGTM

[fdoray, 2016-11-07 15:46:53]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-07 15:46:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2471183003/#ps60001
(title: "CR maxbogue #23")

[commit-bot: I haz the power, 2016-11-07 15:47:08]

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, 2016-11-07 16:35:32]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=650723
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=650723
==========

[commit-bot: I haz the power, 2016-11-07 16:35:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-07 16:43:03]

Description was changed from

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=650723
==========

to

==========
Do not observe MessageLoop destruction from ModelSafeWorker.

BrowserThreads are being migrated to TaskScheduler which doesn't
support MessageLoop destruction observers. Destruction observers
currently serve two purposes in ModelSafeWorker:

1. Set a "stopped" flag to true. When this flag is true,
DoWorkAndWaitUntilDone() returns CANNOT_DO_WORK immediately.

-> This is useless because all DoWorkAndWaitUntilDoneImpl()
   implementations already return CANNOT_DO_WORK when they
   are unable to schedule a task.

   For example, in
   BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl():

    // Note: PostTask returns false once the MessageLoop
    // running on the BrowserThread is destroyed.
    if (!runner_->PostTask(
            FROM_HERE,
            base::Bind(...))) {
      DLOG(WARNING) << "Failed to post task to runner " << runner_;
      error = CANNOT_DO_WORK;
      return error;
    }

2. Notify SyncBackendRegistrar that the MessageLoop was
   destroyed so that it can move the ModelSafeWorker to an
   inactive list. When all ModelSafeWorkers have been moved to
   the inactive list, SyncBackendRegistrar deletes itself.

-> There is no reason to keep SyncBackendRegistrar alive
   after both SyncManager and SyncBackendHostImpl have cleared
   their pointer/unique_ptr to it. With this CL,
   SyncBackendRegistrar is deleted by a DeleteSoon task posted
   from SyncBackendHostImpl::Shutdown().

-> Deleting SyncBackendRegistrar causes the ModelSafeWorker
   references kept in |stopped_workers_| to be released. This
   is not dangerous since any object that still needs to use
   a ModelSafeWorker should have its own reference to it.

BUG=650723

Committed: https://crrev.com/ea7a7eca75b39bfc9cbf72aed8a4dffa1d77f1e2
Cr-Commit-Position: refs/heads/master@{#430293}
==========

[commit-bot: I haz the power, 2016-11-07 16:43:04]

Patchset 4 (id:??) landed as
https://crrev.com/ea7a7eca75b39bfc9cbf72aed8a4dffa1d77f1e2
Cr-Commit-Position: refs/heads/master@{#430293}