Delete MediaStreamManager in the same way as AudioManager

MediaStreamManager needs to outlive its clients like MediaStreamDispatcherHost, which is deleted when the IPC channels goes away. So MediaStreamManager needs to outlive IO thread. But its components like device thread, AudioInputDeviceManager and VideoCaptureManager should be deleted before IO thread gets deleted. We achieve this by having a destructor observer on the IO thread, and delete the components before IO thread goes away.



BUG=137476
TEST=content_unittests

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

[no longer working on chromium, 2012-07-17 15:19:37]

Tommi, could you please take a lead on reviewing this patch?

Thanks,

BR,
/SX

[tommi (sloooow) - chröme, 2012-07-17 15:36:58]

looks good in general. Just a few questions.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:205:
message_loop_.reset();
Did you have to add this explicitly?  This doesn't delete the
media_stream_manager_, right (which managers does it delete?)?

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:633:
device_thread_->Stop();
doesn't reset() imply Stop()?

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:91: // creates a new
request which is identified by a unique |label|.
Thanks for fixing. Previously the documentation was incorrect.
Maybe add:
// creates a new request which is identified by a unique |label|
// that's returned to the caller.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:149: // This object
gets deleted on the UI thread, and IO thread is destroyed it
"gets deleted on the UI thread, and IO thread is destroyed it goes away"
->
"gets deleted on the UI thread after the IO thread has been destroyed."

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:150: // goes away. So
we need to know when IO thread is being destroyed and we can
"So we need to know when the IO thread is destroyed so that we can ..."

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/video_capture_host_unittest.cc:252: //
Delete the IO message loop to delete the device thread and managers.
which managers?

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/test/webrtc...
File content/test/webrtc_audio_device_test.cc (left):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/test/webrtc...
content/test/webrtc_audio_device_test.cc:227: media_stream_manager_.reset();
hmm... should we reset this object and the audio_manager_ up where we do
|mock_process_.reset();|?  Maybe we should also create both the audio manager
and media stream managers inside SetUp() before we initialize the IO thread
(after we call |ui_thread_.reset|)?

[jam, 2012-07-17 15:54:34]

content/browser/browser_main_loop.cc lgtm

[no longer working on chromium, 2012-07-17 17:18:25]

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:205:
message_loop_.reset();
On 2012/07/17 15:36:58, tommi wrote:
> Did you have to add this explicitly?  This doesn't delete the
> media_stream_manager_, right (which managers does it delete?)?

This deletes the device thread, AudioInputDeviceManager and VideoCaptureManager
(they are deleted when the IO loop goes away). So we need to do it explicit,
otherwise it will hit the DCHECKs in the destructor of MediaStreamManager.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:633:
device_thread_->Stop();
On 2012/07/17 15:36:58, tommi wrote:
> doesn't reset() imply Stop()?

True, reset() should be enough.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:91: // creates a new
request which is identified by a unique |label|.
On 2012/07/17 15:36:58, tommi wrote:
> Thanks for fixing. Previously the documentation was incorrect.
> Maybe add:
> // creates a new request which is identified by a unique |label|
> // that's returned to the caller.

Done.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:149: // This object
gets deleted on the UI thread, and IO thread is destroyed it
On 2012/07/17 15:36:58, tommi wrote:
> "gets deleted on the UI thread, and IO thread is destroyed it goes away"
> ->
> "gets deleted on the UI thread after the IO thread has been destroyed."


Thanks. I missed a "before" in my comment.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:150: // goes away. So
we need to know when IO thread is being destroyed and we can
On 2012/07/17 15:36:58, tommi wrote:
> "So we need to know when the IO thread is destroyed so that we can ..."

Done.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/browser/ren...
content/browser/renderer_host/media/video_capture_host_unittest.cc:252: //
Delete the IO message loop to delete the device thread and managers.
On 2012/07/17 15:36:58, tommi wrote:
> which managers?

Done.

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/test/webrtc...
File content/test/webrtc_audio_device_test.cc (left):

https://chromiumcodereview.appspot.com/10786029/diff/2001/content/test/webrtc...
content/test/webrtc_audio_device_test.cc:227: media_stream_manager_.reset();
On 2012/07/17 15:36:58, tommi wrote:
> hmm... should we reset this object and the audio_manager_ up where we do
> |mock_process_.reset();|?  Maybe we should also create both the audio manager
> and media stream managers inside SetUp() before we initialize the IO thread
> (after we call |ui_thread_.reset|)?

Good idea, it fits the current design more.
Another alternative might be that we do not reset the media_stream_manager_ and
audio_manager_ object explicitly.

[tommi (sloooow) - chröme, 2012-07-17 19:46:51]

lgtm

[no longer working on chromium, 2012-07-18 08:04:00]

Tommi, could you please take one more look at the patch set 4?
MediaStreamDeviceSettings needs to be deleted before IO thread goes away.

Thanks,

BR,
/SX

[tommi (sloooow) - chröme, 2012-07-18 08:15:22]

https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
content/browser/renderer_host/media/media_stream_device_settings.cc:46: if
(!settings_)
it's better to do this check only on the IO thread as it was before.  This
change makes it so that we're checking the variable on more than one thread.

[no longer working on chromium, 2012-07-18 09:39:28]

https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
content/browser/renderer_host/media/media_stream_device_settings.cc:46: if
(!settings_)
On 2012/07/18 08:15:23, tommi wrote:
> it's better to do this check only on the IO thread as it was before.  This
> change makes it so that we're checking the variable on more than one thread.

But can BrowserThread::PostTask(BrowserThread::IO, .. crash when the IO thread
has gone away?

[tommi (sloooow) - chröme, 2012-07-18 10:15:35]

On 2012/07/18 09:39:28, xians1 wrote:
>
https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
> File content/browser/renderer_host/media/media_stream_device_settings.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
> content/browser/renderer_host/media/media_stream_device_settings.cc:46: if
> (!settings_)
> On 2012/07/18 08:15:23, tommi wrote:
> > it's better to do this check only on the IO thread as it was before.  This
> > change makes it so that we're checking the variable on more than one thread.
> 
> But can BrowserThread::PostTask(BrowserThread::IO, .. crash when the IO thread
> has gone away?

I don't think so but it should be easy to test.

[no longer working on chromium, 2012-07-18 11:55:11]

On 2012/07/18 10:15:35, tommi wrote:
> On 2012/07/18 09:39:28, xians1 wrote:
> >
>
https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
> > File content/browser/renderer_host/media/media_stream_device_settings.cc
> > (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10786029/diff/13001/content/browser/re...
> > content/browser/renderer_host/media/media_stream_device_settings.cc:46: if
> > (!settings_)
> > On 2012/07/18 08:15:23, tommi wrote:
> > > it's better to do this check only on the IO thread as it was before.  This
> > > change makes it so that we're checking the variable on more than one
thread.
> > 
> > But can BrowserThread::PostTask(BrowserThread::IO, .. crash when the IO
thread
> > has gone away?
> 
> I don't think so but it should be easy to test.

Look into the code, BrowserThread::PostTask will just return false when it could
not find the message loop. 

So I revert the relevant change.

Going to land this CL soon.

Thanks,
BR,
SX

[commit-bot: I haz the power, 2012-07-18 11:58:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10786029/16002

[commit-bot: I haz the power, 2012-07-18 13:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10786029/23002

[commit-bot: I haz the power, 2012-07-18 13:38:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10786029/25009

[commit-bot: I haz the power, 2012-07-18 15:06:04]

Change committed as 147240