Fix a teardown hang caused by an Abort() call while there is a pending read.

Fix a teardown hang caused by an Abort() call while there is a pending read.


BUG=64754
TEST=BufferedDataSourceTest.StopDoesNotUseMessageLoopForCallback , BufferedDataSourceTest.AbortDuringPendingRead

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

[acolwell GONE FROM CHROMIUM, 2011-01-25 05:01:16]

Here are some changes that fix a teardown "deadlock" I found last week. 

Basically the pipeline won't complete pausing if there are outstanding reads.
Calling Abort() on the data source did not notify the downstream filters (Demux,
decoders, & renderers) that the read was never going to finish. All of these
filters sit there waiting forever for the read to complete. This was difficult
to detect because it only happens if the Abort() call occurs in the time between
a Read() call and ReadTask() being run.

To fix this problem, I simply made it so that the read_callback was available
for Abort() to call. I then had to arrange for the lock to cover all
read_callback_ accesses since it is now modified from multiple threads.

I added a unit test to reproduce this situation and verified that the old code
failed, and the new code passes.

I also added a test to make sure we don't introduce a different deadlock
scenario in the future. WebMediaClientImpl blocks the render thread while
waiting for the "stop done" callback from the pipeline. We must make sure that
the BufferedDataSource never requires a task to execute on the render thread
before calling the "stop done" callback. If it does, then the code will get
stuck because the task will not execute until "stop done" is called.

[Alpha Left Google, 2011-01-25 05:38:30]

The lock seems to be protecting read_callback in this case but I don't see a
strong reason to write read_callback on one thread (demuxer thread) and be read
from another thread (render thread).

Since Abort() is called on the same thread as ReadTask(), what about calling
read_callback(ERROR) when stopped_on_render_loop_ is true? Note that Abort() is
running before or after ReadTask() but never at the same time, the code only
handled the later case, it seems to me it's just the first case is failing.

[acolwell GONE FROM CHROMIUM, 2011-01-25 17:27:59]

On 2011/01/25 05:38:30, Alpha wrote:
> The lock seems to be protecting read_callback in this case but I don't see a
> strong reason to write read_callback on one thread (demuxer thread) and be
read
> from another thread (render thread).

The reason is because the render thread doesn't get to execute during teardown.
WebMediaPlayerImpl blocks the thread while waiting for the pipeline's Stop()
callback. Abort() is the only opportunity for code to be run on this thread in
the scenario I'm describing.

> 
> Since Abort() is called on the same thread as ReadTask(), what about calling
> read_callback(ERROR) when stopped_on_render_loop_ is true? Note that Abort()
is
> running before or after ReadTask() but never at the same time, the code only
> handled the later case, it seems to me it's just the first case is failing.

In the case I am describing here. Abort() will always run before ReadTask() and
ReadTask() won't ever execute until read_callback is called. This is the whole
reason I have to move the assignment to Read(). It needs to be available when
the Abort() call happens otherwise we get stuck.

[scherkus (not reviewing), 2011-01-26 03:04:10]

so I think this is LG but agreed crummy but I know this is the surgical strike
hot fix

I'd prefer the longer term fix but seeing as we'll be ripping apart buffing code
in the coming days/weeks I'm ok with this for the short term

http://codereview.chromium.org/6342018/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/6342018/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source_unittest.cc:509: ON_CALL(*stop_callback,
RunWithParams(_))
nice usage :)

to confirm this keeps the expectation but simply assigns an action?

[acolwell GONE FROM CHROMIUM, 2011-01-26 17:41:05]

I agree this isn't the greatest solution, but it is the minimal impact one. The
overall problem stems from filters not being able to cancel their pending reads
across the pipeline. I determined that fixing that problem was too invasive to
solve right now.

http://codereview.chromium.org/6342018/diff/1/webkit/glue/media/buffered_data...
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/6342018/diff/1/webkit/glue/media/buffered_data...
webkit/glue/media/buffered_data_source_unittest.cc:509: ON_CALL(*stop_callback,
RunWithParams(_))
On 2011/01/26 03:04:10, scherkus wrote:
> nice usage :)

Thanks.

> 
> to confirm this keeps the expectation but simply assigns an action?
Yes. The expectation stays in place. It just changes what happens when the
method is called.

[Alpha Left Google, 2011-01-26 18:43:10]

That sounds good to me. LGTM. Thanks for fixing this!

[annacc, 2011-01-26 19:01:52]

If it LG to Alpha and Andrew, it LTGM.  Thanks for looking into this, I wasn't
sure what to do with the Abort() problem.

[scherkus (not reviewing), 2011-01-26 19:40:21]

cool let's get it in!