Change code to use ScopedRunnableMethodFactory & a static callback

media/base/composite_filter.cc

 // Copyright (c) 2010 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.
 
 #include "media/base/composite_filter.h"
 
 #include "base/stl_util-inl.h"
 #include "media/base/callback.h"
 
 namespace media {
@@ skipping 37 matching lines @@
                                  ThreadFactoryFunction thread_factory) {
   DCHECK(thread_factory);
   Init(message_loop, thread_factory);
 }
 
 void CompositeFilter::Init(MessageLoop* message_loop,
                            ThreadFactoryFunction thread_factory) {
   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;
   }
 
   state_ = kCreated;
   sequence_index_ = 0;
   error_ = PIPELINE_OK;
 }
 
 CompositeFilter::~CompositeFilter() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
   DCHECK(state_ == kCreated || state_ == kStopped);
 
   // Stop every running filter thread.
   for (FilterThreadVector::iterator iter = filter_threads_.begin();
        iter != filter_threads_.end();
        ++iter) {
     (*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_);
 }
 
 bool CompositeFilter::AddFilter(scoped_refptr<Filter> filter) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
   if (!filter.get() || state_ != kCreated || !host())
     return false;
 
   // Create a dedicated thread for this filter if applicable.
@@ skipping 371 matching lines @@
   if (error != PIPELINE_OK) {
     SendErrorToHost(error);
   }
 
   DispatchPendingCallback();
 }
 
 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() {
   return (state_ == kCreated) || (state_ == kPlaying) || (state_ == kPaused);
 }
 
 void CompositeFilter::SetError(PipelineError error) {
   // TODO(acolwell): Temporary hack to handle errors that occur
   // during filter initialization. In this case we just forward
   // the error to the host even if it is on the wrong thread. We
@@ skipping 96 matching lines @@
 
 void CompositeFilter::FilterHostImpl::SetCurrentReadPosition(int64 offset) {
   host_->SetCurrentReadPosition(offset);
 }
 
 int64 CompositeFilter::FilterHostImpl::GetCurrentReadPosition() {
   return host_->GetCurrentReadPosition();
 }
 
 }  // namespace media