Commit 8b4f7fa7 authored by miu's avatar miu Committed by Commit bot

AudioMirroringManager becomes a global LazyInstance.

While attempting to resolve a flaky browser_test, it became clear that
AudioMirroringManager was not outliving its use.  In the original change
that introduced this class, it was instantiated and owned by
BrowserMainLoop as a matter of convenience.  However, with some
debugging, it's clear that it must outlive objects that can outlive
BrowserMainLoop (e.g., WebContentsAudioInputStream).

Side notes: I've checked the feasibility of other solutions, confirming
that the shutdown of AudioManager does NOT guarantee complete teardown
of an AudioInputStream, so it's not sufficient to simply change the
destruction order of the objects in BrowserMainLoop to resolve this
problem.  As AudioMirroringManager provides a browser-wide service and
owns no objects, it seems reasonable for it to exist as a global
LazyInstance.

BUG=396413

Review URL: https://codereview.chromium.org/499483003

Cr-Commit-Position: refs/heads/master@{#292258}
parent 22b41158
...@@ -35,7 +35,6 @@ ...@@ -35,7 +35,6 @@
#include "content/browser/gpu/gpu_process_host_ui_shim.h" #include "content/browser/gpu/gpu_process_host_ui_shim.h"
#include "content/browser/histogram_synchronizer.h" #include "content/browser/histogram_synchronizer.h"
#include "content/browser/loader/resource_dispatcher_host_impl.h" #include "content/browser/loader/resource_dispatcher_host_impl.h"
#include "content/browser/media/capture/audio_mirroring_manager.h"
#include "content/browser/media/media_internals.h" #include "content/browser/media/media_internals.h"
#include "content/browser/net/browser_online_state_observer.h" #include "content/browser/net/browser_online_state_observer.h"
#include "content/browser/plugin_service_impl.h" #include "content/browser/plugin_service_impl.h"
...@@ -506,10 +505,6 @@ void BrowserMainLoop::MainMessageLoopStart() { ...@@ -506,10 +505,6 @@ void BrowserMainLoop::MainMessageLoopStart() {
ContentWebUIControllerFactory::GetInstance()); ContentWebUIControllerFactory::GetInstance());
} }
{
TRACE_EVENT0("startup", "BrowserMainLoop::Subsystem:AudioMirroringManager");
audio_mirroring_manager_.reset(new AudioMirroringManager());
}
{ {
TRACE_EVENT0("startup", "BrowserMainLoop::Subsystem:OnlineStateObserver"); TRACE_EVENT0("startup", "BrowserMainLoop::Subsystem:OnlineStateObserver");
online_state_observer_.reset(new BrowserOnlineStateObserver); online_state_observer_.reset(new BrowserOnlineStateObserver);
......
...@@ -37,7 +37,6 @@ class NetworkChangeNotifier; ...@@ -37,7 +37,6 @@ class NetworkChangeNotifier;
} // namespace net } // namespace net
namespace content { namespace content {
class AudioMirroringManager;
class BrowserMainParts; class BrowserMainParts;
class BrowserOnlineStateObserver; class BrowserOnlineStateObserver;
class BrowserShutdownImpl; class BrowserShutdownImpl;
...@@ -92,9 +91,6 @@ class CONTENT_EXPORT BrowserMainLoop { ...@@ -92,9 +91,6 @@ class CONTENT_EXPORT BrowserMainLoop {
int GetResultCode() const { return result_code_; } int GetResultCode() const { return result_code_; }
media::AudioManager* audio_manager() const { return audio_manager_.get(); } media::AudioManager* audio_manager() const { return audio_manager_.get(); }
AudioMirroringManager* audio_mirroring_manager() const {
return audio_mirroring_manager_.get();
}
MediaStreamManager* media_stream_manager() const { MediaStreamManager* media_stream_manager() const {
return media_stream_manager_.get(); return media_stream_manager_.get();
} }
...@@ -160,7 +156,6 @@ class CONTENT_EXPORT BrowserMainLoop { ...@@ -160,7 +156,6 @@ class CONTENT_EXPORT BrowserMainLoop {
scoped_ptr<media::UserInputMonitor> user_input_monitor_; scoped_ptr<media::UserInputMonitor> user_input_monitor_;
scoped_ptr<media::AudioManager> audio_manager_; scoped_ptr<media::AudioManager> audio_manager_;
scoped_ptr<media::MidiManager> midi_manager_; scoped_ptr<media::MidiManager> midi_manager_;
scoped_ptr<AudioMirroringManager> audio_mirroring_manager_;
scoped_ptr<MediaStreamManager> media_stream_manager_; scoped_ptr<MediaStreamManager> media_stream_manager_;
// Per-process listener for online state changes. // Per-process listener for online state changes.
scoped_ptr<BrowserOnlineStateObserver> online_state_observer_; scoped_ptr<BrowserOnlineStateObserver> online_state_observer_;
......
...@@ -4,50 +4,33 @@ ...@@ -4,50 +4,33 @@
#include "content/browser/media/capture/audio_mirroring_manager.h" #include "content/browser/media/capture/audio_mirroring_manager.h"
#include "content/public/browser/browser_thread.h" #include "base/lazy_instance.h"
namespace content { namespace content {
namespace { namespace {
// Debug utility to make sure methods of AudioMirroringManager are not invoked base::LazyInstance<AudioMirroringManager>::Leaky g_audio_mirroring_manager =
// more than once in a single call stack. In release builds, this compiles to LAZY_INSTANCE_INITIALIZER;
// nothing and gets completely optimized out.
class ReentrancyGuard {
public:
#ifdef NDEBUG
ReentrancyGuard() {}
~ReentrancyGuard() {}
#else
ReentrancyGuard() {
DCHECK(!inside_a_method_);
inside_a_method_ = true;
}
~ReentrancyGuard() {
inside_a_method_ = false;
}
static bool inside_a_method_; // Safe to be static, since AMM is a singleton.
#endif
};
#ifndef NDEBUG
bool ReentrancyGuard::inside_a_method_ = false;
#endif
} // namespace } // namespace
AudioMirroringManager::AudioMirroringManager() {} // static
AudioMirroringManager* AudioMirroringManager::GetInstance() {
return g_audio_mirroring_manager.Pointer();
}
AudioMirroringManager::~AudioMirroringManager() { AudioMirroringManager::AudioMirroringManager() {
DCHECK(diverters_.empty()); // Only *after* construction, check that AudioMirroringManager is being
DCHECK(sessions_.empty()); // invoked on the same single thread.
thread_checker_.DetachFromThread();
} }
AudioMirroringManager::~AudioMirroringManager() {}
void AudioMirroringManager::AddDiverter( void AudioMirroringManager::AddDiverter(
int render_process_id, int render_view_id, Diverter* diverter) { int render_process_id, int render_view_id, Diverter* diverter) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK(thread_checker_.CalledOnValidThread());
ReentrancyGuard guard;
DCHECK(diverter); DCHECK(diverter);
// DCHECK(diverter not already in diverters_ under any key) // DCHECK(diverter not already in diverters_ under any key)
...@@ -73,8 +56,7 @@ void AudioMirroringManager::AddDiverter( ...@@ -73,8 +56,7 @@ void AudioMirroringManager::AddDiverter(
void AudioMirroringManager::RemoveDiverter( void AudioMirroringManager::RemoveDiverter(
int render_process_id, int render_view_id, Diverter* diverter) { int render_process_id, int render_view_id, Diverter* diverter) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK(thread_checker_.CalledOnValidThread());
ReentrancyGuard guard;
// Stop diverting the audio stream if a mirroring session is active. // Stop diverting the audio stream if a mirroring session is active.
const Target target(render_process_id, render_view_id); const Target target(render_process_id, render_view_id);
...@@ -95,8 +77,7 @@ void AudioMirroringManager::RemoveDiverter( ...@@ -95,8 +77,7 @@ void AudioMirroringManager::RemoveDiverter(
void AudioMirroringManager::StartMirroring( void AudioMirroringManager::StartMirroring(
int render_process_id, int render_view_id, int render_process_id, int render_view_id,
MirroringDestination* destination) { MirroringDestination* destination) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK(thread_checker_.CalledOnValidThread());
ReentrancyGuard guard;
DCHECK(destination); DCHECK(destination);
// Insert an entry into the set of active mirroring sessions. If a mirroring // Insert an entry into the set of active mirroring sessions. If a mirroring
...@@ -137,8 +118,7 @@ void AudioMirroringManager::StartMirroring( ...@@ -137,8 +118,7 @@ void AudioMirroringManager::StartMirroring(
void AudioMirroringManager::StopMirroring( void AudioMirroringManager::StopMirroring(
int render_process_id, int render_view_id, int render_process_id, int render_view_id,
MirroringDestination* destination) { MirroringDestination* destination) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK(thread_checker_.CalledOnValidThread());
ReentrancyGuard guard;
// Stop mirroring if there is an active session *and* the destination // Stop mirroring if there is an active session *and* the destination
// matches. // matches.
......
...@@ -5,7 +5,7 @@ ...@@ -5,7 +5,7 @@
// AudioMirroringManager is a singleton object that maintains a set of active // AudioMirroringManager is a singleton object that maintains a set of active
// audio mirroring destinations and auto-connects/disconnects audio streams // audio mirroring destinations and auto-connects/disconnects audio streams
// to/from those destinations. It is meant to be used exclusively on the IO // to/from those destinations. It is meant to be used exclusively on the IO
// BrowserThread. // thread.
// //
// How it works: // How it works:
// //
...@@ -31,6 +31,7 @@ ...@@ -31,6 +31,7 @@
#include <utility> #include <utility>
#include "base/basictypes.h" #include "base/basictypes.h"
#include "base/threading/thread_checker.h"
#include "content/common/content_export.h" #include "content/common/content_export.h"
#include "media/audio/audio_source_diverter.h" #include "media/audio/audio_source_diverter.h"
...@@ -60,10 +61,13 @@ class CONTENT_EXPORT AudioMirroringManager { ...@@ -60,10 +61,13 @@ class CONTENT_EXPORT AudioMirroringManager {
virtual ~MirroringDestination() {} virtual ~MirroringDestination() {}
}; };
// Note: Use GetInstance() for non-test code.
AudioMirroringManager(); AudioMirroringManager();
virtual ~AudioMirroringManager(); virtual ~AudioMirroringManager();
// Returns the global instance.
static AudioMirroringManager* GetInstance();
// Add/Remove a diverter for an audio stream with a known RenderView target // Add/Remove a diverter for an audio stream with a known RenderView target
// (represented by |render_process_id| + |render_view_id|). Multiple // (represented by |render_process_id| + |render_view_id|). Multiple
// diverters may be added for the same target. |diverter| must live until // diverters may be added for the same target. |diverter| must live until
...@@ -100,6 +104,9 @@ class CONTENT_EXPORT AudioMirroringManager { ...@@ -100,6 +104,9 @@ class CONTENT_EXPORT AudioMirroringManager {
// Currently-active mirroring sessions. // Currently-active mirroring sessions.
SessionMap sessions_; SessionMap sessions_;
// Used to check that all AudioMirroringManager code runs on the same thread.
base::ThreadChecker thread_checker_;
DISALLOW_COPY_AND_ASSIGN(AudioMirroringManager); DISALLOW_COPY_AND_ASSIGN(AudioMirroringManager);
}; };
......
...@@ -755,14 +755,14 @@ void RenderProcessHostImpl::CreateMessageFilters() { ...@@ -755,14 +755,14 @@ void RenderProcessHostImpl::CreateMessageFilters() {
AddFilter(new AudioInputRendererHost( AddFilter(new AudioInputRendererHost(
audio_manager, audio_manager,
media_stream_manager, media_stream_manager,
BrowserMainLoop::GetInstance()->audio_mirroring_manager(), AudioMirroringManager::GetInstance(),
BrowserMainLoop::GetInstance()->user_input_monitor())); BrowserMainLoop::GetInstance()->user_input_monitor()));
// The AudioRendererHost needs to be available for lookup, so it's // The AudioRendererHost needs to be available for lookup, so it's
// stashed in a member variable. // stashed in a member variable.
audio_renderer_host_ = new AudioRendererHost( audio_renderer_host_ = new AudioRendererHost(
GetID(), GetID(),
audio_manager, audio_manager,
BrowserMainLoop::GetInstance()->audio_mirroring_manager(), AudioMirroringManager::GetInstance(),
media_internals, media_internals,
media_stream_manager); media_stream_manager);
AddFilter(audio_renderer_host_.get()); AddFilter(audio_renderer_host_.get());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment