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

chrome/browser/sync/weak_handle.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Weak handles provides a way to refer to weak pointers from another
 // thread.  This is useful because it is not safe to reference a weak
 // pointer from a thread other than the thread on which it was
 // created.
 //
 // Weak handles can be passed across threads, so for example, you can
@@ skipping 39 matching lines @@
 
 #include <cstddef>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #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
 
 namespace browser_sync {
 
 template <typename T> class WeakHandle;
 
 namespace internal {
 // These classes are part of the WeakHandle implementation.  DO NOT
@@ skipping 14 matching lines @@
 template <typename T, size_t n>
 struct ParamTraits<T[n]> {
   typedef const T* ForwardType;
 };
 
 template <typename T>
 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;
 
+  // MessageLoop::DestructionObserver implementation.  Must be called
+  // on the owner thread.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
+
  protected:
-  ~WeakHandleCoreBase();
+  // May be deleted on any thread, but only via DestroyAndDelete()
+  // which is called by our traits class.
+  virtual ~WeakHandleCoreBase();
 
-  void PostOnOwnerThread(const tracked_objects::Location& from_here,
+  // 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:
+  friend struct WeakHandleCoreBaseTraits;
 
- private:
-  const scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
+  // 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
   // should be safe to be bound and copied across threads.
 
   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));
   }
 
   template <typename U, typename A1>
   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));
   }
 
   template <typename U, typename A1, typename A2>
   void Call(const tracked_objects::Location& from_here,
             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));
   }
 
   template <typename U, typename A1, typename A2, typename A3>
   void Call(const tracked_objects::Location& from_here,
             void (U::*fn)(A1, A2, A3),
             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));
   }
 
   template <typename U, typename A1, typename A2, typename A3, typename A4>
   void Call(const tracked_objects::Location& from_here,
             void (U::*fn)(A1, A2, A3, A4),
             typename ParamTraits<A1>::ForwardType a1,
             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.
 
   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>
   void DoCall2(void (U::*fn)(A1, A2),
                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>
   void DoCall3(void (U::*fn)(A1, A2, A3),
                typename ParamTraits<A1>::ForwardType a1,
                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>
   void DoCall4(void (U::*fn)(A1, A2, A3, A4),
                typename ParamTraits<A1>::ForwardType a1,
                typename ParamTraits<A2>::ForwardType a2,
                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);
 };
 
 }  // namespace internal
 
 // May be destroyed on any thread.
 // Copying and assignment are welcome.
 template <typename T>
@@ skipping 79 matching lines @@
 
 // Makes a WeakHandle from a WeakPtr.
 template <typename T>
 WeakHandle<T> MakeWeakHandle(const base::WeakPtr<T>& ptr) {
   return WeakHandle<T>(ptr);
 }
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_WEAK_HANDLE_H_