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

chrome/browser/sync/weak_handle.cc

Index: chrome/browser/sync/weak_handle.cc
diff --git a/chrome/browser/sync/weak_handle.cc b/chrome/browser/sync/weak_handle.cc
index 34eaab89f6cfdf8a9c4bb97c79ef174fedb64e52..0a2bd372bef03cf8b5a691d5d87269a7f657d016 100644
--- a/chrome/browser/sync/weak_handle.cc
+++ b/chrome/browser/sync/weak_handle.cc
@@ -4,24 +4,96 @@
 
 #include "chrome/browser/sync/weak_handle.h"
 
+#include <sstream>
+
+#include "base/message_loop_proxy.h"
+#include "base/tracked.h"
+
 namespace browser_sync {
 
 namespace internal {
 
 WeakHandleCoreBase::WeakHandleCoreBase()
-    : message_loop_proxy_(
-        base::MessageLoopProxy::CreateForCurrentThread()) {}
-
-WeakHandleCoreBase::~WeakHandleCoreBase() {}
+    : owner_loop_(MessageLoop::current()),
+      owner_loop_proxy_(base::MessageLoopProxy::CreateForCurrentThread()),
+      destroyed_on_owner_thread_(false) {
+  owner_loop_->AddDestructionObserver(this);
+}
 
 bool WeakHandleCoreBase::IsOnOwnerThread() const {
-  return message_loop_proxy_->BelongsToCurrentThread();
+  // We can't use |owner_loop_proxy_->BelongsToCurrentThread()| as
+  // it may not work from within a MessageLoop::DestructionObserver
+  // callback.
+  return MessageLoop::current() == owner_loop_;
+}
+
+void WeakHandleCoreBase::WillDestroyCurrentMessageLoop() {
+  CHECK(IsOnOwnerThread());
+  CHECK(!destroyed_on_owner_thread_);
+  DestroyOnOwnerThread();

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

What's the purpose to this call? All DestroyOnOwnerThread does is unregister
itself as a destruction observer right? Is that necessary when the loop is
already being destroyed?

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

Not quite.  It's overridden by WeakHandle (to destroy the WeakPtr).  Added
clarifying comment.

+  CHECK(destroyed_on_owner_thread_);
+}
+
+WeakHandleCoreBase::~WeakHandleCoreBase() {
+  // It is safe to read |destroyed_on_owner_thread_| here even if
+  // we're not on the owner thread (see comments on
+  // base::AtomicRefCountDecN()).
+  CHECK(destroyed_on_owner_thread_);
+}
+
+void WeakHandleCoreBase::DestroyOnOwnerThread() {
+  CHECK(IsOnOwnerThread());
+  CHECK(!destroyed_on_owner_thread_);
+  owner_loop_->RemoveDestructionObserver(this);
+  destroyed_on_owner_thread_ = true;
 }
 
-void WeakHandleCoreBase::PostOnOwnerThread(
+namespace {
+
+// TODO(akalin): Merge with similar function in
+// js_transaction_observer.cc.
+std::string GetLocationString(const tracked_objects::Location& location) {
+  std::ostringstream oss;
+  oss << location.function_name() << "@"
+      << location.file_name() << ":" << location.line_number();
+  return oss.str();
+}
+
+}  // namespace
+
+void WeakHandleCoreBase::PostToOwnerThread(
     const tracked_objects::Location& from_here,
     const base::Closure& fn) const {
-  ignore_result(message_loop_proxy_->PostTask(from_here, fn));
+  if (!owner_loop_proxy_->PostTask(from_here, fn)) {
+    VLOG(1) << "Could not post task from " << GetLocationString(from_here);
+  }
+}
+
+void WeakHandleCoreBase::DestroyAndDelete() {
+  if (IsOnOwnerThread()) {
+    CHECK(!destroyed_on_owner_thread_);
+    DestroyAndDeleteOnOwnerThread();
+  } else if (!owner_loop_proxy_->PostTask(
+      FROM_HERE,
+      base::Bind(&WeakHandleCoreBase::DestroyAndDeleteOnOwnerThread,
+                 base::Unretained(this)))) {
+    // If the post fails, that means that the owner loop is gone and
+    // therefore DestroyOnOwnerThread() should have already been
+    // called via WillDestroyCurrentMessageLoop().
+    delete this;
+  }
+}
+
+void WeakHandleCoreBase::DestroyAndDeleteOnOwnerThread() {
+  CHECK(IsOnOwnerThread());
+  CHECK(!destroyed_on_owner_thread_);
+  DestroyOnOwnerThread();
+  CHECK(destroyed_on_owner_thread_);
+  delete this;
+}
+
+void WeakHandleCoreBaseTraits::Destruct(const WeakHandleCoreBase* core_base) {
+  const_cast<WeakHandleCoreBase*>(core_base)->DestroyAndDelete();
 }
 
 }  // namespace internal