[Sync] Avoid ObserverListThreadSafe in ChromeSyncNotificationBridge

chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc

Index: chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
diff --git a/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc b/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
index ffd42fd6eea1e76ee1bbe028c166f802de0ecf31..39f5f2e535da57c4bd165f7a066b9d6fe3fa920c 100644
--- a/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
+++ b/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
@@ -5,8 +5,11 @@
 #include "chrome/browser/sync/glue/chrome_sync_notification_bridge.h"
 
 #include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
+#include "base/sequenced_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread.h"
@@ -17,7 +20,6 @@
 #include "content/public/test/test_browser_thread.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base/model_type_payload_map.h"
-#include "sync/notifier/mock_sync_notifier_observer.h"
 #include "sync/notifier/sync_notifier_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,24 +36,28 @@ using content::BrowserThread;
 // ModelTypePayloadMap. ReceivedProperNotification() will return true only
 // if the observer has received a notification with the proper source and
 // payload.
-// Note: Because this object lives on the IO thread, we use a fake (vs a mock)
-// so we don't have to worry about possible thread safety issues within
-// GTest/GMock.
-class FakeSyncNotifierObserverIO
-    : public syncer::SyncNotifierObserver {
+// Note: Because this object lives on the sync thread, we use a fake
+// (vs a mock) so we don't have to worry about possible thread safety
+// issues within GTest/GMock.
+class FakeSyncNotifierObserver : public syncer::SyncNotifierObserver {
  public:
-  FakeSyncNotifierObserverIO(
+  FakeSyncNotifierObserver(
+      const scoped_refptr<base::SequencedTaskRunner>& sync_task_runner,
       ChromeSyncNotificationBridge* bridge,
-      const syncer::ModelTypePayloadMap& expected_payloads)
-      : bridge_(bridge),
+      const syncer::ModelTypePayloadMap& expected_payloads,
+      syncer::IncomingNotificationSource expected_source)
+      : sync_task_runner_(sync_task_runner),
+        bridge_(bridge),
         received_improper_notification_(false),
         notification_count_(0),
-        expected_payloads_(expected_payloads) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+        expected_payloads_(expected_payloads),
+        expected_source_(expected_source) {
+    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
     bridge_->AddObserver(this);
   }
-  virtual ~FakeSyncNotifierObserverIO() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  virtual ~FakeSyncNotifierObserver() {
+    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
     bridge_->RemoveObserver(this);
   }
 
@@ -59,14 +65,14 @@ class FakeSyncNotifierObserverIO
   virtual void OnIncomingNotification(
       const syncer::ModelTypePayloadMap& type_payloads,
       syncer::IncomingNotificationSource source) OVERRIDE {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
     notification_count_++;
-    if (source != syncer::LOCAL_NOTIFICATION) {
-      LOG(ERROR) << "Received notification with wrong source.";
+    if (source != expected_source_) {
+      LOG(ERROR) << "Received notification with wrong source";
       received_improper_notification_ = true;
     }
     if (expected_payloads_ != type_payloads) {
-      LOG(ERROR) << "Received wrong payload.";
+      LOG(ERROR) << "Received wrong payload";
       received_improper_notification_ = true;
     }
   }
@@ -79,89 +85,103 @@ class FakeSyncNotifierObserverIO
   }
 
   bool ReceivedProperNotification() const {
+    DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
     return (notification_count_ == 1) && !received_improper_notification_;
   }
 
  private:
-  ChromeSyncNotificationBridge* bridge_;
+  const scoped_refptr<base::SequencedTaskRunner> sync_task_runner_;
+  ChromeSyncNotificationBridge* const bridge_;
   bool received_improper_notification_;
   size_t notification_count_;
-  syncer::ModelTypePayloadMap expected_payloads_;
+  const syncer::ModelTypePayloadMap expected_payloads_;
+  const syncer::IncomingNotificationSource expected_source_;
 };
 
 class ChromeSyncNotificationBridgeTest : public testing::Test {
  public:
   ChromeSyncNotificationBridgeTest()
       : ui_thread_(BrowserThread::UI, &ui_loop_),

[Nicolas Zea, 2012/07/20 20:01:37]

are ui thread and ui loop both still needed? I don't think you post tasks to the
UI thread anymore, so you just need to ensure the "on UI thread" dcheck's pass
right?

[akalin, 2012/07/20 20:19:54]

Removed ui_loop_.  Also made all the member variables private.

-        io_thread_(BrowserThread::IO),
-        io_observer_(NULL),
-        io_observer_notification_failure_(false),
-        bridge_(&mock_profile_),
-        done_(true, false) {
-    io_thread_.StartIOThread();
-  }
+        sync_thread_("Sync thread"),
+        sync_observer_(NULL),
+        sync_observer_notification_failure_(false),
+        done_(true, false) {}
+
   virtual ~ChromeSyncNotificationBridgeTest() {}
 
  protected:
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(sync_thread_.Start());
+    bridge_.reset(
+        new ChromeSyncNotificationBridge(
+            &mock_profile_, sync_thread_.message_loop_proxy()));
+  }
+
   virtual void TearDown() OVERRIDE {
-    io_thread_.Stop();
+    sync_thread_.Stop();
+    // Must be reset only after the sync thread is stopped.
+    bridge_.reset();
     ui_loop_.RunAllPending();
-    EXPECT_EQ(NULL, io_observer_);
-    if (io_observer_notification_failure_)
-      ADD_FAILURE() << "IO Observer did not receive proper notification.";
+    EXPECT_EQ(NULL, sync_observer_);
+    if (sync_observer_notification_failure_)
+      ADD_FAILURE() << "Sync Observer did not receive proper notification.";
   }
 
-  void VerifyAndDestroyObserverOnIOThread() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    if (!io_observer_) {
-      io_observer_notification_failure_ = true;
+  void VerifyAndDestroyObserverOnSyncThread() {
+    DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
+    if (!sync_observer_) {
+      sync_observer_notification_failure_ = true;
     } else {
-      io_observer_notification_failure_ =
-          !io_observer_->ReceivedProperNotification();
-      delete io_observer_;
-      io_observer_ = NULL;
+      sync_observer_notification_failure_ =
+          !sync_observer_->ReceivedProperNotification();
+      delete sync_observer_;
+      sync_observer_ = NULL;
     }
   }
 
   void VerifyAndDestroyObserver() {
-    ASSERT_TRUE(BrowserThread::PostTask(
-        BrowserThread::IO,
+    ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
         base::Bind(&ChromeSyncNotificationBridgeTest::
-                       VerifyAndDestroyObserverOnIOThread,
+                       VerifyAndDestroyObserverOnSyncThread,
                    base::Unretained(this))));

[Nicolas Zea, 2012/07/20 20:01:37]

does this need a BlockForSyncThread after it? I suppose the sync_thread_.Stop()
in Teardown might take care of it, but I think it makes sense to have one here
anyways.

[akalin, 2012/07/20 20:19:54]

Done.

   }
 
-  void CreateObserverOnIOThread(
-      syncer::ModelTypePayloadMap expected_payloads) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    io_observer_ = new FakeSyncNotifierObserverIO(&bridge_,
-                                                  expected_payloads);
+  void CreateObserverOnSyncThread(
+      syncer::ModelTypePayloadMap expected_payloads,
+      syncer::IncomingNotificationSource expected_source) {
+    DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
+    sync_observer_ = new FakeSyncNotifierObserver(
+        sync_thread_.message_loop_proxy(),
+        bridge_.get(),
+        expected_payloads,
+        expected_source);
   }
 
-  void CreateObserverWithExpectedPayload(
-      syncer::ModelTypePayloadMap expected_payloads) {
-    ASSERT_TRUE(BrowserThread::PostTask(
-        BrowserThread::IO,
+  void CreateObserverWithExpectations(
+      syncer::ModelTypePayloadMap expected_payloads,
+      syncer::IncomingNotificationSource expected_source) {
+    ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
-        base::Bind(&ChromeSyncNotificationBridgeTest::CreateObserverOnIOThread,
-                   base::Unretained(this),
-                   expected_payloads)));
-    BlockForIOThread();
+        base::Bind(
+            &ChromeSyncNotificationBridgeTest::CreateObserverOnSyncThread,
+            base::Unretained(this),
+            expected_payloads,
+            expected_source)));
+    BlockForSyncThread();
   }
 
-  void SignalOnIOThread() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  void SignalOnSyncThread() {
+    DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
     done_.Signal();
   }
 
-  void BlockForIOThread() {
+  void BlockForSyncThread() {
     done_.Reset();
-    BrowserThread::PostTask(
-        BrowserThread::IO,
+    ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
-        base::Bind(&ChromeSyncNotificationBridgeTest::SignalOnIOThread,
-                   base::Unretained(this)));
+        base::Bind(&ChromeSyncNotificationBridgeTest::SignalOnSyncThread,
+                   base::Unretained(this))));
     done_.TimedWait(TestTimeouts::action_timeout());
     if (!done_.IsSignaled())
       ADD_FAILURE() << "Timed out waiting for IO thread.";
@@ -178,101 +198,66 @@ class ChromeSyncNotificationBridgeTest : public testing::Test {
 
   MessageLoop ui_loop_;
   content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread io_thread_;
+  base::Thread sync_thread_;
   NiceMock<ProfileMock> mock_profile_;
-  // Created/used/destroyed on I/O thread.
-  FakeSyncNotifierObserverIO* io_observer_;
-  bool io_observer_notification_failure_;
-  ChromeSyncNotificationBridge bridge_;
+  // Created/used/destroyed on sync thread.
+  FakeSyncNotifierObserver* sync_observer_;
+  bool sync_observer_notification_failure_;
+  scoped_ptr<ChromeSyncNotificationBridge> bridge_;
   base::WaitableEvent done_;
 };
 
-// Adds an observer on the UI thread, triggers a local refresh notification, and
-// ensures the bridge posts a LOCAL_NOTIFICATION with the proper payload to it.
+// Adds an observer on the sync thread, triggers a local refresh
+// notification, and ensures the bridge posts a LOCAL_NOTIFICATION
+// with the proper payload to it.
 TEST_F(ChromeSyncNotificationBridgeTest, LocalNotification) {
   syncer::ModelTypePayloadMap payload_map;
   payload_map[syncer::SESSIONS] = "";
-  StrictMock<syncer::MockSyncNotifierObserver> observer;
-  EXPECT_CALL(observer,
-              OnIncomingNotification(payload_map,
-                                     syncer::LOCAL_NOTIFICATION));
-  bridge_.AddObserver(&observer);
+  CreateObserverWithExpectations(payload_map, syncer::LOCAL_NOTIFICATION);
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
                              payload_map);
-  ui_loop_.RunAllPending();
-  Mock::VerifyAndClearExpectations(&observer);
+  VerifyAndDestroyObserver();
 }
 
