sync: Implement disabling of non blocking types

Adds logic to have the model-thread NonBlockingDataTypeProcessor send
a message to the sync-thread SyncCore when it receives notifcation from
the ui-thread NonBlockingDataTypeController that it should stop syncing.
This message will allow the sync thread to stop requeting updates and
commits on behalf of the now-disabled type.

Fixes the handling of a race in the NonBlockingDataTypeProcessor.  The
race is as follows:
1. NBDTP receives a request to enable sync from the UI thread, and sends
   a connection request to the sync thread via SyncCoreProxy.
2. NBDTP receives a request to disable sync from the UI thread.  It
   updates its internal state accordingly.
3. NBDTP receives the connection OK response from the sync thread,
   which was genrated in response to its request in step 1.

Previously, the processor would wrongly set itself to the enable state
in step 3.  The fix is to use a new WeakPtrFactory and invalidate the
pointers it has issued in step 2 in order to prevent the response seen
in step 3 from being run.

Adds some more tests for this scenario and more.  The test framework had
to be made a bit more complicated in order to handle these tests, but
I think the extra complexity is worth it.  I don't know of any other
way to reliably defend against these race cases.

BUG=351005

Review URL: https://codereview.chromium.org/272323002

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

2 files changed, 202 insertions, 21 deletions

diff --git a/components/sync_driver/non_blocking_data_type_controller.cc b/components/sync_driver/non_blocking_data_type_controller.cc
index e2e0366..a1ee251 100644
--- a/components/sync_driver/non_blocking_data_type_controller.cc
+++ b/components/sync_driver/non_blocking_data_type_controller.cc
@@ -84,11 +84,10 @@ void NonBlockingDataTypeController::SendEnableSignal() {
   DCHECK_EQ(ENABLED, GetDesiredState());
   DVLOG(1) << "Enabling non-blocking sync type " << ModelTypeToString(type_);
 
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&syncer::NonBlockingTypeProcessor::Enable,
-                 processor_,
-                 base::Owned(proxy_->Clone().release())));
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&syncer::NonBlockingTypeProcessor::Enable,
+                                    processor_,
+                                    base::Passed(proxy_->Clone())));
   current_state_ = ENABLED;
 }
 
