Make ~GoogleURLChangeNotifier happen on the I/O thread.

Also change the test code to allow for its destruction.

One key problem was that the object containing WeakPtr is created on the UI
thread but then always used on the I/O thread like everything else that hangs
off of ResourceMessageFilter. The solution was to allow WeakPtr to detach
from its thread (and automatically re-attach the next time the thread is
checked).

BUG=38475
TEST=base_unittest --gtest_filter=NonThread*:ThreadChecker* 
     unit_tests --gtest_filter=SearchProviderInstallData*

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

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

7 files changed, 109 insertions, 5 deletions

diff --git a/base/non_thread_safe.cc b/base/non_thread_safe.cc
index 6889101..b01ed55 100644
--- a/base/non_thread_safe.cc
+++ b/base/non_thread_safe.cc
@@ -9,12 +9,16 @@
 
 #include "base/logging.h"
 
+NonThreadSafe::~NonThreadSafe() {
+  DCHECK(CalledOnValidThread());
+}
+
 bool NonThreadSafe::CalledOnValidThread() const {
   return thread_checker_.CalledOnValidThread();
 }
 
-NonThreadSafe::~NonThreadSafe() {
-  DCHECK(CalledOnValidThread());
+void NonThreadSafe::DetachFromThread() {
+  thread_checker_.DetachFromThread();
 }
 
 #endif  // NDEBUG
diff --git a/base/non_thread_safe.h b/base/non_thread_safe.h
index e435884..6f993ee 100644
--- a/base/non_thread_safe.h
+++ b/base/non_thread_safe.h
@@ -34,6 +34,14 @@ class NonThreadSafe {
 
   bool CalledOnValidThread() const;
 
+ protected:
+  // Changes the thread that is checked for in CalledOnValidThread. The next
+  // call to CalledOnValidThread will attach this class to a new thread. It is
+  // up to the NonThreadSafe derived class to decide to expose this or not.
+  // This may be useful when an object may be created on one thread and then
+  // used exclusively on another thread.
+  void DetachFromThread();
+
  private:
   ThreadChecker thread_checker_;
 };
@@ -44,6 +52,9 @@ class NonThreadSafe {
   bool CalledOnValidThread() const {
     return true;
   }
+
+ protected:
+  void DetachFromThread() {}
 };
 #endif  // NDEBUG
 
diff --git a/base/non_thread_safe_unittest.cc b/base/non_thread_safe_unittest.cc
index 0603987..1db198b 100644
--- a/base/non_thread_safe_unittest.cc
+++ b/base/non_thread_safe_unittest.cc
@@ -23,6 +23,10 @@ class NonThreadSafeClass : public NonThreadSafe {
     DCHECK(CalledOnValidThread());
   }
 
+  void DetachFromThread() {
+    NonThreadSafe::DetachFromThread();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(NonThreadSafeClass);
 };
@@ -74,6 +78,20 @@ TEST(NonThreadSafeTest, CallsAllowedOnSameThread) {
   non_thread_safe_class.reset();
 }
 
+TEST(NonThreadSafeTest, DetachThenDestructOnDifferentThread) {
+  scoped_ptr<NonThreadSafeClass> non_thread_safe_class(
+      new NonThreadSafeClass);
+
+  // Verify that the destructor doesn't assert when called on a different thread
+  // after a detach.
+  non_thread_safe_class->DetachFromThread();
+  DeleteNonThreadSafeClassOnThread delete_on_thread(
+      non_thread_safe_class.release());
+
+  delete_on_thread.Start();
+  delete_on_thread.Join();
+}
+
 #if GTEST_HAS_DEATH_TEST
 
 TEST(NonThreadSafeDeathTest, MethodNotAllowedOnDifferentThread) {
diff --git a/base/thread_checker.cc b/base/thread_checker.cc
index 124b76c..7a41a448 100644
--- a/base/thread_checker.cc
+++ b/base/thread_checker.cc
@@ -6,11 +6,24 @@
 
 // This code is only done in debug builds.
 #ifndef NDEBUG
-ThreadChecker::ThreadChecker() : valid_thread_id_(PlatformThread::CurrentId()) {
+
+ThreadChecker::ThreadChecker() {
+  EnsureThreadIdAssigned();
 }
 
 bool ThreadChecker::CalledOnValidThread() const {
-  return valid_thread_id_ == PlatformThread::CurrentId();
+  EnsureThreadIdAssigned();
+  return *valid_thread_id_ == PlatformThread::CurrentId();
+}
+
+void ThreadChecker::DetachFromThread() {
+  valid_thread_id_.reset();
+}
+
+void ThreadChecker::EnsureThreadIdAssigned() const {
+  if (valid_thread_id_.get())
+    return;
+  valid_thread_id_.reset(new PlatformThreadId(PlatformThread::CurrentId()));
 }
 
 #endif  // NDEBUG
diff --git a/base/thread_checker.h b/base/thread_checker.h
index 4e1d792..60b8474 100644
--- a/base/thread_checker.h
+++ b/base/thread_checker.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include "base/platform_thread.h"
+#include "base/scoped_ptr.h"
 
 // Before using this class, please consider using NonThreadSafe as it
 // makes it much easier to determine the nature of your class.
@@ -37,8 +38,16 @@ class ThreadChecker {
 
   bool CalledOnValidThread() const;
 
+  // Changes the thread that is checked for in CalledOnValidThread.  This may
+  // be useful when an object may be created on one thread and then used
+  // exclusively on another thread.
+  void DetachFromThread();
+
  private:
-  const PlatformThreadId valid_thread_id_;
+  void EnsureThreadIdAssigned() const;
+
+  // This is mutable so that CalledOnValidThread can set it.
+  mutable scoped_ptr<PlatformThreadId> valid_thread_id_;
 };
 #else
 // Do nothing in release mode.
@@ -47,6 +56,8 @@ class ThreadChecker {
   bool CalledOnValidThread() const {
     return true;
   }
+
+  void DetachFromThread() {}
 };
 #endif  // NDEBUG
 
diff --git a/base/thread_checker_unittest.cc b/base/thread_checker_unittest.cc
index fe49e21..6c55348 100644
--- a/base/thread_checker_unittest.cc
+++ b/base/thread_checker_unittest.cc
@@ -23,6 +23,10 @@ class ThreadCheckerClass : public ThreadChecker {
     DCHECK(CalledOnValidThread());
   }
 
+  void DetachFromThread() {
+    ThreadChecker::DetachFromThread();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ThreadCheckerClass);
 };
@@ -87,6 +91,19 @@ TEST(ThreadCheckerTest, DestructorAllowedOnDifferentThread) {
   delete_on_thread.Join();
 }
 
+TEST(ThreadCheckerTest, DetachFromThread) {
+  scoped_ptr<ThreadCheckerClass> thread_checker_class(
+      new ThreadCheckerClass);
+
+  // Verify that DoStuff doesn't assert when called on a different thread after
+  // a call to DetachFromThread.
+  thread_checker_class->DetachFromThread();
+  CallDoStuffOnThread call_on_thread(thread_checker_class.get());
+
+  call_on_thread.Start();
+  call_on_thread.Join();
+}
+
 #if GTEST_HAS_DEATH_TEST
 
 TEST(ThreadCheckerDeathTest, MethodNotAllowedOnDifferentThread) {
@@ -102,6 +119,24 @@ TEST(ThreadCheckerDeathTest, MethodNotAllowedOnDifferentThread) {
     }, "");
 }
 
+TEST(ThreadCheckerDeathTest, DetachFromThread) {
+  ASSERT_DEBUG_DEATH({
+      scoped_ptr<ThreadCheckerClass> thread_checker_class(
+          new ThreadCheckerClass);
+
+      // Verify that DoStuff doesn't assert when called on a different thread
+      // after a call to DetachFromThread.
+      thread_checker_class->DetachFromThread();
+      CallDoStuffOnThread call_on_thread(thread_checker_class.get());
+
+      call_on_thread.Start();
+      call_on_thread.Join();
+
+      // Verify that DoStuff asserts after moving to another thread.
+      thread_checker_class->DoStuff();
+    }, "");
+}
+
 #endif  // GTEST_HAS_DEATH_TEST
 
 #endif  // NDEBUG
diff --git a/base/weak_ptr.h b/base/weak_ptr.h
index 4ae7972..ed9ef66 100644
--- a/base/weak_ptr.h
+++ b/base/weak_ptr.h
@@ -74,6 +74,8 @@ class WeakReference {
     void Invalidate() { handle_ = NULL; }
     bool is_valid() const { return handle_ != NULL; }
 
+    void DetachFromThread() { NonThreadSafe::DetachFromThread(); }
+
    private:
     Flag** handle_;
   };
@@ -101,6 +103,11 @@ class WeakReferenceOwner {
 
   void Invalidate();
 
+  // Indicates that this object will be used on another thread from now on.
+  void DetachFromThread() {
+    if (flag_) flag_->DetachFromThread();
+  }
+
  private:
   mutable WeakReference::Flag* flag_;
 };
@@ -192,6 +199,11 @@ class SupportsWeakPtr {
     return WeakPtr<T>(weak_reference_owner_.GetRef(), static_cast<T*>(this));
   }
 
+  // Indicates that this object will be used on another thread from now on.
+  void DetachFromThread() {
+    weak_reference_owner_.DetachFromThread();
+  }
+
  private:
   internal::WeakReferenceOwner weak_reference_owner_;
   DISALLOW_COPY_AND_ASSIGN(SupportsWeakPtr);