Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(540)

Unified Diff: sync/internal_api/public/engine/model_safe_worker.cc

Issue 637413003: Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Fixed issues with the test Created 6 years, 2 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: sync/internal_api/public/engine/model_safe_worker.cc
diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index eb3fc1f1c3ec286c961e743b23b05e77a7eae358..15f4d4cc897f0b4456065a23a4c07ead5aa26f32 100644
--- a/sync/internal_api/public/engine/model_safe_worker.cc
+++ b/sync/internal_api/public/engine/model_safe_worker.cc
@@ -75,8 +75,8 @@ ModelSafeWorker::ModelSafeWorker(WorkerLoopDestructionObserver* observer)
: stopped_(false),
work_done_or_stopped_(false, false),
observer_(observer),
- working_loop_(NULL),
- working_loop_set_wait_(true, false) {}
+ working_loop_(NULL) {
+}
ModelSafeWorker::~ModelSafeWorker() {}
@@ -135,26 +135,44 @@ void ModelSafeWorker::WillDestroyCurrentMessageLoop() {
void ModelSafeWorker::SetWorkingLoopToCurrent() {
base::AutoLock l(working_loop_lock_);
DCHECK(!working_loop_);
- working_loop_ = base::MessageLoop::current();
- working_loop_set_wait_.Signal();
+
+ if (unregister_done_callback_.is_null()) {
+ // Normal case - UnregisterForLoopDestruction hasn't been
+ // called yet (which is possible because UnregisterForLoopDestruction
Nicolas Zea 2014/10/17 00:04:02 The comment that this is possible "because Unregis
stanisc 2014/10/17 06:58:02 OK. Clarified/simplified these two comments.
+ // is a direct call while this method comes from a posted call).
+ // Add the observer and remember the message loop to be used later
+ // to post the unregister callback from UnregisterForLoopDestructionAsync.
+ base::MessageLoop::current()->AddDestructionObserver(this);
+ working_loop_ = base::MessageLoop::current();
+ } else {
+ // Rare case which is possible when the model type thread remains
+ // blocked for the entire session and UnregisterForLoopDestruction ends
+ // up being called before this method.
+ // In this case we skip the destruction observer registration
+ // and just invoke the callback postponed at UnregisterForLoopDestruction.
+ DCHECK(stopped_);
+ unregister_done_callback_.Run(GetModelSafeGroup());
+ }
}
void ModelSafeWorker::UnregisterForLoopDestruction(
base::Callback<void(ModelSafeGroup)> unregister_done_callback) {
- // Ok to wait until |working_loop_| is set because this is called on sync
- // loop.
- working_loop_set_wait_.Wait();
-
- {
- base::AutoLock l(working_loop_lock_);
- if (working_loop_ != NULL) {
- // Should be called on sync loop.
- DCHECK_NE(base::MessageLoop::current(), working_loop_);
- working_loop_->PostTask(
- FROM_HERE,
- base::Bind(&ModelSafeWorker::UnregisterForLoopDestructionAsync,
- this, unregister_done_callback));
- }
+ base::AutoLock l(working_loop_lock_);
+ if (working_loop_ != NULL) {
+ // Normal case - observer registratio has been already done.
Nicolas Zea 2014/10/17 00:04:01 nit: registratio -> registration
stanisc 2014/10/17 06:58:02 Done.
+ // Delegate to the sync thread to do the actual unregistration in
+ // UnregisterForLoopDestructionAsync.
+ DCHECK_NE(base::MessageLoop::current(), working_loop_);
+ working_loop_->PostTask(
+ FROM_HERE,
+ base::Bind(&ModelSafeWorker::UnregisterForLoopDestructionAsync,
+ this,
+ unregister_done_callback));
+ } else {
+ // The working loop is still unknown, probably because the model type
+ // thread is blocked. Store the callback to be called from
+ // SetWorkingLoopToCurrent.
+ unregister_done_callback_ = unregister_done_callback;
}
}

Powered by Google App Engine
This is Rietveld 408576698