diff --git a/components/sync_driver/non_blocking_data_type_controller_unittest.cc b/components/sync_driver/non_blocking_data_type_controller_unittest.cc
index 43c4fa4..b1c8dc6 100644
--- a/components/sync_driver/non_blocking_data_type_controller_unittest.cc
+++ b/components/sync_driver/non_blocking_data_type_controller_unittest.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <list>
+
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
@@ -14,22 +18,74 @@
 
 namespace {
 
+// A class that pretends to be the sync backend.
+class MockSyncCore {
+ public:
+  void Connect(
+      syncer::ModelType type,
+      const scoped_refptr<base::SingleThreadTaskRunner>& model_task_runner,
+      const base::WeakPtr<syncer::NonBlockingTypeProcessor>& type_processor) {
+    enabled_types_.Put(type);
+    model_task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&syncer::NonBlockingTypeProcessor::OnConnect,
+                   type_processor,
+                   base::WeakPtr<syncer::NonBlockingTypeProcessorCore>(),
+                   scoped_refptr<base::SequencedTaskRunner>()));
+  }
+
+  void Disconnect(syncer::ModelType type) {
+    DCHECK(enabled_types_.Has(type));
+    enabled_types_.Remove(type);
+  }
+
+ private:
+  std::list<base::Closure> tasks_;
+  syncer::ModelTypeSet enabled_types_;
+};
+
+// A proxy to the MockSyncCore that implements SyncCoreProxy.
 class MockSyncCoreProxy : public syncer::SyncCoreProxy {
  public:
-  MockSyncCoreProxy() {}
+  MockSyncCoreProxy(
+      MockSyncCore* sync_core,
+      const scoped_refptr<base::TestSimpleTaskRunner>& model_task_runner,
+      const scoped_refptr<base::TestSimpleTaskRunner>& sync_task_runner)
+      : mock_sync_core_(sync_core),
+        model_task_runner_(model_task_runner),
+        sync_task_runner_(sync_task_runner) {}
   virtual ~MockSyncCoreProxy() {}
 
   virtual void ConnectTypeToCore(
       syncer::ModelType type,
       base::WeakPtr<syncer::NonBlockingTypeProcessor> type_processor) OVERRIDE {
-    type_processor->OnConnect(
-        base::WeakPtr<syncer::NonBlockingTypeProcessorCore>(),
-        scoped_refptr<base::SequencedTaskRunner>());
+    // Normally we'd use MessageLoopProxy::current() as the TaskRunner argument
+    // to Connect().  That won't work here in this test, so we use the
+    // model_task_runner_ that was injected for this purpose instead.
+    sync_task_runner_->PostTask(FROM_HERE,
+                                base::Bind(&MockSyncCore::Connect,
+                                           base::Unretained(mock_sync_core_),
+                                           type,
+                                           model_task_runner_,
+                                           type_processor));
+  }
+
+  virtual void Disconnect(syncer::ModelType type) OVERRIDE {
+    sync_task_runner_->PostTask(FROM_HERE,
+                                base::Bind(&MockSyncCore::Disconnect,
+                                           base::Unretained(mock_sync_core_),
+                                           type));
   }
 
   virtual scoped_ptr<SyncCoreProxy> Clone() const OVERRIDE {
-    return scoped_ptr<SyncCoreProxy>(new MockSyncCoreProxy());
+    return scoped_ptr<SyncCoreProxy>(new MockSyncCoreProxy(
+        mock_sync_core_, model_task_runner_, sync_task_runner_));
   }
+
+ private:
+  MockSyncCore* mock_sync_core_;
+  scoped_refptr<base::TestSimpleTaskRunner> model_task_runner_;
+  scoped_refptr<base::TestSimpleTaskRunner> sync_task_runner_;
 };
 
 class NonBlockingDataTypeControllerTest : public testing::Test {
@@ -37,48 +93,75 @@ class NonBlockingDataTypeControllerTest : public testing::Test {
   NonBlockingDataTypeControllerTest()
       : processor_(syncer::DICTIONARY),
         model_thread_(new base::TestSimpleTaskRunner()),
-        controller_(syncer::DICTIONARY, true) {}
+        sync_thread_(new base::TestSimpleTaskRunner()),
+        controller_(syncer::DICTIONARY, true),
+        mock_core_proxy_(&mock_sync_core_, model_thread_, sync_thread_),
+        auto_run_tasks_(true) {}
 
   virtual ~NonBlockingDataTypeControllerTest() {}
 
   // Connects the processor to the NonBlockingDataTypeController.
   void InitProcessor() {
-    controller_.InitializeProcessor(
-        model_thread_, processor_.AsWeakPtr());
-    RunQueuedTasks();
+    controller_.InitializeProcessor(model_thread_, processor_.AsWeakPtrForUI());
+    if (auto_run_tasks_) {
+      RunAllTasks();
+    }
   }
 
   // Connects the sync backend to the NonBlockingDataTypeController.
   void InitSync() {
     controller_.InitializeSyncCoreProxy(mock_core_proxy_.Clone());
-    RunQueuedTasks();
+    if (auto_run_tasks_) {
+      RunAllTasks();
+    }
   }
 
   // Disconnects the sync backend from the NonBlockingDataTypeController.
   void UninitializeSync() {
     controller_.ClearSyncCoreProxy();
-    RunQueuedTasks();
+    if (auto_run_tasks_) {
+      RunAllTasks();
+    }
   }
 
   // Toggles the user's preference for syncing this type.
   void SetIsPreferred(bool preferred) {
     controller_.SetIsPreferred(preferred);
-    RunQueuedTasks();
+    if (auto_run_tasks_) {
+      RunAllTasks();
+    }
+  }
+
+  // These threads can ping-pong for a bit so we run the model thread twice.
+  void RunAllTasks() {
+    RunQueuedModelThreadTasks();
+    RunQueuedSyncThreadTasks();
+    RunQueuedModelThreadTasks();
   }
 
   // The processor pretends to run tasks on a different thread.
   // This function runs any posted tasks.
-  void RunQueuedTasks() {
-    model_thread_->RunUntilIdle();
+  void RunQueuedModelThreadTasks() { model_thread_->RunUntilIdle(); }
+
+  // Processes any pending connect or disconnect requests and sends
+  // responses synchronously.
+  void RunQueuedSyncThreadTasks() { sync_thread_->RunUntilIdle(); }
+
+  void SetAutoRunTasks(bool auto_run_tasks) {
+    auto_run_tasks_ = auto_run_tasks;
   }
 
  protected:
-  MockSyncCoreProxy mock_core_proxy_;
-
   syncer::NonBlockingTypeProcessor processor_;
   scoped_refptr<base::TestSimpleTaskRunner> model_thread_;
+  scoped_refptr<base::TestSimpleTaskRunner> sync_thread_;
 
   browser_sync::NonBlockingDataTypeController controller_;
+
+  MockSyncCore mock_sync_core_;
+  MockSyncCoreProxy mock_core_proxy_;
+
+  bool auto_run_tasks_;
 };
 
 // Initialization when the user has disabled syncing for this type.
@@ -224,4 +307,103 @@ TEST_F(NonBlockingDataTypeControllerTest, TogglePreferenceWithoutBackend) {
   EXPECT_TRUE(processor_.IsPreferred());
 }
 
+// Turns off auto-task-running to test the effects of delaying a connection
+// response.
+//
+// This is mostly a test of the test framework.  It's not very interesting on
+// its own, but it provides a useful "control" against some of the more
+// complicated race tests below.
+TEST_F(NonBlockingDataTypeControllerTest, DelayedConnect) {
+  SetAutoRunTasks(false);
+
+  SetIsPreferred(true);
+  InitProcessor();
+  InitSync();
+
+  // Allow the model to emit the request.
+  RunQueuedModelThreadTasks();
+
+  // That should result in a request to connect, but it won't be
+  // executed right away.
+  EXPECT_FALSE(processor_.IsConnected());
+  EXPECT_TRUE(processor_.IsPreferred());
+
+  // Let the sync thread process the request and the model thread handle its
+  // response.
+  RunQueuedSyncThreadTasks();
+  RunQueuedModelThreadTasks();
+
+  EXPECT_TRUE(processor_.IsConnected());
+  EXPECT_TRUE(processor_.IsPreferred());
+}
+
+// Send Disable signal while a connection request is in progress.
+TEST_F(NonBlockingDataTypeControllerTest, DisableRacesWithOnConnect) {
+  SetAutoRunTasks(false);
+
+  SetIsPreferred(true);
+  InitProcessor();
+  InitSync();
+
+  // Allow the model to emit the request.
+  RunQueuedModelThreadTasks();
+
+  // That should result in a request to connect, but it won't be
+  // executed right away.
+  EXPECT_FALSE(processor_.IsConnected());
+  EXPECT_TRUE(processor_.IsPreferred());
+
+  // Send and execute a disable signal before the OnConnect callback returns.
+  SetIsPreferred(false);
+
+  // Now we let sync process the initial request and the disable request,
+  // both of which should be sitting in its queue.
+  RunQueuedSyncThreadTasks();
+
+  // Let the model thread process any responses received from the sync thread.
+  // A plausible error would be that the sync thread returns a "connection OK"
+  // message, and this message overrides the request to disable that arrived
+  // from the UI thread earlier.  We need to make sure that doesn't happen.
+  RunQueuedModelThreadTasks();
+
+  EXPECT_FALSE(processor_.IsPreferred());
+  EXPECT_FALSE(processor_.IsConnected());
+}
+
+// Send a request to enable, then disable, then re-enable the data type.
+//
+// To make it more interesting, we stall the sync thread until all three
+// requests have been passed to the model thread.
+TEST_F(NonBlockingDataTypeControllerTest, EnableDisableEnableRace) {
+  SetAutoRunTasks(false);
+
+  SetIsPreferred(true);
+  InitProcessor();
+  InitSync();
+  RunQueuedModelThreadTasks();
+
+  // That was the first enable.
+  EXPECT_FALSE(processor_.IsConnected());
+  EXPECT_TRUE(processor_.IsPreferred());
+
+  // Now disable.
+  SetIsPreferred(false);
+  RunQueuedModelThreadTasks();
+  EXPECT_FALSE(processor_.IsPreferred());
+
+  // And re-enable.
+  SetIsPreferred(true);
+  RunQueuedModelThreadTasks();
+  EXPECT_TRUE(processor_.IsPreferred());
+
+  // The sync thread has three messages related to those enables and
+  // disables sittin in its queue.  Let's allow it to process them.
+  RunQueuedSyncThreadTasks();
+
+  // Let the model thread process any messages from the sync thread.
+  RunQueuedModelThreadTasks();
+  EXPECT_TRUE(processor_.IsPreferred());
+  EXPECT_TRUE(processor_.IsConnected());
+}
+
 }  // namespace