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..5a57776b1b25ade80e0cab5531dc5e2805e92f0c 100644
--- a/media/base/bind_to_current_loop.h
+++ b/media/base/bind_to_current_loop.h
@@ -7,8 +7,11 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/single_thread_task_runner.h"
+#include "media/base/media_export.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
@@ -17,17 +20,24 @@
 //
 // Typical usage: request to be called back on the current thread:
 // other->StartAsyncProcessAndCallMeBack(
-//    media::BindToCurrentLoop(base::Bind(&MyClass::MyMethod, this)));
+//    media::base::BindToCurrentLoop(base::Bind(&MyClass::MyMethod, this)));

[danakj, 2015/04/29 23:35:01]

no base::?

[johnme, 2015/04/30 16:15:53]

Done.

 //
 // 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 +49,76 @@ 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;
-
-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)...));
+}  // namespace internal
+
+// Function object which deletes its parameter, which must be a pointer,
+// on the task runner on which this object was constructed.
+//
+// This is more expensive than content::BrowserThread::DeleteOnUIThread etc;
+// use those instead when possible.
+//
+// Sample usage with RefCountedThreadSafe:
+// class Foo : public base::RefCountedThreadSafe<
+//                 Foo, media::base::DeleteOnCurrentLoop> {
+// ...
+//  private:
+//   friend struct media::base::DeleteOnCurrentLoop;
+//   friend class base::DeleteHelper<Foo>;
+//   ~Foo();
+// }
+//
+// Sample usage with scoped_ptr:
+// scoped_ptr<Foo, media::base::DeleteOnCurrentLoop> ptr;

[danakj, 2015/04/29 23:35:01]

no base::?

[johnme, 2015/04/30 16:15:53]

Done (ditto above).

+struct MEDIA_EXPORT DeleteOnCurrentLoop {

[danakj, 2015/04/29 23:35:01]

should this be inside anon/internal?

[johnme, 2015/04/30 16:15:53]

I've moved DeleteOnLoop into internal (a subclass I created for your comment
below about explicitly passing in the loop).

However I think DeleteOnCurrentLoop could be useful to expose. Almost every
RefCountedThreadSafe has thread-racey destruction, except for those that use
content::BrowserThread::DeleteOn*Thread as traits. But you can only use those
traits from parts of the codebase that have access to content/public; this could
be an essential companion to RefCountedThreadSafe everywhere else.

Maybe I should save DeleteOnCurrentLoop for a follow-up patch though, so it can
be discussed on its own (and only include internal::DeleteOnLoop in this patch)?

+  DeleteOnCurrentLoop();

[danakj, 2015/04/29 23:35:01]

i prefer this take a STTR* as an argument instead of finding the current one. Do
that finding in BindToCurrentLoop

[johnme, 2015/04/30 16:15:53]

Done.

+  ~DeleteOnCurrentLoop();
+
+  template <typename T>
+  void operator()(T* ptr) const {
+    enum { type_must_be_complete = sizeof(T) };
+    if (origin_loop_->BelongsToCurrentThread()) {

[danakj, 2015/04/29 23:37:34]

oh and no conditional posting please

[johnme, 2015/04/30 16:15:53]

I modeled this on BrowserThread::DeleteOnThread, which does conditional
DeleteSoon:

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

The original code was written by jam/darin:

https://codereview.chromium.org/338065

I'm guessing their reasoning is that we may as well delete it if we have the
chance, whereas if we delay it may no longer be possible to delete it.

I tried removing the conditional and always using DeleteSoon, but that causes
the following tests to fail:
    NullVideoSinkTest.BasicFunctionality
    VideoRendererImplTest.StartPlayingFrom_LowDelay
    VideoRendererImplTest.Underflow

These unit tests each have a MessageLoop owned by the test class. They wait for
some event on the message loop, then return, which causes the test class to be
destroyed. Hence the MessageLoop is destroyed, and hence the DeleteOnLoop
scoped_ptr to the callback ends up being destructed. With the conditional this
is fine -- it's already on the right thread, so it goes ahead and synchronously
deletes the callback. But without the conditional, it tries to send a DeleteSoon
to the being-deleted message loop, which fails and our LOG(FATAL) causes the
test to crash.

+      delete ptr;
+    } else {
+      if (!origin_loop_->DeleteSoon(FROM_HERE, ptr)) {
+#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/29 23:35:01]

I'd still prefer LOG(FATAL) and maybe a way to turn this off for a specific test
if it really shows itself as an unsolvable problem? So far I think there's only
speculation. Maybe a "SetLeakOnDestroy" or something if we needed it, and it
could just not try delete then or something even.

[johnme, 2015/04/30 16:15:53]

Done (hmm ok, I re-read https://codereview.chromium.org/6691044/ and it seems
the key point is the UNIT_TEST guards, but FATAL should be ok since we've got
those).

+#endif  // UNIT_TEST
+      }
+    }
   }
+
+  private:
+    scoped_refptr<base::SingleThreadTaskRunner> origin_loop_;
 };
 
-}  // namespace internal
+namespace {

[danakj, 2015/04/29 23:35:01]

why is this anon but the above is internal?

[johnme, 2015/04/30 16:15:53]

Done (oops, shouldn't use anonymous namespaces in a header; and template
function definitions have to go in the header. So I've moved
PostBackToOriginLoop to internal:: as well)

+
+template<typename... A>
+void PostBackToOriginLoop(
+    const scoped_refptr<base::SingleThreadTaskRunner>& origin_task_runner,
+    scoped_ptr<base::Callback<void(A...)>,
+               DeleteOnCurrentLoop>* origin_cb_ptr_ptr,
+    A... args) {
+  origin_task_runner->PostTask(

[danakj, 2015/04/29 23:35:01]

I think that I prefer posting to our own method on the origin thread here, and
passing ownership of the callback there to run it, rather than posting the
original callback here, and letting the destructor handle the posttask. Then we
don't need the custom deletor?

[johnme, 2015/04/30 16:15:53]

We'd still need the custom deleter in case it doesn't get run at all. So I don't
think this gains us much?

+      FROM_HERE, base::Bind(**origin_cb_ptr_ptr,
+                            internal::TrampolineForward(args)...));
+}
+
+}  // 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... A>
+static base::Callback<void(A...)> BindToCurrentLoop(
+    const base::Callback<void(A...)>& origin_cb) {
+  auto cb_copy_ptr = new base::Callback<void(A...)>(origin_cb);
+  // Normally it's pointless to create a scoped_ptr on the heap; here we're just
+  // (ab)using it to take advantage of its custom deleter support.
+  auto cb_copy_ptr_ptr = new scoped_ptr<base::Callback<void(A...)>,

[danakj, 2015/04/29 23:35:01]

can we do this with a scoped_ptr Passed instead of a scoped_ptr* which is making
this harder to understand. it'll make the callback only callable once, but
that's maybe okay?

[johnme, 2015/04/30 16:15:53]

Only allowing the callback to be called once breaks 57 tests in media_unittests.

+                                        DeleteOnCurrentLoop>(cb_copy_ptr);
+  return base::Bind(&PostBackToOriginLoop<A...>,
+                    base::MessageLoop::current()->task_runner(),

[danakj, 2015/04/29 23:35:01]

ThreadTaskRunnerHandler::Get()?

[johnme, 2015/04/30 16:15:53]

Done (and in .cc).

+                    base::Owned(cb_copy_ptr_ptr));
 }
 
 }  // namespace media