Use a ConditionVariable in place of WaitableEvent in BookmarkModelWorker.

This is to ensure future changes to the code don't introduce harmful races
that could arise in presence of compiler optimizations due to the lack of
a barrier permitting instruction re-ordering.  The theoretical race that
exists now isn't causing any real malfunction today, but is causing
ThreadSanitizer warnings.

BUG=25915
TEST=BookmarkModelWorkerTest

Review URL: http://codereview.chromium.org/488012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34558 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 23 insertions, 18 deletions

diff --git a/chrome/browser/sync/glue/bookmark_model_worker.cc b/chrome/browser/sync/glue/bookmark_model_worker.cc
index d358bfd..423aa2e 100644
--- a/chrome/browser/sync/glue/bookmark_model_worker.cc
+++ b/chrome/browser/sync/glue/bookmark_model_worker.cc
@@ -31,7 +31,7 @@ void BookmarkModelWorker::CallDoWorkFromModelSafeThreadAndWait(
     // We lock only to avoid PostTask'ing a NULL pending_work_ (because it
     // could get Run() in Stop() and call OnTaskCompleted before we post).
     // The task is owned by the message loop as per usual.
-    AutoLock lock(pending_work_lock_);
+    AutoLock lock(lock_);
     DCHECK(!pending_work_);
     pending_work_ = new CallDoWorkAndSignalTask(visitor, &work_done, this);
     bookmark_model_loop_->PostTask(FROM_HERE, pending_work_);
@@ -45,11 +45,12 @@ BookmarkModelWorker::~BookmarkModelWorker() {
 }
 
 void BookmarkModelWorker::OnSyncerShutdownComplete() {
+  AutoLock lock(lock_);
   // The SyncerThread has terminated and we are no longer needed by syncapi.
   // The UI loop initiated shutdown and is (or will be) waiting in Stop().
   // We could either be WORKING or RUNNING_MANUAL_SHUTDOWN_PUMP, depending
   // on where we timeslice the UI thread in Stop; but we can't be STOPPED,
-  // because that would imply NotifySyncapiShutdownComplete already signaled.
+  // because that would imply OnSyncerShutdownComplete already signaled.
   DCHECK_NE(state_, STOPPED);
 
   syncapi_has_shutdown_ = true;
@@ -58,26 +59,22 @@ void BookmarkModelWorker::OnSyncerShutdownComplete() {
 
 void BookmarkModelWorker::Stop() {
   DCHECK_EQ(MessageLoop::current(), bookmark_model_loop_);
+
+  AutoLock lock(lock_);
   DCHECK_EQ(state_, WORKING);
 
   // We're on our own now, the beloved UI MessageLoop is no longer running.
   // Any tasks scheduled or to be scheduled on the UI MessageLoop will not run.
   state_ = RUNNING_MANUAL_SHUTDOWN_PUMP;
 
-  // Drain any final task manually until the SyncerThread tells us it has
-  // totally finished. Note we use a 'while' loop and not 'if'. The main subtle
-  // reason for this is that syncapi_event could be signaled the first time we
-  // come through due to an old CallDoWork call, and we need to keep looping
-  // until the SyncerThread either calls it again or tells us it is done. There
-  // should only ever be 0 or 1 tasks Run() here, however.
+  // Drain any final tasks manually until the SyncerThread tells us it has
+  // totally finished. There should only ever be 0 or 1 tasks Run() here.
   while (!syncapi_has_shutdown_) {
-    {
-      AutoLock lock(pending_work_lock_);
-      if (pending_work_)
-        pending_work_->Run();
-    }
-    syncapi_event_.Wait();  // Signaled either by new task, or SyncerThread
-                            // termination.
+    if (pending_work_)
+      pending_work_->Run();  // OnTaskCompleted will set pending_work_ to NULL.
+
+    // Wait for either a new task or SyncerThread termination.
+    syncapi_event_.Wait();
   }
 
   state_ = STOPPED;
diff --git a/chrome/browser/sync/glue/bookmark_model_worker.h b/chrome/browser/sync/glue/bookmark_model_worker.h
index f2c80a1..e6491f3 100644
--- a/chrome/browser/sync/glue/bookmark_model_worker.h
+++ b/chrome/browser/sync/glue/bookmark_model_worker.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_SYNC_GLUE_BOOKMARK_MODEL_WORKER_H_
 #define CHROME_BROWSER_SYNC_GLUE_BOOKMARK_MODEL_WORKER_H_
 
+#include "base/condition_variable.h"
 #include "base/lock.h"
 #include "base/task.h"
 #include "base/waitable_event.h"
@@ -30,7 +31,7 @@ class BookmarkModelWorker
         pending_work_(NULL),
         syncapi_has_shutdown_(false),
         bookmark_model_loop_(bookmark_model_loop),
-        syncapi_event_(false, false) {
+        syncapi_event_(&lock_) {
   }
   virtual ~BookmarkModelWorker();
 
@@ -105,7 +106,6 @@ class BookmarkModelWorker
   // We keep a reference to any task we have scheduled so we can gracefully
   // force them to run if the syncer is trying to shutdown.
   Task* pending_work_;
-  Lock pending_work_lock_;
 
   // Set by the SyncCoreThread when Syncapi shutdown has completed and the
   // SyncerThread has terminated, so no more work will be scheduled. Read by
@@ -115,12 +115,20 @@ class BookmarkModelWorker
   // The BookmarkModel's home-sweet-home MessageLoop.
   MessageLoop* const bookmark_model_loop_;
 
+  // We use a Lock for all data members and a ConditionVariable to synchronize.
+  // We do this instead of using a WaitableEvent and a bool condition in order
+  // to guard against races that could arise due to the fact that the lack of a
+  // barrier permits instructions to be reordered by compiler optimizations.
+  // Possible or not, that route makes for very fragile code due to existence
+  // of theoretical races.
+  Lock lock_;
+
   // Used as a barrier at shutdown to ensure the SyncerThread terminates before
   // we allow the UI thread to return from Stop(). This gets signalled whenever
   // one of two events occur: a new pending_work_ task was scheduled, or the
   // SyncerThread has terminated. We only care about (1) when we are in Stop(),
   // because we have to manually Run() the task.
-  base::WaitableEvent syncapi_event_;
+  ConditionVariable syncapi_event_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelWorker);
 };