Change code to use ScopedRunnableMethodFactory & a static callback

Change CompositeFilter to use ScopedRunnableMethodFactory &
make OnCallback() static to prevent the CompositeFilter from
becoming owned by other filter threads.

BUG=68784
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71067

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

Fix for flakiness in webkit tests caused by CompositeFilter. 

The demux & video renderer threads were gaining ownership of the composite when
the OnCallback() call for the "stop done" callback didn't return and release
it's reference before the pipeline thread shutdown and released it's reference
to the composite. The easiest way to expose this is to put a usleep(50000) in
OnCallback() right after the PostTask().

Once the demux and renderer thread had ownership all sorts of problems would
occur because the CompositeFilter expects to be destroyed on the pipeline
thread.

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc
File media/base/composite_filter.cc (left):

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc#...
media/base/composite_filter.cc:81: // To be safe, we'll set our pipeline
reference to NULL.
Removed because this comment is no longer relevant and simply a result of
copying & pasting.

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

tentative LGTM w/ some sanity checking questions!

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc#...
media/base/composite_filter.cc:484: void
CompositeFilter::OnCallback(MessageLoop* message_loop,
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:58]

Response to tentative LGTM comments.

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc#...
media/base/composite_filter.cc:484: void
CompositeFilter::OnCallback(MessageLoop* message_loop,
On 2011/01/11 00:36:57, scherkus wrote:
> 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?
> 

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.

> 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)

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:31]

LGTM x1000

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/6226001/diff/1/media/base/composite_filter.cc#...
media/base/composite_filter.cc:484: void
CompositeFilter::OnCallback(MessageLoop* message_loop,
On 2011/01/11 05:23:59, acolwell wrote:
> On 2011/01/11 00:36:57, scherkus wrote:
> > 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?
> > 
> 
> 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.

Gotcha!

> > 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)
> 
> 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. 

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