Add locking around store_birthday to ensure atomic overwrites and avoid race

conditions.

BUG=50217
TEST=Remove suppressions in chrome/test/data/valgrind/sync_unit_tests.gtest-tsan.txt, then run sync_unit_tests with tsan to confirm no race conditions involving store_birthday.

Original patch by: zea@chromium.org
Original review: http://codereview.chromium.org/3333011/show

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

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

2 files changed, 14 insertions, 3 deletions

diff --git a/chrome/test/sync/engine/mock_connection_manager.cc b/chrome/test/sync/engine/mock_connection_manager.cc
index cb3f309..bfba668 100644
--- a/chrome/test/sync/engine/mock_connection_manager.cc
+++ b/chrome/test/sync/engine/mock_connection_manager.cc
@@ -105,8 +105,10 @@ bool MockConnectionManager::PostBufferToPath(const PostBufferParams* params,
 
   // Default to an ok connection.
   params->response->server_status = HttpResponse::SERVER_CONNECTION_OK;
-  response.set_store_birthday(store_birthday_);
-  if (post.has_store_birthday() && post.store_birthday() != store_birthday_) {
+  const string current_store_birthday = store_birthday();
+  response.set_store_birthday(current_store_birthday);
+  if (post.has_store_birthday() && post.store_birthday() !=
+      current_store_birthday) {
     response.set_error_code(ClientToServerResponse::NOT_MY_BIRTHDAY);
     response.set_error_message("Merry Unbirthday!");
     response.SerializeToString(params->buffer_out);
diff --git a/chrome/test/sync/engine/mock_connection_manager.h b/chrome/test/sync/engine/mock_connection_manager.h
index 858d6f0e..7d68844 100644
--- a/chrome/test/sync/engine/mock_connection_manager.h
+++ b/chrome/test/sync/engine/mock_connection_manager.h
@@ -185,6 +185,9 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
   }
 
   void set_store_birthday(string new_birthday) {
+    // Multiple threads can set store_birthday_ in our tests, need to lock it to
+    // ensure atomic read/writes and avoid race conditions.
+    AutoLock lock(store_birthday_lock_);
     store_birthday_ = new_birthday;
   }
 
@@ -209,7 +212,12 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
 
   void SetServerNotReachable();
 
-  std::string store_birthday() { return store_birthday_; }
+  // Const necessary to avoid any hidden copy-on-write issues that would break
+  // in multithreaded scenarios (see |set_store_birthday|).
+  const std::string& store_birthday() {
+    AutoLock lock(store_birthday_lock_);
+    return store_birthday_;
+  }
 
   // Locate the most recent update message for purpose of alteration.
   sync_pb::SyncEntity* GetMutableLastUpdate();
@@ -265,6 +273,7 @@ class MockConnectionManager : public browser_sync::ServerConnectionManager {
 
   // The store birthday we send to the client.
   string store_birthday_;
+  Lock store_birthday_lock_;
   bool store_birthday_sent_;
   bool client_stuck_;
   string commit_time_rename_prepended_string_;