Make WeakPtr thread-safe, i.e. allow cross-thread copying of WeakPtr

and destruction on a thread other than the one where the original
reference was created.

The problem with the current implementation is modifying the flag pointer
from WeakPtr::~WeakPtr which might happen on a different thread (potentially
racing with somebody invalidating the flag on the original thread).

For compatibility reasons, creating the initial reference attaches to the
thread and governs the thread-safety checking logic with respect to checking
a reference to be valid and invalidating it, which should both only be done
on the same thread.

BUG=82509
TEST=added unit tests

Added memleak suppression: http://crbug.com/94345

TBR=timurrrr@chromium.org

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

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

base/memory/weak_ptr.cc

@@ -7,28 +7,28 @@
 namespace base {
 namespace internal {
 
-WeakReference::Flag::Flag(Flag** handle) : handle_(handle) {
+WeakReference::Flag::Flag() : is_valid_(true) {
 }
 
 void WeakReference::Flag::Invalidate() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  handle_ = NULL;
+  // 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());
+  is_valid_ = false;
 }
 
 bool WeakReference::Flag::IsValid() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return handle_ != NULL;
+  return is_valid_;
 }
 
 WeakReference::Flag::~Flag() {
-  if (handle_)
-    *handle_ = NULL;
 }
 
 WeakReference::WeakReference() {
 }
 
-WeakReference::WeakReference(Flag* flag) : flag_(flag) {
+WeakReference::WeakReference(const Flag* flag) : flag_(flag) {
 }
 
 WeakReference::~WeakReference() {
@@ -38,7 +38,7 @@ bool WeakReference::is_valid() const {
   return flag_ && flag_->IsValid();
 }
 
-WeakReferenceOwner::WeakReferenceOwner() : flag_(NULL) {
+WeakReferenceOwner::WeakReferenceOwner() {
 }
 
 WeakReferenceOwner::~WeakReferenceOwner() {
@@ -46,8 +46,10 @@ WeakReferenceOwner::~WeakReferenceOwner() {
 }
 
 WeakReference WeakReferenceOwner::GetRef() const {
-  if (!flag_)
-    flag_ = new WeakReference::Flag(&flag_);
+  // 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_);
 }
 

base/memory/weak_ptr.h

@@ -7,6 +7,15 @@
 // bound to the lifetime of the referrers.  In other words, this is useful when
 // reference counting is not a good fit.
 //
+// Thread-safety notes:
+// When you get a WeakPtr (from a WeakPtrFactory or SupportsWeakPtr),
+// the WeakPtr becomes bound to the current thread. You may only
+// dereference the WeakPtr on that thread. However, it is safe to
+// destroy the WeakPtr object on another thread.
+// Since a WeakPtr object may be destroyed on a background thread,
+// querying WeakPtrFactory's HasWeakPtrs() method can be racy.
+//
+//
 // A common alternative to weak pointers is to have the shared object hold a
 // list of all referrers, and then when the shared object is destroyed, it
 // calls a method on the referrers to tell them to drop their references.  This
@@ -45,8 +54,6 @@
 // dereferencing the Controller back pointer after the Controller has been
 // destroyed.
 //
-// WARNING: weak pointers are not threadsafe!!!  You must only use a WeakPtr
-// instance on thread where it was created.
 
 #ifndef BASE_MEMORY_WEAK_PTR_H_
 #define BASE_MEMORY_WEAK_PTR_H_
@@ -69,7 +76,7 @@ class BASE_EXPORT WeakReference {
   // via base::WeakPtr::~WeakPtr().
   class Flag : public RefCountedThreadSafe<Flag> {
    public:
-    explicit Flag(Flag** handle);
+    Flag();
 
     void Invalidate();
     bool IsValid() const;
@@ -82,17 +89,17 @@ class BASE_EXPORT WeakReference {
     ~Flag();
 
     ThreadChecker thread_checker_;
-    Flag** handle_;
+    bool is_valid_;
   };
 
   WeakReference();
-  WeakReference(Flag* flag);
+  explicit WeakReference(const Flag* flag);
   ~WeakReference();
 
   bool is_valid() const;
 
  private:
-  scoped_refptr<Flag> flag_;
+  scoped_refptr<const Flag> flag_;
 };
 
 class BASE_EXPORT WeakReferenceOwner {
@@ -103,7 +110,7 @@ class BASE_EXPORT WeakReferenceOwner {
   WeakReference GetRef() const;
 
   bool HasRefs() const {
-    return flag_ != NULL;
+    return flag_.get() && !flag_->HasOneRef();
   }
 
   void Invalidate();
@@ -114,7 +121,7 @@ class BASE_EXPORT WeakReferenceOwner {
   }
 
  private:
-  mutable WeakReference::Flag* flag_;
+  mutable scoped_refptr<WeakReference::Flag> flag_;
 };
 
 // This class simplifies the implementation of WeakPtr's type conversion

base/memory/weak_ptr_unittest.cc

@@ -6,6 +6,7 @@
 #include "base/memory/weak_ptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "base/message_loop.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 
 namespace base {
@@ -38,6 +39,104 @@ struct Derived : Base {};
 struct Producer : SupportsWeakPtr<Producer> {};
 struct Consumer { WeakPtr<Producer> producer; };
 
+// Helper class to create and destroy weak pointer copies
+// and delete objects on a background thread.
+class BackgroundThread : public Thread {
+ public:
+  BackgroundThread()
+      : Thread("owner_thread") {
+  }
+
+  void CreateConsumerFromProducer(Consumer** consumer, Producer* producer) {
+    WaitableEvent completion(true, false);
+    message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableFunction(&BackgroundThread::DoCreateFromProducer,
+                            consumer,
+                            producer,
+                            &completion));
+    completion.Wait();
+  }
+
+  void CreateConsumerFromConsumer(Consumer** consumer, const Consumer* other) {
+    WaitableEvent completion(true, false);
+    message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableFunction(&BackgroundThread::DoCreateFromConsumer,
+                            consumer,
+                            other,
+                            &completion));
+    completion.Wait();
+  }
+
+  void DeleteProducer(Producer* object) {
+    WaitableEvent completion(true, false);
+    message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableFunction(&BackgroundThread::DoDeleteProducer,
+                            object,
+                            &completion));
+    completion.Wait();
+  }
+
+  void DeleteConsumer(Consumer* object) {
+    WaitableEvent completion(true, false);
+    message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableFunction(&BackgroundThread::DoDeleteConsumer,
+                            object,
+                            &completion));
+    completion.Wait();
+  }
+
+  Producer* DeRef(const Consumer* consumer) {
+    WaitableEvent completion(true, false);
+    Producer* result = NULL;
+    message_loop()->PostTask(
+        FROM_HERE,
+        NewRunnableFunction(&BackgroundThread::DoDeRef,
+                            consumer,
+                            &result,
+                            &completion));
+    completion.Wait();
+    return result;
+  }
+
+ protected:
+  static void DoCreateFromConsumer(Consumer** consumer,
+                                   const Consumer* other,
+                                   WaitableEvent* completion) {
+    *consumer = new Consumer;
+    **consumer = *other;
+    completion->Signal();
+  }
+
+  static void DoCreateFromProducer(Consumer** consumer,
+                                   Producer* producer,
+                                   WaitableEvent* completion) {
+    *consumer = new Consumer;
+    (*consumer)->producer = producer->AsWeakPtr();
+    completion->Signal();
+  }
+
+  static void DoDeRef(const Consumer* consumer,
+                      Producer** result,
+                      WaitableEvent* completion) {
+    *result = consumer->producer.get();
+    completion->Signal();
+  }
+
+  static void DoDeleteProducer(Producer* object, WaitableEvent* completion) {
+    delete object;
+    completion->Signal();
+  }
+
+  static void DoDeleteConsumer(Consumer* object, WaitableEvent* completion) {
+    delete object;
+    completion->Signal();
+  }
+};
+
 }  // namespace
 
 TEST(WeakPtrTest, Basic) {
@@ -148,4 +247,98 @@ TEST(WeakPtrTest, SingleThreaded2) {
   EXPECT_EQ(&producer, consumer->producer.get());
 }
 
+TEST(WeakPtrTest, MoveOwnershipImplicit) {
+  // Move object ownership to other thread by releasing all weak pointers
+  // on the original thread first. Establishing weak pointers on a different
+  // thread after previous pointers have been destroyed implicitly reattaches
+  // the thread checks.
+  // - Thread A creates object and weak pointer
+  // - Thread A deletes the weak pointer
+  // - Thread B creates weak pointer
+  // - Thread B derefs weak pointer
+  // - Thread B deletes object
+  BackgroundThread thread;
+  thread.Start();
+  Producer* producer = new Producer();
+  {
+    WeakPtr<Producer> weak_ptr = producer->AsWeakPtr();
+  }
+  Consumer* consumer;
+  thread.CreateConsumerFromProducer(&consumer, producer);
+  EXPECT_EQ(thread.DeRef(consumer), producer);
+  thread.DeleteProducer(producer);
+  thread.DeleteConsumer(consumer);
+}
+
+TEST(WeakPtrTest, MoveOwnershipExplicit) {
+  // Test that we do not trip any checks if we establish weak references
+  // on one thread and delete the object on another thread after explicit
+  // detachment.
+  // - Thread A creates object
+  // - Thread B creates weak pointer
+  // - Thread B releases weak pointer
+  // - Detach owner from Thread B
+  // - Thread A destroys object
+  BackgroundThread thread;
+  thread.Start();
+  Producer producer;
+  Consumer* consumer;
+  thread.CreateConsumerFromProducer(&consumer, &producer);
+  EXPECT_EQ(thread.DeRef(consumer), &producer);
+  thread.DeleteConsumer(consumer);
+  producer.DetachFromThread();
+}
+
+TEST(WeakPtrTest, ThreadARefOutlivesThreadBRef) {
+  // Originating thread has a WeakPtr that outlives others.
+  // - Thread A creates WeakPtr<> and passes copy to Thread B
+  // - Destruct the pointer on Thread B
+  // - Destruct the pointer on Thread A
+  BackgroundThread thread;
+  thread.Start();
+  Producer producer;
+  Consumer consumer;
+  consumer.producer = producer.AsWeakPtr();
+  Consumer* consumer_copy;
+  thread.CreateConsumerFromConsumer(&consumer_copy, &consumer);
+  EXPECT_EQ(consumer_copy->producer, &producer);
+  thread.DeleteConsumer(consumer_copy);
+}
+
+TEST(WeakPtrTest, ThreadBRefOutlivesThreadARef) {
+  // Originating thread drops all references before another thread.
+  // - Thread A creates WeakPtr<> and passes copy to Thread B
+  // - Destruct the pointer on Thread A
+  // - Destruct the pointer on Thread B
+  BackgroundThread thread;
+  thread.Start();
+  Producer producer;
+  Consumer* consumer_copy;
+  {
+    Consumer consumer;
+    consumer.producer = producer.AsWeakPtr();
+    thread.CreateConsumerFromConsumer(&consumer_copy, &consumer);
+  }
+  EXPECT_EQ(consumer_copy->producer, &producer);
+  thread.DeleteConsumer(consumer_copy);
+}
+
+TEST(WeakPtrTest, OwnerThreadDeletesObject) {
+  // Originating thread invalidates WeakPtrs while its held by other thread.
+  // - Thread A creates WeakPtr<> and passes Copy to Thread B
+  // - WeakReferenceOwner gets destroyed on Thread A
+  // - WeakPtr gets destroyed on Thread B
+  BackgroundThread thread;
+  thread.Start();
+  Consumer* consumer_copy;
+  {
+    Producer producer;
+    Consumer consumer;
+    consumer.producer = producer.AsWeakPtr();
+    thread.CreateConsumerFromConsumer(&consumer_copy, &consumer);
+  }
+  EXPECT_TRUE(consumer_copy->producer == NULL);
+  thread.DeleteConsumer(consumer_copy);
+}
+
 }  // namespace base

tools/valgrind/memcheck/suppressions.txt

@@ -5359,6 +5359,16 @@
    fun:_ZN12GpuBlacklist16LoadGpuBlacklistERKSsNS_8OsFilterE
 }
 
+{
+   bug_94345
+   Memcheck:Leak
+   fun:_Znw*
+   fun:_ZNK4base8internal18WeakReferenceOwner6GetRefEv
+   fun:_ZN4base15SupportsWeakPtrI16ObserverListBaseIN11MessageLoop12TaskObserverEEE9AsWeakPtrEv
+   fun:_ZN16ObserverListBaseIN11MessageLoop12TaskObserverEE8IteratorC1ERS2_
+   fun:_ZN11MessageLoop7RunTaskERKNS_11PendingTaskE
+}
+
 #-----------------------------------------------------------------------
 # These only occur on our Google workstations
 {