No longer keep a lingering reference to TypedUrlDataTypeController after PSS exits

BUG=127706
TEST=Start chrome, sign in to sync, start print job spooling and exit while the print job is spooling, should not get a crash.


Review URL: https://chromiumcodereview.appspot.com/10391080

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

4 files changed, 68 insertions, 52 deletions

diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc
index ec4efea..025894a 100644
--- a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc
+++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc
@@ -33,7 +33,6 @@ NonFrontendDataTypeController::NonFrontendDataTypeController(
       state_(NOT_RUNNING),
       abort_association_(false),
       abort_association_complete_(false, false),
-      datatype_stopped_(false, false),
       start_association_called_(true, false),
       start_models_failed_(false) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -111,6 +110,14 @@ void NonFrontendDataTypeController::StopWhileAssociating() {
   StartDoneImpl(ABORTED, STOPPING, SyncError());
 }
 
+namespace {
+// Helper function that signals the UI thread once the StopAssociation task
+// has finished completing (this task is queued after the StopAssociation task).
+void SignalCallback(base::WaitableEvent* wait_event) {
+  wait_event->Signal();
+}
+}  // namespace
+
 void NonFrontendDataTypeController::Stop() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_NE(state_, NOT_RUNNING);
@@ -208,9 +215,7 @@ void NonFrontendDataTypeController::Stop() {
   // for any more changes or process them from server.
   profile_sync_service_->DeactivateDataType(type());
 
-  if (StopAssociationAsync()) {
-    datatype_stopped_.Wait();
-  } else {
+  if (!StopAssociationAsync()) {
     // We do DFATAL here because this will eventually lead to a failed CHECK
     // when the change processor gets destroyed on the wrong thread.
     LOG(DFATAL) << "Failed to destroy datatype " << name();
@@ -258,7 +263,6 @@ NonFrontendDataTypeController::NonFrontendDataTypeController()
       state_(NOT_RUNNING),
       abort_association_(false),
       abort_association_complete_(false, false),
-      datatype_stopped_(false, false),
       start_association_called_(true, false) {
 }
 
@@ -466,13 +470,31 @@ void NonFrontendDataTypeController::StartAssociation() {
 bool NonFrontendDataTypeController::StopAssociationAsync() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(state(), STOPPING);
-  return PostTaskOnBackendThread(
-      FROM_HERE,
-      base::Bind(
-          &NonFrontendDataTypeController::StopAssociation, this));
+  if (PostTaskOnBackendThread(
+          FROM_HERE,
+          base::Bind(
+              &NonFrontendDataTypeController::StopAssociation, this))) {
+    // The remote thread will hold on to a reference to this object until
+    // the StopAssociation task finishes running. We want to make sure that we
+    // do not return from this routine until there are no more references to
+    // this object on the remote thread, so we queue up the SignalCallback
+    // task below - this task does not maintain a reference to the DTC, so
+    // when it signals this thread, we know that the previous task has executed
+    // and there are no more lingering remote references to the DTC.
+    // This fixes the race described in http://crbug.com/127706.
+    base::WaitableEvent datatype_stopped(false, false);
+    if (PostTaskOnBackendThread(
+            FROM_HERE,
+            base::Bind(&SignalCallback, &datatype_stopped))) {
+      datatype_stopped.Wait();
+      return true;
+    }
+  }
+  return false;
 }
 
 void NonFrontendDataTypeController::StopAssociation() {
+  DCHECK(!HasOneRef());
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (model_associator_.get()) {
     SyncError error;  // Not used.
@@ -480,7 +502,6 @@ void NonFrontendDataTypeController::StopAssociation() {
   }
   model_associator_.reset();
   change_processor_.reset();
-  datatype_stopped_.Signal();
 }
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.h b/chrome/browser/sync/glue/non_frontend_data_type_controller.h
index 9d9d609..f924563 100644
--- a/chrome/browser/sync/glue/non_frontend_data_type_controller.h
+++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.h
@@ -78,6 +78,9 @@ class NonFrontendDataTypeController : public DataTypeController {
   // Posts the given task to the backend thread, i.e. the thread the
   // datatype lives on.  Return value: True if task posted successfully,
   // false otherwise.
+  // NOTE: The StopAssociationAsync() implementation relies on the fact that
+  // implementations of this API do not hold any references to the DTC while
+  // the task is executing. See http://crbug.com/127706.
   virtual bool PostTaskOnBackendThread(
       const tracked_objects::Location& from_here,
       const base::Closure& task) = 0;
@@ -176,10 +179,6 @@ class NonFrontendDataTypeController : public DataTypeController {
   bool abort_association_;
   base::WaitableEvent abort_association_complete_;
 
-  // Barrier to ensure that the datatype has been stopped on the DB thread
-  // from the UI thread.
-  base::WaitableEvent datatype_stopped_;
-
   // This is added for debugging purpose.
   // TODO(lipalani): Remove this after debugging.
   base::WaitableEvent start_association_called_;
diff --git a/chrome/browser/sync/glue/typed_url_data_type_controller.cc b/chrome/browser/sync/glue/typed_url_data_type_controller.cc
index 0642d06..287ae14 100644
--- a/chrome/browser/sync/glue/typed_url_data_type_controller.cc
+++ b/chrome/browser/sync/glue/typed_url_data_type_controller.cc
@@ -23,25 +23,39 @@ using content::BrowserThread;
 
 namespace {
 
-typedef base::Callback<void(history::HistoryBackend*)> HistoryBackendTask;
-
-class RunHistoryBackendTask : public HistoryDBTask {
+// The history service exposes a special non-standard task API which calls back
+// once a task has been dispatched, so we have to build a special wrapper around
+// the tasks we want to run.
+class RunTaskOnHistoryThread : public HistoryDBTask {
  public:
-  explicit RunHistoryBackendTask(const HistoryBackendTask& task)
-      : task_(task) {}
+  explicit RunTaskOnHistoryThread(const base::Closure& task,
+                                  TypedUrlDataTypeController* dtc)
+      : task_(new base::Closure(task)),
+        dtc_(dtc) {
+  }
 
   virtual bool RunOnDBThread(history::HistoryBackend* backend,
-                             history::HistoryDatabase* db) {
-    task_.Run(backend);
+                             history::HistoryDatabase* db) OVERRIDE {
+    // Set the backend, then release our reference before executing the task.
+    dtc_->SetBackend(backend);
+    dtc_ = NULL;
+
+    // Invoke the task, then free it immediately so we don't keep a reference
+    // around all the way until DoneRunOnMainThread() is invoked back on the
+    // main thread - we want to release references as soon as possible to avoid
+    // keeping them around too long during shutdown.
+    task_->Run();
+    task_.reset();
     return true;
   }
 
-  virtual void DoneRunOnMainThread() {}
+  virtual void DoneRunOnMainThread() OVERRIDE {}
 
  protected:
-  virtual ~RunHistoryBackendTask() {}
+  virtual ~RunTaskOnHistoryThread() {}
 
-  HistoryBackendTask task_;
+  scoped_ptr<base::Closure> task_;
+  scoped_refptr<TypedUrlDataTypeController> dtc_;
 };
 
 }  // namespace
@@ -67,6 +81,11 @@ browser_sync::ModelSafeGroup TypedUrlDataTypeController::model_safe_group()
   return browser_sync::GROUP_HISTORY;
 }
 
+void TypedUrlDataTypeController::SetBackend(history::HistoryBackend* backend) {
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  backend_ = backend;
+}
+
 void TypedUrlDataTypeController::Observe(
     int type,
     const content::NotificationSource& source,
@@ -100,12 +119,8 @@ bool TypedUrlDataTypeController::PostTaskOnBackendThread(
   HistoryService* history = profile()->GetHistoryService(
       Profile::IMPLICIT_ACCESS);
   if (history) {
-    history_service_ = history;
-    history_service_->ScheduleDBTask(
-        new RunHistoryBackendTask(
-            base::Bind(&TypedUrlDataTypeController::RunTaskOnBackendThread,
-                       this, task)),
-        &cancelable_consumer_);
+    history->ScheduleDBTask(new RunTaskOnHistoryThread(task, this),
+                            &cancelable_consumer_);
     return true;
   } else {
     // History must be disabled - don't start.
@@ -136,12 +151,4 @@ void TypedUrlDataTypeController::StopModels() {
 
 TypedUrlDataTypeController::~TypedUrlDataTypeController() {}
 
-void TypedUrlDataTypeController::RunTaskOnBackendThread(
-    const base::Closure& task,
-    history::HistoryBackend* backend) {
-  // Store |backend| so that |task| can use it.
-  backend_ = backend;
-  task.Run();
-}
-
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/typed_url_data_type_controller.h b/chrome/browser/sync/glue/typed_url_data_type_controller.h
index 11145d2..411dca5 100644
--- a/chrome/browser/sync/glue/typed_url_data_type_controller.h
+++ b/chrome/browser/sync/glue/typed_url_data_type_controller.h
@@ -45,15 +45,9 @@ class TypedUrlDataTypeController : public NonFrontendDataTypeController,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // CancelableRequestConsumerBase implementation.
-  virtual void OnRequestAdded(CancelableRequestProvider* provider,
-                              CancelableRequestProvider::Handle handle) {}
-  virtual void OnRequestRemoved(CancelableRequestProvider* provider,
-                                CancelableRequestProvider::Handle handle) {}
-  virtual void WillExecute(CancelableRequestProvider* provider,
-                           CancelableRequestProvider::Handle handle) {}
-  virtual void DidExecute(CancelableRequestProvider* provider,
-                          CancelableRequestProvider::Handle handle) {}
+  // Invoked on the history thread to set our history backend - must be called
+  // before CreateSyncComponents() is invoked.
+  void SetBackend(history::HistoryBackend* backend);
 
  protected:
   // NonFrontendDataTypeController interface.
@@ -66,12 +60,7 @@ class TypedUrlDataTypeController : public NonFrontendDataTypeController,
  private:
   virtual ~TypedUrlDataTypeController();
 
-  // Used by PostTaskOnBackendThread().
-  void RunTaskOnBackendThread(const base::Closure& task,
-                              history::HistoryBackend* backend);
-
   history::HistoryBackend* backend_;
-  scoped_refptr<HistoryService> history_service_;
   content::NotificationRegistrar notification_registrar_;
   PrefChangeRegistrar pref_registrar_;