Fix 99596.

Fix 99596.

BUG=99596
TEST=New unit test. Play the failing video 500 times.

R=scherkus@chromium.org

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

[scherkus (not reviewing), 2011-10-14 21:34:54]

Idea for a unit test:

void ClearReference() {
  demuxer_ = NULL;  // should delete it
}

void ShouldNotGetExecuted() {
  NOTREACHED();
}

scoped_refptr<FFmpegDemuxer> demuxer_ = /* create one */
scoped_refptr<FFmpegDemuxerStream> stream_ = /* grab one */

demuxer->Stop(base::Bind(ClearReference));
demuxer_stream->Read(base::Bind(ShouldNotGetExecuted));
MessageLoop::current()->RunAllPending();

[acolwell GONE FROM CHROMIUM, 2011-10-14 21:37:35]

http://codereview.chromium.org/8301011/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8301011/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:168: return;
I think it would be better to return an EndOfStream buffer in this case. I don't
remember the exact situation, but I believe we can hang if we just drop the
callbacks.

[ddorwin, 2011-10-14 23:41:54]

Added callback runs and unit test.
With the callback runs, the bug repro crashes. Investigating, but please provide
feedback as is.

http://codereview.chromium.org/8301011/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8301011/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:168: return;
On 2011/10/14 21:37:35, acolwell wrote:
> I think it would be better to return an EndOfStream buffer in this case. I
don't
> remember the exact situation, but I believe we can hang if we just drop the
> callbacks.

Okay. I suppose we should be doing that in ReadTask() below too.

[scherkus (not reviewing), 2011-10-14 23:58:34]

acolwell and I just chatted

we think the root cause is because FFmpegVideoDecoder and FFmpegAudioDecoder
aren't clearing their reference to demuxer_ during Stop() and preventing reads

do you know which stream (audio/video) is getting used during the crash?

[scherkus (not reviewing), 2011-10-15 00:17:22]

overall change is LG w/ nits

http://codereview.chromium.org/8301011/diff/4001/media/filters/ffmpeg_demuxer...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8301011/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:36: return arg->IsEndOfStream();
indent by 2

http://codereview.chromium.org/8301011/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:448: // video_stream now has a demuxer_
pointer to invalid memory.
do you mean |audio| instead of video_stream?

[ddorwin, 2011-10-15 01:19:47]

The bug repro crash statement in my previous update was a misunderstanding -
xhwang had the wrong code. So we are good.

Filed http://crbug.com/100434 to solve the larger root cause of this problem as
Aaron mentioned.

http://codereview.chromium.org/8301011/diff/4001/media/filters/ffmpeg_demuxer...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8301011/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:36: return arg->IsEndOfStream();
On 2011/10/15 00:17:22, scherkus wrote:
> indent by 2

Done.

http://codereview.chromium.org/8301011/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:448: // video_stream now has a demuxer_
pointer to invalid memory.
On 2011/10/15 00:17:22, scherkus wrote:
> do you mean |audio| instead of video_stream?

Done.

[scherkus (not reviewing), 2011-10-15 01:32:14]

LGTM w/ nit

http://codereview.chromium.org/8301011/diff/10001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8301011/diff/10001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:59: if (demuxer_)
could you add {} around here

[ddorwin, 2011-10-15 01:38:15]

Braces added. Bots running with previous patch.

http://codereview.chromium.org/8301011/diff/10001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8301011/diff/10001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:59: if (demuxer_)
On 2011/10/15 01:32:14, scherkus wrote:
> could you add {} around here

Done.

[commit-bot: I haz the power, 2011-10-15 04:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/8301011/11001

[commit-bot: I haz the power, 2011-10-15 04:48:50]

Try job failure for 8301011-11001 on win_rel for steps "update_scripts, update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-10-15 04:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/8301011/11001