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

13 files changed, 283 insertions, 53 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
diff --git a/sync/internal_api/non_blocking_type_processor.cc b/sync/internal_api/non_blocking_type_processor.cc
index d426443..870c296 100644
--- a/sync/internal_api/non_blocking_type_processor.cc
+++ b/sync/internal_api/non_blocking_type_processor.cc
@@ -11,10 +11,12 @@
 namespace syncer {
 
 NonBlockingTypeProcessor::NonBlockingTypeProcessor(ModelType type)
-  : type_(type),
-    is_preferred_(false),
-    is_connected_(false),
-    weak_ptr_factory_(this) {}
+    : type_(type),
+      is_preferred_(false),
+      is_connected_(false),
+      weak_ptr_factory_for_ui_(this),
+      weak_ptr_factory_for_sync_(this) {
+}
 
 NonBlockingTypeProcessor::~NonBlockingTypeProcessor() {
 }
@@ -34,10 +36,14 @@ ModelType NonBlockingTypeProcessor::GetModelType() const {
   return type_;
 }
 
-void NonBlockingTypeProcessor::Enable(SyncCoreProxy* core_proxy) {
+void NonBlockingTypeProcessor::Enable(
+    scoped_ptr<SyncCoreProxy> sync_core_proxy) {
   DCHECK(CalledOnValidThread());
+  DVLOG(1) << "Asked to enable " << ModelTypeToString(type_);
   is_preferred_ = true;
-  core_proxy->ConnectTypeToCore(GetModelType(), AsWeakPtr());
+  sync_core_proxy_ = sync_core_proxy.Pass();
+  sync_core_proxy_->ConnectTypeToCore(GetModelType(),
+                                      weak_ptr_factory_for_sync_.GetWeakPtr());
 }
 
 void NonBlockingTypeProcessor::Disable() {
@@ -48,20 +54,30 @@ void NonBlockingTypeProcessor::Disable() {
 
 void NonBlockingTypeProcessor::Disconnect() {
   DCHECK(CalledOnValidThread());
+  DVLOG(1) << "Asked to disconnect " << ModelTypeToString(type_);
   is_connected_ = false;
+
+  if (sync_core_proxy_) {
+    sync_core_proxy_->Disconnect(GetModelType());
+    sync_core_proxy_.reset();
+  }
+
+  weak_ptr_factory_for_sync_.InvalidateWeakPtrs();
   core_ = base::WeakPtr<NonBlockingTypeProcessorCore>();
   sync_thread_ = scoped_refptr<base::SequencedTaskRunner>();
 }
 
-base::WeakPtr<NonBlockingTypeProcessor> NonBlockingTypeProcessor::AsWeakPtr() {
+base::WeakPtr<NonBlockingTypeProcessor>
+NonBlockingTypeProcessor::AsWeakPtrForUI() {
   DCHECK(CalledOnValidThread());
-  return weak_ptr_factory_.GetWeakPtr();
+  return weak_ptr_factory_for_ui_.GetWeakPtr();
 }
 
 void NonBlockingTypeProcessor::OnConnect(
     base::WeakPtr<NonBlockingTypeProcessorCore> core,
     scoped_refptr<base::SequencedTaskRunner> sync_thread) {
   DCHECK(CalledOnValidThread());
+  DVLOG(1) << "Successfully connected " << ModelTypeToString(type_);
   is_connected_ = true;
   core_ = core;
   sync_thread_ = sync_thread;
diff --git a/sync/internal_api/public/non_blocking_type_processor.h b/sync/internal_api/public/non_blocking_type_processor.h
index 3745088..2db2c32 100644
--- a/sync/internal_api/public/non_blocking_type_processor.h
+++ b/sync/internal_api/public/non_blocking_type_processor.h
@@ -43,7 +43,7 @@ class SYNC_EXPORT_PRIVATE NonBlockingTypeProcessor : base::NonThreadSafe {
   ModelType GetModelType() const;
 
   // Starts the handshake with the sync thread.
-  void Enable(SyncCoreProxy* core_proxy);
+  void Enable(scoped_ptr<SyncCoreProxy> core_proxy);
 
   // Severs all ties to the sync thread and may delete local sync state.
   // Another call to Enable() can be used to re-establish this connection.
@@ -57,7 +57,9 @@ class SYNC_EXPORT_PRIVATE NonBlockingTypeProcessor : base::NonThreadSafe {
   void OnConnect(base::WeakPtr<NonBlockingTypeProcessorCore> core,
                  scoped_refptr<base::SequencedTaskRunner> sync_thread);
 
-  base::WeakPtr<NonBlockingTypeProcessor> AsWeakPtr();
+  // Returns the long-lived WeakPtr that is intended to be registered with the
+  // ProfileSyncService.
+  base::WeakPtr<NonBlockingTypeProcessor> AsWeakPtrForUI();
 
  private:
   ModelType type_;
@@ -71,10 +73,20 @@ class SYNC_EXPORT_PRIVATE NonBlockingTypeProcessor : base::NonThreadSafe {
   // SyncCoreProxy.
   bool is_connected_;
 
+  // Our link to data type management on the sync thread.
+  // Used for enabling and disabling sync for this type.
+  scoped_ptr<SyncCoreProxy> sync_core_proxy_;
+
   base::WeakPtr<NonBlockingTypeProcessorCore> core_;
   scoped_refptr<base::SequencedTaskRunner> sync_thread_;
 
-  base::WeakPtrFactory<NonBlockingTypeProcessor> weak_ptr_factory_;
+  // We use two different WeakPtrFactories because we want the pointers they
+  // issue to have different lifetimes.  When asked to disconnect from the sync
+  // thread, we want to make sure that no tasks generated as part of the
+  // now-obsolete connection to affect us.  But we also want the WeakPtr we
+  // sent to the UI thread to remain valid.
+  base::WeakPtrFactory<NonBlockingTypeProcessor> weak_ptr_factory_for_ui_;
+  base::WeakPtrFactory<NonBlockingTypeProcessor> weak_ptr_factory_for_sync_;
 };
 
 }  // namespace syncer
diff --git a/sync/internal_api/public/sync_core_proxy.h b/sync/internal_api/public/sync_core_proxy.h
index 5ec8efe..98f481d 100644
--- a/sync/internal_api/public/sync_core_proxy.h
+++ b/sync/internal_api/public/sync_core_proxy.h
@@ -27,6 +27,13 @@ class SYNC_EXPORT_PRIVATE SyncCoreProxy {
       syncer::ModelType type,
       base::WeakPtr<NonBlockingTypeProcessor> type_processor) = 0;
 
+  // Tells the syncer that we're no longer interested in syncing this type.
+  //
+  // Once this takes effect, the syncer can assume that it will no longer
+  // receive commit requests for this type.  It should also stop requesting
+  // and applying updates for this type, too.
+  virtual void Disconnect(syncer::ModelType type) = 0;
+
   // Creates a clone of this SyncCoreProxy.
   virtual scoped_ptr<SyncCoreProxy> Clone() const = 0;
 };
diff --git a/sync/internal_api/public/test/null_sync_core_proxy.h b/sync/internal_api/public/test/null_sync_core_proxy.h
index a491de9..d957aec5 100644
--- a/sync/internal_api/public/test/null_sync_core_proxy.h
+++ b/sync/internal_api/public/test/null_sync_core_proxy.h
@@ -23,6 +23,7 @@ class NullSyncCoreProxy : public SyncCoreProxy {
   virtual void ConnectTypeToCore(
       syncer::ModelType type,
       base::WeakPtr<NonBlockingTypeProcessor> processor) OVERRIDE;
+  virtual void Disconnect(syncer::ModelType type) OVERRIDE;
   virtual scoped_ptr<SyncCoreProxy> Clone() const OVERRIDE;
 };
 
diff --git a/sync/internal_api/sync_core.cc b/sync/internal_api/sync_core.cc
index 9c26095..dbeb76d 100644
--- a/sync/internal_api/sync_core.cc
+++ b/sync/internal_api/sync_core.cc
@@ -25,6 +25,10 @@ void SyncCore::ConnectSyncTypeToCore(
   model_type_registry_->InitializeNonBlockingType(type, task_runner, processor);
 }
 
+void SyncCore::Disconnect(ModelType type) {
+  model_type_registry_->RemoveNonBlockingType(type);
+}
+
 base::WeakPtr<SyncCore> SyncCore::AsWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }
diff --git a/sync/internal_api/sync_core.h b/sync/internal_api/sync_core.h
index 504304c..6f63070 100644
--- a/sync/internal_api/sync_core.h
+++ b/sync/internal_api/sync_core.h
@@ -36,6 +36,17 @@ class SYNC_EXPORT_PRIVATE SyncCore {
     scoped_refptr<base::SequencedTaskRunner> datatype_task_runner,
     base::WeakPtr<NonBlockingTypeProcessor> sync_client);
 
+  // Disconnects the syncer from the model and stops syncing the type.
+  //
+  // By the time this is called, the model thread should have already
+  // invalidated the WeakPtr it sent to us in the connect request.  Any
+  // messages sent to that NonBlockingTypeProcessor will not be recived.
+  //
+  // This is the sync thread's chance to clear state associated with the type.
+  // It also causes the syncer to stop requesting updates for this type, and to
+  // abort any in-progress commit requests.
+  void Disconnect(ModelType type);
+
   base::WeakPtr<SyncCore> AsWeakPtr();
 
  private:
diff --git a/sync/internal_api/sync_core_proxy_impl.cc b/sync/internal_api/sync_core_proxy_impl.cc
index a75041ac..7e4ecce 100644
--- a/sync/internal_api/sync_core_proxy_impl.cc
+++ b/sync/internal_api/sync_core_proxy_impl.cc
@@ -32,6 +32,11 @@ void SyncCoreProxyImpl::ConnectTypeToCore(
                  type_processor));
 }
 
+void SyncCoreProxyImpl::Disconnect(ModelType type) {
+  sync_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SyncCore::Disconnect, sync_core_, type));
+}
+
 scoped_ptr<SyncCoreProxy> SyncCoreProxyImpl::Clone() const {
   return scoped_ptr<SyncCoreProxy>(
       new SyncCoreProxyImpl(sync_task_runner_, sync_core_));
diff --git a/sync/internal_api/sync_core_proxy_impl.h b/sync/internal_api/sync_core_proxy_impl.h
index 99e1b8f..3622d2b 100644
--- a/sync/internal_api/sync_core_proxy_impl.h
+++ b/sync/internal_api/sync_core_proxy_impl.h
@@ -40,6 +40,9 @@ class SYNC_EXPORT_PRIVATE SyncCoreProxyImpl : public SyncCoreProxy {
       syncer::ModelType type,
       base::WeakPtr<NonBlockingTypeProcessor> type_processor) OVERRIDE;
 
+  // Disables syncing for the given type on the sync thread.
+  virtual void Disconnect(syncer::ModelType type) OVERRIDE;
+
   virtual scoped_ptr<SyncCoreProxy> Clone() const OVERRIDE;
 
  private:
diff --git a/sync/internal_api/sync_core_proxy_impl_unittest.cc b/sync/internal_api/sync_core_proxy_impl_unittest.cc
index 2e306ee..d1913b0 100644
--- a/sync/internal_api/sync_core_proxy_impl_unittest.cc
+++ b/sync/internal_api/sync_core_proxy_impl_unittest.cc
@@ -31,9 +31,7 @@ class SyncCoreProxyImplTest : public ::testing::Test {
     core_.reset();
   }
 
-  SyncCoreProxy* GetProxy() {
-    return &core_proxy_;
-  }
+  scoped_ptr<SyncCoreProxy> GetProxy() { return core_proxy_.Clone(); }
 
  private:
   base::MessageLoop loop_;
diff --git a/sync/internal_api/test/null_sync_core_proxy.cc b/sync/internal_api/test/null_sync_core_proxy.cc
index 10fc757..e0c93f6 100644
--- a/sync/internal_api/test/null_sync_core_proxy.cc
+++ b/sync/internal_api/test/null_sync_core_proxy.cc
@@ -16,6 +16,10 @@ void NullSyncCoreProxy::ConnectTypeToCore(
   NOTREACHED() << "NullSyncCoreProxy is not meant to be used";
 }
 
+void NullSyncCoreProxy::Disconnect(syncer::ModelType type) {
+  NOTREACHED() << "NullSyncCoreProxy is not meant to be used";
+}
+
 scoped_ptr<SyncCoreProxy> NullSyncCoreProxy::Clone() const {
   return scoped_ptr<SyncCoreProxy>(new NullSyncCoreProxy());
 }
diff --git a/sync/sessions/model_type_registry_unittest.cc b/sync/sessions/model_type_registry_unittest.cc
index 375e84b..a5e3c91 100644
--- a/sync/sessions/model_type_registry_unittest.cc
+++ b/sync/sessions/model_type_registry_unittest.cc
@@ -140,16 +140,12 @@ TEST_F(ModelTypeRegistryTest, NonBlockingTypes) {
   EXPECT_TRUE(registry()->GetEnabledTypes().Empty());
 
   registry()->InitializeNonBlockingType(
-      syncer::THEMES,
-      task_runner,
-      themes_processor.AsWeakPtr());
+      syncer::THEMES, task_runner, themes_processor.AsWeakPtrForUI());
   EXPECT_TRUE(registry()->GetEnabledTypes().Equals(
       ModelTypeSet(syncer::THEMES)));
 
   registry()->InitializeNonBlockingType(
-      syncer::SESSIONS,
-      task_runner,
-      sessions_processor.AsWeakPtr());
+      syncer::SESSIONS, task_runner, sessions_processor.AsWeakPtrForUI());
   EXPECT_TRUE(registry()->GetEnabledTypes().Equals(
       ModelTypeSet(syncer::THEMES, syncer::SESSIONS)));
 
@@ -177,9 +173,7 @@ TEST_F(ModelTypeRegistryTest, NonBlockingTypesWithDirectoryTypes) {
 
   // Add the themes non-blocking type.
   registry()->InitializeNonBlockingType(
-      syncer::THEMES,
-      task_runner,
-      themes_processor.AsWeakPtr());
+      syncer::THEMES, task_runner, themes_processor.AsWeakPtrForUI());
   current_types.Put(syncer::THEMES);
   EXPECT_TRUE(registry()->GetEnabledTypes().Equals(current_types));
 
@@ -190,9 +184,7 @@ TEST_F(ModelTypeRegistryTest, NonBlockingTypesWithDirectoryTypes) {
 
   // Add sessions non-blocking type.
   registry()->InitializeNonBlockingType(
-      syncer::SESSIONS,
-      task_runner,
-      sessions_processor.AsWeakPtr());
+      syncer::SESSIONS, task_runner, sessions_processor.AsWeakPtrForUI());
   current_types.Put(syncer::SESSIONS);
   EXPECT_TRUE(registry()->GetEnabledTypes().Equals(current_types));
 
@@ -219,13 +211,9 @@ TEST_F(ModelTypeRegistryTest, DeletionOrdering) {
   EXPECT_TRUE(registry()->GetEnabledTypes().Empty());
 
   registry()->InitializeNonBlockingType(
-      syncer::THEMES,
-      task_runner,
-      themes_processor->AsWeakPtr());
+      syncer::THEMES, task_runner, themes_processor->AsWeakPtrForUI());
   registry()->InitializeNonBlockingType(
-      syncer::SESSIONS,
-      task_runner,
-      sessions_processor->AsWeakPtr());
+      syncer::SESSIONS, task_runner, sessions_processor->AsWeakPtrForUI());
   EXPECT_TRUE(registry()->GetEnabledTypes().Equals(
       ModelTypeSet(syncer::THEMES, syncer::SESSIONS)));