-// Adds an observer on the UI thread, triggers a remote refresh notification,
-// and ensures the bridge posts a REMOTE_NOTIFICATION with the proper payload
-// to it.
+// Adds an observer on the sync thread, triggers a remote refresh
+// notification, and ensures the bridge posts a REMOTE_NOTIFICATION
+// with the proper payload to it.
 TEST_F(ChromeSyncNotificationBridgeTest, RemoteNotification) {
   syncer::ModelTypePayloadMap payload_map;
-  payload_map[syncer::BOOKMARKS] = "";
-  StrictMock<syncer::MockSyncNotifierObserver> observer;
-  EXPECT_CALL(observer,
-              OnIncomingNotification(payload_map,
-                                     syncer::REMOTE_NOTIFICATION));
-  bridge_.AddObserver(&observer);
+  payload_map[syncer::SESSIONS] = "";
+  CreateObserverWithExpectations(payload_map, syncer::REMOTE_NOTIFICATION);
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_REMOTE,
                              payload_map);
-  ui_loop_.RunAllPending();
-  Mock::VerifyAndClearExpectations(&observer);
+  VerifyAndDestroyObserver();
 }
 
-// Adds an observer on the UI thread, triggers a local refresh notification
-// with empty payload map and ensures the bridge posts a
+// Adds an observer on the sync thread, triggers a local refresh
+// notification with empty payload map and ensures the bridge posts a
 // LOCAL_NOTIFICATION with the proper payload to it.
 TEST_F(ChromeSyncNotificationBridgeTest, LocalNotificationEmptyPayloadMap) {
   const syncer::ModelTypeSet enabled_types(
       syncer::BOOKMARKS, syncer::PASSWORDS);
   const syncer::ModelTypePayloadMap enabled_types_payload_map =
       syncer::ModelTypePayloadMapFromEnumSet(enabled_types, std::string());
-
-  StrictMock<syncer::MockSyncNotifierObserver> observer;
-  EXPECT_CALL(observer,
-              OnIncomingNotification(enabled_types_payload_map,
-                                     syncer::LOCAL_NOTIFICATION));
-  bridge_.AddObserver(&observer);
-  // Set enabled types on the bridge.
-  bridge_.UpdateEnabledTypes(enabled_types);
+  CreateObserverWithExpectations(
+      enabled_types_payload_map, syncer::LOCAL_NOTIFICATION);
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
-                             syncer::ModelTypePayloadMap());
-  ui_loop_.RunAllPending();
-  Mock::VerifyAndClearExpectations(&observer);
+                             enabled_types_payload_map);
+  VerifyAndDestroyObserver();
 }
 
