Fix crash in VideoRendererBase::ThreadMain().

Fix crash in VideoRendererBase::ThreadMain().

The interleaving of VideoRendererBase::Stop() with outstanding asynchronous calls to a VideoDecoder would result in continuing to execute code (e.g., starting up a thread) on a stopped VideoRendererBase. The most common manifestation of the bug was crashing in ThreadMain() as VideoRendererBase was being destroyed.

The CHECK()s added to ~VideoRendererBase() in r180573 will stay there as they've proven useful.

BUG=174150
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184048

[scherkus (not reviewing), 2013-02-20 08:26:11]

this is an official PlanePatch™

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base.cc:95: if (state_ == kStopped) {
note: state == kUninitialized actually runs the Stop() code path now (hence the
check for decoder_ below)

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:377: ADD_FAILURE() << "Expected
callback not to be called\n" << stack.ToString();
I did this to get the stack trace of the callback w/o crashing the test suite

Maybe it's not worth it as these tests will [D]CHECK if they fail so hmmm...

[acolwell GONE FROM CHROMIUM, 2013-02-20 17:01:14]

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base.cc:48: CHECK_EQ(state_, kStopped);
Won't this crash if an error is encountered before the renderer is initialized.
For example, wouldn't a demuxer init error cause this to be destroyed in the
kUninitialized state w/o Stop() being called? Audio-only playback might be
another case.

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base.cc:95: if (state_ == kStopped) {
On 2013/02/20 08:26:11, scherkus wrote:
> note: state == kUninitialized actually runs the Stop() code path now (hence
the
> check for decoder_ below)
This seems a little weird. Shouldn't we transition to a kInitializing state
inside Initialize() somewhere? That seems to make a little more sense then
letting kUninitialized fall through here. It avoids the "If we are
uninitialized, why do we have to clean up state." train of thought when looking
at this code.

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:379: 
You should add a test that allows the renderer to be destroyed before an
Initialize() call. I believe this will simulate the audio-only & demuxer init
failure scenarios.

[scherkus (not reviewing), 2013-02-21 20:43:54]

PTAL

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base.cc:48: CHECK_EQ(state_, kStopped);
On 2013/02/20 17:01:14, acolwell wrote:
> Won't this crash if an error is encountered before the renderer is
initialized.
> For example, wouldn't a demuxer init error cause this to be destroyed in the
> kUninitialized state w/o Stop() being called? Audio-only playback might be
> another case.

Indeed it will!

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base.cc:95: if (state_ == kStopped) {
On 2013/02/20 17:01:14, acolwell wrote:
> On 2013/02/20 08:26:11, scherkus wrote:
> > note: state == kUninitialized actually runs the Stop() code path now (hence
> the
> > check for decoder_ below)
> This seems a little weird. Shouldn't we transition to a kInitializing state
> inside Initialize() somewhere? That seems to make a little more sense then
> letting kUninitialized fall through here. It avoids the "If we are
> uninitialized, why do we have to clean up state." train of thought when
looking
> at this code.

Yeah that was on my mind. I fully admit that I was a bit rushed to get something
together before the flight attendant chastised me.

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12324005/diff/2001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:379: 
On 2013/02/20 17:01:14, acolwell wrote:
> You should add a test that allows the renderer to be destroyed before an
> Initialize() call. I believe this will simulate the audio-only & demuxer init
> failure scenarios.

Done.

[acolwell GONE FROM CHROMIUM, 2013-02-21 21:07:16]

LGTM % nits

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base.cc:95: if (state_ == kStopped) {
Do we still want to fall through if called in kUninitialized? DCHECK_NE(state_,
kUninitialized) ?

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:369: TEST_F(VideoRendererBaseTest,
DoNothing) {}
nit: Add comment indicating why we have a DoNothing test since we are actually
testing something.

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:371: TEST_F(VideoRendererBaseTest,
Stop) {
nit: s/Stop/StopBeforeInitialize ?

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:386: TEST_F(VideoRendererBaseTest,
InitializeAndStop) {
nit: s/InitializeAndStop/StopWhileInitializing ?

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:395: TEST_F(VideoRendererBaseTest,
FlushAndStop) {
nit: s/FlushAndStop/StopWhileFlushing ?

[scherkus (not reviewing), 2013-02-22 01:45:49]

thanks for the excellent naming suggestions!

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base.cc:95: if (state_ == kStopped) {
On 2013/02/21 21:07:16, acolwell wrote:
> Do we still want to fall through if called in kUninitialized?
DCHECK_NE(state_,
> kUninitialized) ?

GAH I forgot to fix this

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:369: TEST_F(VideoRendererBaseTest,
DoNothing) {}
On 2013/02/21 21:07:16, acolwell wrote:
> nit: Add comment indicating why we have a DoNothing test since we are actually
> testing something.

Done.

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:371: TEST_F(VideoRendererBaseTest,
Stop) {
On 2013/02/21 21:07:16, acolwell wrote:
> nit: s/Stop/StopBeforeInitialize ?

Settled on StopWithoutInitialize ("before" makes me think we'll eventually call
it)

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:386: TEST_F(VideoRendererBaseTest,
InitializeAndStop) {
On 2013/02/21 21:07:16, acolwell wrote:
> nit: s/InitializeAndStop/StopWhileInitializing ?

Done.

https://codereview.chromium.org/12324005/diff/6001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:395: TEST_F(VideoRendererBaseTest,
FlushAndStop) {
On 2013/02/21 21:07:16, acolwell wrote:
> nit: s/FlushAndStop/StopWhileFlushing ?

Done.

[commit-bot: I haz the power, 2013-02-22 01:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/12324005/9005

[commit-bot: I haz the power, 2013-02-22 06:14:11]

Change committed as 184048