[Sync] Avoid leaking in WeakHandle even when the owner thread is gone

chrome/browser/sync/weak_handle.h

Index: chrome/browser/sync/weak_handle.h
diff --git a/chrome/browser/sync/weak_handle.h b/chrome/browser/sync/weak_handle.h
index 7c8c240183229033658440e9539325ffb46f288f..b6385eb04470f795f4d4f2cd2e1ef642d1baacef 100644
--- a/chrome/browser/sync/weak_handle.h
+++ b/chrome/browser/sync/weak_handle.h
@@ -57,9 +57,13 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/message_loop_proxy.h"
+#include "base/message_loop.h"
 #include "base/tracked.h"
 
+namespace base {
+class MessageLoopProxy;
+}  // namespace base
+
 namespace tracked_objects {
 class Location;
 }  // namespace tracked_objects
@@ -94,59 +98,81 @@ struct ParamTraits<T[]> {
   typedef const T* ForwardType;
 };
 
+class WeakHandleCoreBase;
+
+struct WeakHandleCoreBaseTraits {
+  static void Destruct(const WeakHandleCoreBase* core_base);
+};
+
 // Base class for WeakHandleCore<T> to avoid template bloat.  Handles
-// the trampolining to the MessageLoopProxy for the owner thread.
-//
-// This class is thread-safe.
-class WeakHandleCoreBase {
+// the interaction with the owner thread and its message loop.
+class WeakHandleCoreBase
+    : public MessageLoop::DestructionObserver,
+      public base::RefCountedThreadSafe<WeakHandleCoreBase,
+                                        WeakHandleCoreBaseTraits> {
  public:
   // Assumes the current thread is the owner thread.
   WeakHandleCoreBase();
 
+  // May be called on any thread.
   bool IsOnOwnerThread() const;
 
- protected:
-  ~WeakHandleCoreBase();
+  // MessageLoop::DestructionObserver implementation.  Must be called
+  // on the owner thread.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
 
-  void PostOnOwnerThread(const tracked_objects::Location& from_here,
+ protected:
+  // May be deleted on any thread, but only via DestroyAndDelete()
+  // which is called by our traits class.
+  virtual ~WeakHandleCoreBase();
+
+  // This is called exactly once on the owner thread right before this
+  // object is destroyed or when the owner thread is destroyed,
+  // whichever comes first.  If you override this, make to still call

[Nicolas Zea, 2011/08/04 19:54:10]

"make sure to still"

[akalin, 2011/08/04 21:56:53]

Rendered moot.

+  // WeakHandleCoreBase::DestroyOnOwnerThread());
+  virtual void DestroyOnOwnerThread();
+
+  // May be called on any thread.
+  void PostToOwnerThread(const tracked_objects::Location& from_here,
                          const base::Closure& fn) const;
 
-  template <typename T>
-  void DeleteOnOwnerThread(const tracked_objects::Location& from_here,
-                           const T* ptr) const {
-    if (IsOnOwnerThread()) {
-      delete ptr;
-    } else {
-      ignore_result(message_loop_proxy_->DeleteSoon(from_here, ptr));
-    }
-  }
-
  private:
-  const scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
+  friend struct WeakHandleCoreBaseTraits;
+
+  // May be called on any thread, but only via
+  // WeakHandleCoreBaseTraits::Destruct().  Destroys this object.
+  void DestroyAndDelete();

[Nicolas Zea, 2011/08/04 19:54:10]

I find the difference between "destroy" and "delete" to be confusing. Why
doesn't destroy already delete? What exactly is destroy doing other than delete?

[akalin, 2011/08/04 21:56:53]

Good point.  Renamed 'Destroy' to 'Cleanup' and 'Delete' to 'Destroy'.  I think
that makes more sense.

+
+  // Calls DestroyOnOwnerThread() and deletes |this|.  Must be called
+  // on the owner thread via DestroyAndDelete().
+  void DestroyAndDeleteOnOwnerThread();
+
+  // May be read on any thread, but should only be dereferenced on the
+  // owner thread.
+  MessageLoop* const owner_loop_;
+
+  // May be used on any thread.
+  const scoped_refptr<base::MessageLoopProxy> owner_loop_proxy_;
+
+  // Should only be read on the owner thread or in the destructor.
+  bool destroyed_on_owner_thread_;

[Nicolas Zea, 2011/08/04 19:54:10]

Is this variable just used for the purpose of CHECK's? It doesn't seem to be
used in any actual logic.

[akalin, 2011/08/04 21:56:53]

Yes.  I think it's still worth having, though.

 
   DISALLOW_COPY_AND_ASSIGN(WeakHandleCoreBase);
 };
 
 // WeakHandleCore<T> contains all the logic for WeakHandle<T>.
 template <typename T>
-class WeakHandleCore
-    : public WeakHandleCoreBase,
-      public base::RefCountedThreadSafe<WeakHandleCore<T> > {
+class WeakHandleCore : public WeakHandleCoreBase {
  public:
   // Must be called on |ptr|'s owner thread, which is assumed to be
   // the current thread.
   explicit WeakHandleCore(const base::WeakPtr<T>& ptr)
       : ptr_(new base::WeakPtr<T>(ptr)) {}
 
-  // May be destroyed on any thread.
-  ~WeakHandleCore() {
-    DeleteOnOwnerThread(FROM_HERE, ptr_);
-  }
-
   // Must be called on |ptr_|'s owner thread.
-  const base::WeakPtr<T>& Get() const {
+  base::WeakPtr<T> Get() const {
     CHECK(IsOnOwnerThread());
-    return *ptr_;
+    return ptr_ ? *ptr_ : base::WeakPtr<T>();
   }
 
   // Call(...) may be called on any thread, but all its arguments
@@ -155,7 +181,7 @@ class WeakHandleCore
   template <typename U>
   void Call(const tracked_objects::Location& from_here,
             void (U::*fn)(void)) const {
-    PostOnOwnerThread(
+    PostToOwnerThread(
         from_here,
         Bind(&WeakHandleCore::template DoCall0<U>, this, fn));
   }
@@ -164,7 +190,7 @@ class WeakHandleCore
   void Call(const tracked_objects::Location& from_here,
             void (U::*fn)(A1),
             typename ParamTraits<A1>::ForwardType a1) const {
-    PostOnOwnerThread(
+    PostToOwnerThread(
         from_here,
         Bind(&WeakHandleCore::template DoCall1<U, A1>,
              this, fn, a1));
@@ -175,7 +201,7 @@ class WeakHandleCore
             void (U::*fn)(A1, A2),
             typename ParamTraits<A1>::ForwardType a1,
             typename ParamTraits<A2>::ForwardType a2) const {
-    PostOnOwnerThread(
+    PostToOwnerThread(
         from_here,
         Bind(&WeakHandleCore::template DoCall2<U, A1, A2>,
              this, fn, a1, a2));
@@ -187,7 +213,7 @@ class WeakHandleCore
             typename ParamTraits<A1>::ForwardType a1,
             typename ParamTraits<A2>::ForwardType a2,
             typename ParamTraits<A3>::ForwardType a3) const {
-    PostOnOwnerThread(
+    PostToOwnerThread(
         from_here,
         Bind(&WeakHandleCore::template DoCall3<U, A1, A2, A3>,
              this, fn, a1, a2, a3));
@@ -200,14 +226,29 @@ class WeakHandleCore
             typename ParamTraits<A2>::ForwardType a2,
             typename ParamTraits<A3>::ForwardType a3,
             typename ParamTraits<A4>::ForwardType a4) const {
-    PostOnOwnerThread(
+    PostToOwnerThread(
         from_here,
         Bind(&WeakHandleCore::template DoCall4<U, A1, A2, A3, A4>,
              this, fn, a1, a2, a3, a4));
   }
 
+ protected:
+  // Must be called on |ptr_|'s owner thread exactly once.
+  virtual void DestroyOnOwnerThread() OVERRIDE {
+    CHECK(IsOnOwnerThread());
+    CHECK(ptr_);
+    delete ptr_;
+    ptr_ = NULL;
+    WeakHandleCoreBase::DestroyOnOwnerThread();
+  }
+
  private:
-  friend class base::RefCountedThreadSafe<WeakHandleCore<T> >;
+  // May be called on any thread.
+  ~WeakHandleCore() {
+    // It is safe to read |ptr_| here even if we're not on the owner
+    // thread (see comments on base::AtomicRefCountDecN()).
+    CHECK(!ptr_);
+  }
 
   // GCC 4.2.1 on OS X gets confused if all the DoCall functions are
   // named the same, so we distinguish them.
@@ -215,20 +256,20 @@ class WeakHandleCore
   template <typename U>
   void DoCall0(void (U::*fn)(void)) const {
     CHECK(IsOnOwnerThread());
-    if (!ptr_->get()) {
+    if (!Get().get()) {

[Nicolas Zea, 2011/08/04 19:54:10]

:(
Maybe Get() can  be changed to GetScopedPtr()?

[akalin, 2011/08/04 21:56:53]

WeakPtr<T> can be cast to T*, so just changed to Get().  (it's a weakptr btw,
not a scoped_ptr)

       return;
     }
-    (ptr_->get()->*fn)();
+    (Get()->*fn)();
   }
 
   template <typename U, typename A1>
   void DoCall1(void (U::*fn)(A1),
                typename ParamTraits<A1>::ForwardType a1) const {
     CHECK(IsOnOwnerThread());
-    if (!ptr_->get()) {
+    if (!Get().get()) {
       return;
     }
-    (ptr_->get()->*fn)(a1);
+    (Get()->*fn)(a1);
   }
 
   template <typename U, typename A1, typename A2>
@@ -236,10 +277,10 @@ class WeakHandleCore
                typename ParamTraits<A1>::ForwardType a1,
                typename ParamTraits<A2>::ForwardType a2) const {
     CHECK(IsOnOwnerThread());
-    if (!ptr_->get()) {
+    if (!Get().get()) {
       return;
     }
-    (ptr_->get()->*fn)(a1, a2);
+    (Get()->*fn)(a1, a2);
   }
 
   template <typename U, typename A1, typename A2, typename A3>
@@ -248,10 +289,10 @@ class WeakHandleCore
                typename ParamTraits<A2>::ForwardType a2,
                typename ParamTraits<A3>::ForwardType a3) const {
     CHECK(IsOnOwnerThread());
-    if (!ptr_->get()) {
+    if (!Get().get()) {
       return;
     }
-    (ptr_->get()->*fn)(a1, a2, a3);
+    (Get()->*fn)(a1, a2, a3);
   }
 
   template <typename U, typename A1, typename A2, typename A3, typename A4>
@@ -261,13 +302,14 @@ class WeakHandleCore
                typename ParamTraits<A3>::ForwardType a3,
                typename ParamTraits<A4>::ForwardType a4) const {
     CHECK(IsOnOwnerThread());
-    if (!ptr_->get()) {
+    if (!Get().get()) {
       return;
     }
-    (ptr_->get()->*fn)(a1, a2, a3, a4);
+    (Get()->*fn)(a1, a2, a3, a4);
   }
 
-  // Must be used and destroyed only on the owner thread.
+  // Must be dereferenced and destroyed only on the owner thread.
+  // Must be read only on the owner thread or the destructor.
   base::WeakPtr<T>* ptr_;
 
   DISALLOW_COPY_AND_ASSIGN(WeakHandleCore);