Revert 202038 "Remove all but one use of WeakPtrFactory::DetachF..."

Caused local failures in linux_aura builds. Unsure why this isn't showing
up on the bots.

> Remove all but one use of WeakPtrFactory::DetachFromThread.
> 
> This CL changes WeakPtr in the following ways:
> * Changes thread-bindings semantics so that WeakPtrs only become bound when the first one is dereferenced, or the owning factory invalidates them.
> * Removes WeakPtrFactory::DetachFromThread.
> * Renames SupportsWeakPtr::DetachFromThread  to DetachFromThreadHack.
> 
> Calling code changes to allow this:
> * Unnecessary DetachFromThread() calls removed from PluginInfoMessageFilter, DhcpProxyScript[Adapter]FetcherWin and (Chromoting's) PolicyWatcherLinux.
> * DetachFromThread() calls rendered unnecessary by change in binding semantics removed from IOThread, SearchProviderInstallData, RuleRegistryWithCache and GLSurfaceGlx.
> 
> WebGraphicsContext3DInProcessCommandBufferImpl uses the re-named DetachFromThreadHack() - bug 234964 tracks work to remove that use.
> 
> Review URL: https://chromiumcodereview.appspot.com/14299011

BUG=232143, 234964, 243914
TBR=wez@chromium.org

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

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

3 files changed, 40 insertions, 89 deletions

diff --git a/base/memory/weak_ptr.cc b/base/memory/weak_ptr.cc
index 321b057..9dec8fd 100644
--- a/base/memory/weak_ptr.cc
+++ b/base/memory/weak_ptr.cc
@@ -8,23 +8,17 @@ namespace base {
 namespace internal {
 
 WeakReference::Flag::Flag() : is_valid_(true) {
-  // Flags only become bound when checked for validity, or invalidated,
-  // so that we can check that later validity/invalidation operations on
-  // the same Flag take place on the same thread.
-  thread_checker_.DetachFromThread();
 }
 
 void WeakReference::Flag::Invalidate() {
   // The flag being invalidated with a single ref implies that there are no
   // weak pointers in existence. Allow deletion on other thread in this case.
-  DCHECK(thread_checker_.CalledOnValidThread() || HasOneRef())
-      << "WeakPtrs must be checked and invalidated on the same thread.";
+  DCHECK(thread_checker_.CalledOnValidThread() || HasOneRef());
   is_valid_ = false;
 }
 
 bool WeakReference::Flag::IsValid() const {
-  DCHECK(thread_checker_.CalledOnValidThread())
-      << "WeakPtrs must be checked and invalidated on the same thread.";
+  DCHECK(thread_checker_.CalledOnValidThread());
   return is_valid_;
 }
 
@@ -52,10 +46,10 @@ WeakReferenceOwner::~WeakReferenceOwner() {
 }
 
 WeakReference WeakReferenceOwner::GetRef() const {
-  // If we hold the last reference to the Flag then create a new one.
+  // We also want to reattach to the current thread if all previous references
+  // have gone away.
   if (!HasRefs())
     flag_ = new WeakReference::Flag();
-
   return WeakReference(flag_);
 }
 
diff --git a/base/memory/weak_ptr.h b/base/memory/weak_ptr.h
index 8c2220f..19def01 100644
--- a/base/memory/weak_ptr.h
+++ b/base/memory/weak_ptr.h
@@ -19,9 +19,6 @@
 //    void SpawnWorker() { Worker::StartNew(weak_factory_.GetWeakPtr()); }
 //    void WorkComplete(const Result& result) { ... }
 //   private:
-//    // Member variables should appear before the WeakPtrFactory, to ensure
-//    // that any WeakPtrs to Controller are invalidated before its members
-//    // variable's destructors are executed, rendering them invalid.
 //    WeakPtrFactory<Controller> weak_factory_;
 //  };
 //
@@ -47,18 +44,17 @@
 
 // ------------------------- IMPORTANT: Thread-safety -------------------------
 
-// Weak pointers may be passed safely between threads, but must always be
-// dereferenced and invalidated on the same thread otherwise checking the
-// pointer would be racey.
-//
-// To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory
-// is dereferenced, the factory and its WeakPtrs become bound to the calling
-// thread, and cannot be dereferenced or invalidated on any other thread. Bound
-// WeakPtrs can still be handed off to other threads, e.g. to use to post tasks
-// back to object on the bound thread.
-//
-// Invalidating the factory's WeakPtrs un-binds it from the thread, allowing it
-// to be passed for a different thread to use or delete it.
+// Weak pointers must always be dereferenced and invalidated on the same thread
+// otherwise checking the pointer would be racey.  WeakPtrFactory enforces this
+// by binding itself to the current thread when a WeakPtr is first created
+// and un-binding only when those pointers are invalidated.  WeakPtrs may still
+// be handed off to other threads, however, so long as they are only actually
+// dereferenced on the originating thread. This includes posting tasks to the
+// thread using base::Bind() to invoke a method on the object via the WeakPtr.
+
+// Calling SupportsWeakPtr::DetachFromThread() can work around the limitations
+// above and cancel the thread binding of the object and all WeakPtrs pointing
+// to it, but it's not recommended and unsafe.  See crbug.com/232143.
 
 #ifndef BASE_MEMORY_WEAK_PTR_H_
 #define BASE_MEMORY_WEAK_PTR_H_
@@ -81,7 +77,7 @@ namespace internal {
 
 class BASE_EXPORT WeakReference {
  public:
-  // Although Flag is bound to a specific thread, it may be deleted from another
+  // While Flag is bound to a specific thread, it may be deleted from another
   // via base::WeakPtr::~WeakPtr().
   class Flag : public RefCountedThreadSafe<Flag> {
    public:
@@ -90,8 +86,7 @@ class BASE_EXPORT WeakReference {
     void Invalidate();
     bool IsValid() const;
 
-    // Remove this when crbug.com/234964 is addressed.
-    void DetachFromThreadHack() { thread_checker_.DetachFromThread(); }
+    void DetachFromThread() { thread_checker_.DetachFromThread(); }
 
    private:
     friend class base::RefCountedThreadSafe<Flag>;
@@ -125,9 +120,10 @@ class BASE_EXPORT WeakReferenceOwner {
 
   void Invalidate();
 
-  // Remove this when crbug.com/234964 is addressed.
-  void DetachFromThreadHack() {
-    if (flag_) flag_->DetachFromThreadHack();
+  // Indicates that this object will be used on another thread from now on.
+  // Do not use this in new code. See crbug.com/232143.
+  void DetachFromThread() {
+    if (flag_) flag_->DetachFromThread();
   }
 
  private:
@@ -273,6 +269,13 @@ class WeakPtrFactory {
     return weak_reference_owner_.HasRefs();
   }
 
+  // Indicates that this object will be used on another thread from now on.
+  // Do not use this in new code. See crbug.com/232143.
+  void DetachFromThread() {
+    DCHECK(ptr_);
+    weak_reference_owner_.DetachFromThread();
+  }
+
  private:
   internal::WeakReferenceOwner weak_reference_owner_;
   T* ptr_;
@@ -293,12 +296,10 @@ class SupportsWeakPtr : public internal::SupportsWeakPtrBase {
     return WeakPtr<T>(weak_reference_owner_.GetRef(), static_cast<T*>(this));
   }
 
-  // Removes the binding, if any, from this object to a particular thread.
-  // This is used in WebGraphicsContext3DInProcessCommandBufferImpl to work-
-  // around access to cmmand buffer objects by more than one thread.
-  // Remove this when crbug.com/234964 is addressed.
-  void DetachFromThreadHack() {
-    weak_reference_owner_.DetachFromThreadHack();
+  // Indicates that this object will be used on another thread from now on.
+  // Do not use this in new code. See crbug.com/232143.
+  void DetachFromThread() {
+    weak_reference_owner_.DetachFromThread();
   }
 
  protected:
diff --git a/base/memory/weak_ptr_unittest.cc b/base/memory/weak_ptr_unittest.cc
index ff1103c..dbe7960 100644
--- a/base/memory/weak_ptr_unittest.cc
+++ b/base/memory/weak_ptr_unittest.cc
@@ -339,7 +339,7 @@ TEST(WeakPtrTest, MoveOwnershipExplicitlyObjectNotReferenced) {
   // Case 1: The target is not bound to any thread yet. So calling
   // DetachFromThread() is a no-op.
   Target target;
-  target.DetachFromThreadHack();
+  target.DetachFromThread();
 
   // Case 2: The target is bound to main thread but no WeakPtr is pointing to
   // it. In this case, it will be re-bound to any thread trying to get a
@@ -347,7 +347,7 @@ TEST(WeakPtrTest, MoveOwnershipExplicitlyObjectNotReferenced) {
   {
     WeakPtr<Target> weak_ptr = target.AsWeakPtr();
   }
-  target.DetachFromThreadHack();
+  target.DetachFromThread();
 }
 
 TEST(WeakPtrTest, MoveOwnershipExplicitly) {
@@ -362,7 +362,7 @@ TEST(WeakPtrTest, MoveOwnershipExplicitly) {
     EXPECT_EQ(&target, background.DeRef(arrow));
 
     // Detach from background thread.
-    target.DetachFromThreadHack();
+    target.DetachFromThread();
 
     // Re-bind to main thread.
     EXPECT_EQ(&target, arrow->target.get());
@@ -500,7 +500,7 @@ TEST(WeakPtrDeathTest, WeakPtrCopyDoesNotChangeThreadBinding) {
   background.DeleteArrow(arrow_copy);
 }
 
-TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtrAfterReference) {
+TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtr) {
   // The default style "fast" does not support multi-threaded tests
   // (introduces deadlock on Linux).
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
@@ -512,7 +512,6 @@ TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtrAfterReference) {
   // thread ownership can not be implicitly moved).
   Arrow arrow;
   arrow.target = target.AsWeakPtr();
-  arrow.target.get();
 
   // Background thread tries to deref target, which violates thread ownership.
   BackgroundThread background;
@@ -520,66 +519,23 @@ TEST(WeakPtrDeathTest, NonOwnerThreadDereferencesWeakPtrAfterReference) {
   ASSERT_DEATH(background.DeRef(&arrow), "");
 }
 
-TEST(WeakPtrDeathTest, NonOwnerThreadDeletesWeakPtrAfterReference) {
+TEST(WeakPtrDeathTest, NonOwnerThreadDeletesObject) {
   // The default style "fast" does not support multi-threaded tests
   // (introduces deadlock on Linux).
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
 
   scoped_ptr<Target> target(new Target());
-
-  // Main thread creates an arrow referencing the Target.
-  Arrow arrow;
-  arrow.target = target->AsWeakPtr();
-
-  // Background thread tries to deref target, binding it to the thread.
-  BackgroundThread background;
-  background.Start();
-  background.DeRef(&arrow);
-
-  // Main thread deletes Target, violating thread binding.
-  Target* foo = target.release();
-  ASSERT_DEATH(delete foo, "");
-}
-
-TEST(WeakPtrDeathTest, NonOwnerThreadDeletesObjectAfterReference) {
-  // The default style "fast" does not support multi-threaded tests
-  // (introduces deadlock on Linux).
-  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-
-  scoped_ptr<Target> target(new Target());
-
-  // Main thread creates an arrow referencing the Target, and references it, so
-  // that it becomes bound to the thread.
+  // Main thread creates an arrow referencing the Target (so target's thread
+  // ownership can not be implicitly moved).
   Arrow arrow;
   arrow.target = target->AsWeakPtr();
-  arrow.target.get();
 
-  // Background thread tries to delete target, volating thread binding.
+  // Background thread tries to delete target, which violates thread ownership.
   BackgroundThread background;
   background.Start();
   ASSERT_DEATH(background.DeleteTarget(target.release()), "");
 }
 
-TEST(WeakPtrDeathTest, NonOwnerThreadReferencesObjectAfterDeletion) {
-  // The default style "fast" does not support multi-threaded tests
-  // (introduces deadlock on Linux).
-  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-
-  scoped_ptr<Target> target(new Target());
-
-  // Main thread creates an arrow referencing the Target.
-  Arrow arrow;
-  arrow.target = target->AsWeakPtr();
-
-  // Background thread tries to delete target, binding the object to the thread.
-  BackgroundThread background;
-  background.Start();
-  background.DeleteTarget(target.release());
-
-  // Main thread attempts to dereference the target, violating thread binding.
-  ASSERT_DEATH(arrow.target.get(), "");
-}
-
 #endif
 
 }  // namespace base