-// Adds an observer on the UI thread, triggers a remote refresh notification
-// with empty payload map and ensures the bridge posts a
+// Adds an observer on the sync thread, triggers a remote refresh
+// notification with empty payload map and ensures the bridge posts a
 // REMOTE_NOTIFICATION with the proper payload to it.
 TEST_F(ChromeSyncNotificationBridgeTest, RemoteNotificationEmptyPayloadMap) {
   const syncer::ModelTypeSet enabled_types(
       syncer::BOOKMARKS, syncer::TYPED_URLS);
   const syncer::ModelTypePayloadMap enabled_types_payload_map =
       syncer::ModelTypePayloadMapFromEnumSet(enabled_types, std::string());
-
-  StrictMock<syncer::MockSyncNotifierObserver> observer;
-  EXPECT_CALL(observer,
-              OnIncomingNotification(enabled_types_payload_map,
-                                     syncer::REMOTE_NOTIFICATION));
-  bridge_.AddObserver(&observer);
-  // Set enabled types on the bridge.
-  bridge_.UpdateEnabledTypes(enabled_types);
+  CreateObserverWithExpectations(
+      enabled_types_payload_map, syncer::REMOTE_NOTIFICATION);
   TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_REMOTE,
-                             syncer::ModelTypePayloadMap());
-  ui_loop_.RunAllPending();
-  Mock::VerifyAndClearExpectations(&observer);
-}
-
-// Adds an observer on the I/O thread. Then triggers a refresh notification on
-// the UI thread. We finally verify the proper notification was received by the
-// observer and destroy it on the I/O thread.
-TEST_F(ChromeSyncNotificationBridgeTest, BasicThreaded) {
-  syncer::ModelTypePayloadMap payload_map;
-  payload_map[syncer::SESSIONS] = "";
-  CreateObserverWithExpectedPayload(payload_map);
-  TriggerRefreshNotification(chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
-                             payload_map);
+                             enabled_types_payload_map);
   VerifyAndDestroyObserver();
 }