Do not observe MessageLoop destruction from ModelSafeWorker.

components/sync/driver/glue/sync_backend_host_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync/driver/glue/sync_backend_host_impl.h"
 
 #include <utility>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/invalidation/public/invalidation_service.h"
 #include "components/invalidation/public/object_id_invalidation_map.h"
 #include "components/signin/core/browser/signin_client.h"
 #include "components/sync/base/experiments.h"
@@ skipping 215 matching lines @@
   core_->ShutdownOnUIThread();
 }
 
 std::unique_ptr<base::Thread> SyncBackendHostImpl::Shutdown(
     ShutdownReason reason) {
   // StopSyncingForShutdown() (which nulls out |frontend_|) should be
   // called first.
   DCHECK(!frontend_);
   DCHECK(registrar_->sync_thread()->IsRunning());
 
-  bool sync_thread_claimed = (reason != BROWSER_SHUTDOWN);
-
   if (invalidation_handler_registered_) {
     if (reason == DISABLE_SYNC) {
       UnregisterInvalidationIds();
     }
     invalidator_->UnregisterInvalidationHandler(this);
     invalidator_ = nullptr;
   }
   invalidation_handler_registered_ = false;
 
   model_type_connector_.reset();
 
-  // Shut down and destroy sync manager.
+  // Shut down and destroy SyncManager. SyncManager holds a pointer to
+  // |registrar_| so its destruction must be sequenced before the destruction of
+  // |registrar_|.
   registrar_->sync_thread()->task_runner()->PostTask(
       FROM_HERE,
       base::Bind(&SyncBackendHostCore::DoShutdown, core_, reason));
   core_ = nullptr;
 
-  // Worker cleanup.
-  SyncBackendRegistrar* detached_registrar = registrar_.release();
-  detached_registrar->sync_thread()->task_runner()->PostTask(
-      FROM_HERE, base::Bind(&SyncBackendRegistrar::Shutdown,
-                            base::Unretained(detached_registrar)));
+  // Destroy |registrar_|.
+  std::unique_ptr<base::Thread> released_sync_thread =
+      registrar_->ReleaseSyncThread();
+  released_sync_thread->task_runner()->DeleteSoon(FROM_HERE,
+                                                  registrar_.release());
 
-  if (sync_thread_claimed)
-    return detached_registrar->ReleaseSyncThread();
-  else
-    return std::unique_ptr<base::Thread>();
+  if (reason == BROWSER_SHUTDOWN)
+    return nullptr;
+  return released_sync_thread;

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

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]

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]

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

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

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

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]

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

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

 }
 
 void SyncBackendHostImpl::UnregisterInvalidationIds() {
   if (invalidation_handler_registered_) {
     CHECK(invalidator_->UpdateRegisteredInvalidationIds(this, ObjectIdSet()));
   }
 }
 
 ModelTypeSet SyncBackendHostImpl::ConfigureDataTypes(
     ConfigureReason reason,
@@ skipping 534 matching lines @@
     const SyncManager::ClearServerDataCallback& frontend_callback) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
   frontend_callback.Run();
 }
 
 }  // namespace syncer
 
 #undef SDVLOG
 
 #undef SLOG