BindToCurrentLoop should delete callback on original thread

media/base/bind_to_current_loop.h

Index: media/base/bind_to_current_loop.h
diff --git a/media/base/bind_to_current_loop.h b/media/base/bind_to_current_loop.h
index cb3e28d4839641f3745684fc7757f87c3aa4cba9..e8320fc0ba86b031a6b21117529f4ec6ac231e72 100644
--- a/media/base/bind_to_current_loop.h
+++ b/media/base/bind_to_current_loop.h
@@ -7,8 +7,10 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 
 // This is a helper utility for base::Bind()ing callbacks to the current
 // MessageLoop. The typical use is when |a| (of class |A|) wants to hand a
@@ -22,12 +24,19 @@
 // Note that like base::Bind(), BindToCurrentLoop() can't bind non-constant
 // references, and that *unlike* base::Bind(), BindToCurrentLoop() makes copies
 // of its arguments, and thus can't be used with arrays.
+//
+// The callback passed in to BindToCurrentLoop is guaranteed to be deleted on
+// the thread from which BindToCurrentLoop was invoked. This allows objects that
+// must be deleted on the originating thread to be bound into it. In particular,
+// it can be useful to use WeakPtr<> in the callback so that the reply operation
+// can be canceled.
 
 namespace media {
 
+namespace internal {
+
 // Mimic base::internal::CallbackForward, replacing p.Pass() with
 // base::Passed(&p) to account for the extra layer of indirection.
-namespace internal {
 template <typename T>
 T& TrampolineForward(T& t) { return t; }
 
@@ -39,29 +48,56 @@ template <typename T>
 base::internal::PassedWrapper<ScopedVector<T> > TrampolineForward(
     ScopedVector<T>& p) { return base::Passed(&p); }
 
-// First, tell the compiler TrampolineHelper is a struct template with one
-// type parameter.  Then define specializations where the type is a function
-// returning void and taking zero or more arguments.
-template <typename Sig> struct TrampolineHelper;
+}  // namespace internal
+
+namespace {
 
 template <typename... Args>
-struct TrampolineHelper<void(Args...)> {
-  static void Run(
-      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-      const base::Callback<void(Args...)>& cb,
-      Args... args) {
-    task_runner->PostTask(FROM_HERE,
-                          base::Bind(cb, TrampolineForward(args)...));
+class BindToCurrentLoopRelay {
+ public:
+  BindToCurrentLoopRelay(const base::Callback<void(Args...)>& cb)
+      // TODO(johnme): Caller of BindToCurrentLoop should pass in FROM_HERE.
+      : from_here_(FROM_HERE),
+        origin_loop_(base::ThreadTaskRunnerHandle::Get()),
+        cb_ptr_(new base::Callback<void(Args...)>(cb)) {}
+
+  ~BindToCurrentLoopRelay() {
+    if (origin_loop_->BelongsToCurrentThread()) {
+      delete cb_ptr_;
+    } else if (!origin_loop_->DeleteSoon(from_here_, cb_ptr_)) {

[danakj, 2015/04/24 19:00:03]

I think you want FROM_HERE here? Or why do you use from_here_ here?

[johnme, 2015/04/27 18:32:33]

Ok, I've changed both the |from_here_|s to FROM_HERE. I kept the comment though,
as I think it would be better if BindToCurrentLoop took a
tracked_objects::Location parameter, but that's a separate refactoring.

+      // If DeleteSoon is not possible because the originating MessageLoop is no
+      // longer available, the Callback is leaked. Leaking is considered
+      // preferable to having thread-safety violations caused by invoking the
+      // Callback destructor on the wrong thread.
+#if defined(UNIT_TEST)
+      // Only logged under unit testing because leaks at shutdown
+      // are acceptable under normal circumstances.
+      LOG(ERROR) << "DeleteSoon failed";

[danakj, 2015/04/24 19:00:03]

FATAL?

[johnme, 2015/04/27 18:32:33]

I copied this from https://codereview.chromium.org/6691044, where jam suggested
not to use FATAL, because we know about and are ok with leaks at shutdown, and
was worried that FATAL would introduce confusing flaky crashes during
development.

+#endif  // UNIT_TEST
+    }
+  }
+
+  void Run(Args... args) {
+    // Ignore return value; nothing we can do if PostTask fails.
+    origin_loop_->PostTask(
+        from_here_, base::Bind(*cb_ptr_, internal::TrampolineForward(args)...));

[danakj, 2015/04/24 19:00:03]

Would it be simpler to do this as follows?

template<Args...>
void RelayOnOtherThread(STTR* origin_task_runner,
                        scoped_ptr<Callback<...>> origin_copy_cb,
                        Args... args) {
  // closure holds a ref on anything in origin_copy_cb, so we can delete
origin_copy_cb on the wrong thread here.
  scoped_ptr<base::Closure> origin_closure(new Bind(*origin_copy_cb, args));
  origin_copy_cb.reset();
  origin_task_runner->PostTask(&RelayOnOriginThread<...>,
base::Passed(&closure));
}

void RelayOnOriginThread(STTR* origin_task_runner,
                         scoped_ptr<Closure> origin_closure) {
  origin_closure->Run(args);
  // origin_closure is destroyed on the origin thread here.
}

BindToCurrentLoop(const Callback<...>& origin_cb) {
  scoped_ptr<Callback<...>> origin_copy_cb = new Callback<...>(origin_cb);
  return Bind(&RelayOnOtherThread, TTRH::Get(), base::Passed(&origin_copy_cb));
}

It uses 1 templated function instead of a templated function+struct, and it
deletes the callback when it runs it instead of posting a separate task.

[johnme, 2015/04/27 18:32:33]

If you delete the callback returned from BindToCurrentLoop on a different
thread, then AIUI the bound parameters will be deleted on that thread, and hence
origin_cb_copy will be deleted on the wrong thread. I thought avoiding that was
the whole point of this patch?

Also, using base::Passed to pass a scoped_ptr and deleting it when the callback
is run means the callback can only be run once, which is an odd restriction to
place on a general-purpose method like this.

Finally new Bind(*origin_copy_cb, args) tries to copy the arguments; the quirky
TrampolineForward does unfortunately seem to be necessary.

I tried to see if I could get something more along these lines, and I've
uploaded the closest I got as Patch Set 2
(https://codereview.chromium.org/1082113004/#ps20001). It introduces a
non-template struct called DeleteOnCurrentLoop (which might be of general
utility). But overall it seems less legible (due to the heap-allocated
scoped_ptr hack), so I reuploaded the original implementation (with the nits
from your other comments fixed) as Patch Set 3
(https://codereview.chromium.org/1082113004/#ps40001). If you prefer it, I'm
happy to go back to Patch Set 2 though.
Doesn't your BindToCurrentLoop(const Callback<...>& origin_cb) needs to be a
template function too?

   }
+
+ private:
+  tracked_objects::Location from_here_;
+  scoped_refptr<base::SingleThreadTaskRunner> origin_loop_;
+  base::Callback<void(Args...)>* cb_ptr_;  // Owned.
 };
 
-}  // namespace internal
+}  // namespace
 
-template<typename T>
-static base::Callback<T> BindToCurrentLoop(
-    const base::Callback<T>& cb) {
-  return base::Bind(&internal::TrampolineHelper<T>::Run,
-                    base::MessageLoopProxy::current(), cb);
+template<typename... Args>
+static base::Callback<void(Args...)> BindToCurrentLoop(
+    const base::Callback<void(Args...)>& cb) {
+  BindToCurrentLoopRelay<Args...>* relay =
+      new BindToCurrentLoopRelay<Args...>(cb);
+  return base::Bind(&BindToCurrentLoopRelay<Args...>::Run,
+                    base::Owned(relay));
 }
 
 }  // namespace media