Revert 97808 - 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

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

TBR=sievers@chromium.org
Review URL: http://codereview.chromium.org/7685054

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

3 files changed, 18 insertions, 220 deletions

diff --git a/base/memory/weak_ptr.cc b/base/memory/weak_ptr.cc
index 9dec8fd..30c777c 100644
--- a/base/memory/weak_ptr.cc
+++ b/base/memory/weak_ptr.cc
@@ -7,28 +7,28 @@
 namespace base {
 namespace internal {
 
-WeakReference::Flag::Flag() : is_valid_(true) {
+WeakReference::Flag::Flag(Flag** handle) : handle_(handle) {
 }
 
 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());
-  is_valid_ = false;
+  DCHECK(thread_checker_.CalledOnValidThread());
+  handle_ = NULL;
 }
 
 bool WeakReference::Flag::IsValid() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return is_valid_;
+  return handle_ != NULL;
 }
 
 WeakReference::Flag::~Flag() {
+  if (handle_)
+    *handle_ = NULL;
 }
 
 WeakReference::WeakReference() {
 }
 
-WeakReference::WeakReference(const Flag* flag) : flag_(flag) {
+WeakReference::WeakReference(Flag* flag) : flag_(flag) {
 }
 
 WeakReference::~WeakReference() {
@@ -38,7 +38,7 @@ bool WeakReference::is_valid() const {
   return flag_ && flag_->IsValid();
 }
 
-WeakReferenceOwner::WeakReferenceOwner() {
+WeakReferenceOwner::WeakReferenceOwner() : flag_(NULL) {
 }
 
 WeakReferenceOwner::~WeakReferenceOwner() {
@@ -46,10 +46,8 @@ WeakReferenceOwner::~WeakReferenceOwner() {
 }
 
 WeakReference WeakReferenceOwner::GetRef() const {
-  // We also want to reattach to the current thread if all previous references
-  // have gone away.
-  if (!HasRefs())
-    flag_ = new WeakReference::Flag();
+  if (!flag_)
+    flag_ = new WeakReference::Flag(&flag_);
   return WeakReference(flag_);
 }
 
diff --git a/base/memory/weak_ptr.h b/base/memory/weak_ptr.h
index bdfc308..abda1af 100644
--- a/base/memory/weak_ptr.h
+++ b/base/memory/weak_ptr.h
@@ -7,15 +7,6 @@
 // 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
@@ -54,6 +45,8 @@
 // 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_
@@ -76,7 +69,7 @@ class BASE_EXPORT WeakReference {
   // via base::WeakPtr::~WeakPtr().
   class Flag : public RefCountedThreadSafe<Flag> {
    public:
-    Flag();
+    explicit Flag(Flag** handle);
 
     void Invalidate();
     bool IsValid() const;
@@ -89,17 +82,17 @@ class BASE_EXPORT WeakReference {
     ~Flag();
 
     ThreadChecker thread_checker_;
-    bool is_valid_;
+    Flag** handle_;
   };
 
   WeakReference();
-  explicit WeakReference(const Flag* flag);
+  WeakReference(Flag* flag);
   ~WeakReference();
 
   bool is_valid() const;
 
  private:
-  scoped_refptr<const Flag> flag_;
+  scoped_refptr<Flag> flag_;
 };
 
 class BASE_EXPORT WeakReferenceOwner {
@@ -110,7 +103,7 @@ class BASE_EXPORT WeakReferenceOwner {
   WeakReference GetRef() const;
 
   bool HasRefs() const {
-    return flag_.get() && !flag_->HasOneRef();
+    return flag_ != NULL;
   }
 
   void Invalidate();
@@ -121,7 +114,7 @@ class BASE_EXPORT WeakReferenceOwner {
   }
 
  private:
-  mutable scoped_refptr<WeakReference::Flag> flag_;
+  mutable WeakReference::Flag* flag_;
 };
 
 // This class simplifies the implementation of WeakPtr's type conversion
diff --git a/base/memory/weak_ptr_unittest.cc b/base/memory/weak_ptr_unittest.cc
index c244ec0..c1a9526 100644
--- a/base/memory/weak_ptr_unittest.cc
+++ b/base/memory/weak_ptr_unittest.cc
@@ -6,7 +6,6 @@
 #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 {
@@ -39,104 +38,6 @@ 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) {
@@ -247,98 +148,4 @@ 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