Change code to use ScopedRunnableMethodFactory & a static callback

media/base/composite_filter.cc

Index: media/base/composite_filter.cc
diff --git a/media/base/composite_filter.cc b/media/base/composite_filter.cc
index 87ff799fa060cfb8682d5094df4d8a38614b6484..94665fd97925388f206f8a7974d3cf446a3e4e4e 100644
--- a/media/base/composite_filter.cc
+++ b/media/base/composite_filter.cc
@@ -55,6 +55,8 @@ void CompositeFilter::Init(MessageLoop* message_loop,
   DCHECK(message_loop);
   message_loop_ = message_loop;
   thread_factory_ = thread_factory;
+  runnable_factory_.reset(
+      new ScopedRunnableMethodFactory<CompositeFilter>(this));
 
   if (!thread_factory_) {
     thread_factory_ = &CompositeFilter::DefaultThreadFactory;
@@ -76,9 +78,6 @@ CompositeFilter::~CompositeFilter() {
     (*iter)->Stop();
   }
 
-  // Reset the pipeline, which will decrement a reference to this object.
-  // We will get destroyed as soon as the remaining tasks finish executing.
-  // To be safe, we'll set our pipeline reference to NULL.

[acolwell GONE FROM CHROMIUM, 2011/01/10 22:31:31]

Removed because this comment is no longer relevant and simply a result of
copying & pasting.

   filters_.clear();
   STLDeleteElements(&filter_threads_);
 }
@@ -470,21 +469,28 @@ void CompositeFilter::HandleError(PipelineError error) {
 FilterCallback* CompositeFilter::NewThreadSafeCallback(
     void (CompositeFilter::*method)()) {
   return TaskToCallbackAdapter::NewCallback(
-      NewRunnableMethod(this,
-                        &CompositeFilter::OnCallback,
-                        message_loop_,
-                        method));
-}
-
+      NewRunnableFunction(&CompositeFilter::OnCallback,
+                          message_loop_,
+                          runnable_factory_->NewRunnableMethod(method)));
+}
+
+// This method is intentionally static so that no reference to the composite
+// is needed to call it. This method may be called by other threads and we
+// don't want those threads to gain ownership of this composite by having a
+// reference to it. |task| will contain a weak reference to the composite
+// so that the reference can be cleared if the composite is destroyed before
+// the callback is called.
+// static
 void CompositeFilter::OnCallback(MessageLoop* message_loop,

[scherkus (not reviewing), 2011/01/11 00:36:57]

few sanity checks...

Is the issue that someone is simply holding onto a reference as opposed to
executing the task after CompositeFilter should have died?

Can we make any guarantees of the lifetime of CompositeFilter?  I know
CompositeFilter inherits from Filter, which includes RefCountedThreadSafe but
say if it didn't (somehow!) we could declare CompositeFilter with
DISABLE_RUNNABLE_METHOD_REFCOUNT() and then this problem would go away (but that
only really works if we're positive that any potential callees have been
destroyed and/or won't call CompositeFilter)

[acolwell GONE FROM CHROMIUM, 2011/01/11 05:23:59]

The issue is that this code was accidentally opening the possibility of
transferring ownership to a different thread. This can cause all sorts of
problems because filters & threads owned by the composite can end up outliving
the pipeline & pipeline thread in extreme situations.
For all intents and purposes the PipelineImpl owns the composite. Ideally I
would like to get rid of the RefCountedThreadSafe in Filter and make ownership a
lot more straightforward and predictable. I just didn't want to make that change
yet. I figured that addressing the current flakiness was preferable to doing
something that could trigger other issues to appear. 

[scherkus (not reviewing), 2011/01/11 18:37:32]

Gotcha!
Agree 100% -- we haven't found an explicit need for ref counting and guess what!
 it can cause bugs like this!

-                                 void (CompositeFilter::*method)()) {
+                                 CancelableTask* task) {
   if (MessageLoop::current() != message_loop) {
     // Posting callback to the proper thread.
-    message_loop->PostTask(FROM_HERE, NewRunnableMethod(this, method));
+    message_loop->PostTask(FROM_HERE, task);
     return;
   }
 
-  (this->*method)();
+  task->Run();
+  delete task;
 }
 
 bool CompositeFilter::CanForwardError() {