Call close_timer_.Reset() from StopStreamTask.

Call close_timer_.Reset() from StopStreamTask.

There is a race here if ClosePendingStreams() is called before StopStreamTask.
The stream added to idle_streams_ will stay there forever unless we reset the
timer.

This test fails when run locally under tsan and on the drmemory bots here:
http://build.chromium.org/p/chromium.fyi/builders/Windows%20Tests%20%28DrMemory%20Win%207%29/builds/1402/steps/memory%20test%3A%20media

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

[Reid Kleckner (google), 2011-11-23 15:11:59]

PTAL

[Reid Kleckner (google), 2011-11-23 16:00:27]

On 2011/11/23 15:11:59, Reid Kleckner (google) wrote:
> PTAL

Hang on, other media_unittests are failing.  I can't tell if it's my fault yet.

[Reid Kleckner (google), 2011-11-23 16:45:50]

On 2011/11/23 16:00:27, Reid Kleckner (google) wrote:
> On 2011/11/23 15:11:59, Reid Kleckner (google) wrote:
> > PTAL
> 
> Hang on, other media_unittests are failing.  I can't tell if it's my fault
yet.

All of the failures I see on my machine occur without my change, so it should be
OK.  =/

[Sergey Ulanov, 2011-11-23 18:07:57]

LGTM if my comment is addressed.

http://codereview.chromium.org/8680009/diff/1/media/audio/audio_output_dispat...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/8680009/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:72: close_timer_.Reset();
I think you should also remove this line, as it is pointless now.

[Reid Kleckner (google), 2011-11-23 18:40:44]

I realized that StopStreamTask is posted with a delay of
pause_delay_milliseconds_, so I had to add a second sleep and RunAllPending
call.  Probably worth another look.

http://codereview.chromium.org/8680009/diff/1/media/audio/audio_output_dispat...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/8680009/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:72: close_timer_.Reset();
On 2011/11/23 18:07:57, sergeyu wrote:
> I think you should also remove this line, as it is pointless now.

Done.

[Sergey Ulanov, 2011-11-23 19:12:17]

still LGTM, but please see my nits.

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:195: // Wait for StreamStopped to
post StopStreamTask.
nit: StreamStopped() and StopStreamTask() to make it clear that they are
functions.

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:196:
base::PlatformThread::Sleep(pause_delay_milliseconds_ * 3);
I think sleeping pause_delay_milliseconds_ should be enough here. But you can
set it to "pause_delay_milliseconds_ * 2" just to be sure.

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:200:
base::PlatformThread::Sleep(kTestCloseDelayMs * 3);
"*2" and can you please update two other tests that use Sleep()?

[Reid Kleckner (google), 2011-11-23 19:23:04]

Sending to CQ, thanks for the review!

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:195: // Wait for StreamStopped to
post StopStreamTask.
On 2011/11/23 19:12:18, sergeyu wrote:
> nit: StreamStopped() and StopStreamTask() to make it clear that they are
> functions.

Done.

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:196:
base::PlatformThread::Sleep(pause_delay_milliseconds_ * 3);
On 2011/11/23 19:12:18, sergeyu wrote:
> I think sleeping pause_delay_milliseconds_ should be enough here. But you can
> set it to "pause_delay_milliseconds_ * 2" just to be sure.

Done.

http://codereview.chromium.org/8680009/diff/4002/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:200:
base::PlatformThread::Sleep(kTestCloseDelayMs * 3);
On 2011/11/23 19:12:18, sergeyu wrote:
> "*2" and can you please update two other tests that use Sleep()?

Done.

[commit-bot: I haz the power, 2011-11-23 19:23:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rnk@google.com/8680009/7001

[scherkus (not reviewing), 2011-11-23 19:50:33]

+enal, henrika, crogers for CC

[commit-bot: I haz the power, 2011-11-23 21:26:18]

Try job failure for 8680009-7001 (retry) on mac_rel for step "media_unittests".
It's a second try, previously, step "media_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-24 17:06:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rnk@google.com/8680009/7001

[commit-bot: I haz the power, 2011-11-24 18:43:54]

Change committed as 111545