Ensures that audio tasks cannot run after AudioManager is deleted.

media/audio/audio_manager.cc

Index: media/audio/audio_manager.cc
diff --git a/media/audio/audio_manager.cc b/media/audio/audio_manager.cc
index 9fb92703ee4dc4f97ee42530fcba54c1dd4bef75..772f80d352ebbb71c19e25f77576f409e4b43266 100644
--- a/media/audio/audio_manager.cc
+++ b/media/audio/audio_manager.cc
@@ -241,51 +241,18 @@ AudioManagerHelper* GetHelper() {
 
 }  // namespace
 
-void AudioManagerDeleter::operator()(const AudioManager* instance) const {
-  CHECK(instance);
-  // We reset g_last_created here instead of in the destructor of AudioManager
-  // because the destructor runs on the audio thread. We want to always change
-  // g_last_created from the main thread.
-  if (g_last_created == instance) {
-    g_last_created = nullptr;
-  } else {
-    // We create multiple instances of AudioManager only when testing.
-    // We should not encounter this case in production.
-    LOG(WARNING) << "Multiple instances of AudioManager detected";
-  }
-
-  // The deleter runs on the main thread, and AudioManager must be destroyed on
-  // the audio thread. If the audio thread is the same as the main one, tasks
-  // after this point are not executed, hence this is the only chance to delete
-  // AudioManager. See http://crbug.com/623703 for more details.
-  if (instance->GetTaskRunner()->BelongsToCurrentThread()) {
-    delete instance;
-    return;
-  }
-
-  // AudioManager must be destroyed on the audio thread. See
-  // http://crbug.com/705455 for an existing AudioManager lifetime issue.
-  if (!instance->GetTaskRunner()->DeleteSoon(FROM_HERE, instance))
-    LOG(WARNING) << "Failed to delete AudioManager instance.";
-}
-
 // Forward declaration of the platform specific AudioManager factory function.
-ScopedAudioManagerPtr CreateAudioManager(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner,
+std::unique_ptr<AudioManager> CreateAudioManager(
+    std::unique_ptr<AudioThread> audio_thread,
     AudioLogFactory* audio_log_factory);
 
 void AudioManager::SetMaxStreamCountForTesting(int max_input, int max_output) {
   NOTREACHED();
 }
 
-AudioManager::AudioManager(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner)
-    : task_runner_(std::move(task_runner)),
-      worker_task_runner_(std::move(worker_task_runner)) {
-  DCHECK(task_runner_);
-  DCHECK(worker_task_runner_);
+AudioManager::AudioManager(std::unique_ptr<AudioThread> audio_thread)
+    : audio_thread_(std::move(audio_thread)) {
+  DCHECK(audio_thread_);
 
   if (g_last_created) {
     // We create multiple instances of AudioManager only when testing.
@@ -298,19 +265,27 @@ AudioManager::AudioManager(
 }
 
 AudioManager::~AudioManager() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Ensure that AudioManager has been shutdown.
+  DCHECK(!audio_thread_);
+
+  if (g_last_created == this) {
+    g_last_created = nullptr;
+  } else {
+    // We create multiple instances of AudioManager only when testing.
+    // We should not encounter this case in production.
+    LOG(WARNING) << "Multiple instances of AudioManager detected";
+  }
 }
 
 // static
-ScopedAudioManagerPtr AudioManager::Create(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner,
+std::unique_ptr<AudioManager> AudioManager::Create(
+    std::unique_ptr<AudioThread> audio_thread,
     scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
     AudioLogFactory* audio_log_factory) {
-  DCHECK(task_runner);
-  DCHECK(worker_task_runner);
-  ScopedAudioManagerPtr manager = CreateAudioManager(
-      std::move(task_runner), std::move(worker_task_runner), audio_log_factory);
+  std::unique_ptr<AudioManager> manager =
+      CreateAudioManager(std::move(audio_thread), audio_log_factory);
 #if BUILDFLAG(ENABLE_WEBRTC)
   manager->InitializeOutputDebugRecording(std::move(file_task_runner));
 #endif
@@ -318,12 +293,14 @@ ScopedAudioManagerPtr AudioManager::Create(
 }
 
 // static
-ScopedAudioManagerPtr AudioManager::CreateForTesting(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+std::unique_ptr<AudioManager> AudioManager::CreateForTesting(
+    std::unique_ptr<AudioThread> audio_thread) {
 #if defined(OS_WIN)
   GetHelper()->InitializeCOMForTesting();
 #endif
-  return Create(task_runner, task_runner, task_runner,
+  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner =
+      audio_thread->GetWorkerTaskRunner();
+  return Create(std::move(audio_thread), std::move(file_task_runner),
                 GetHelper()->fake_log_factory());
 }
 
@@ -357,4 +334,19 @@ AudioManager* AudioManager::Get() {
   return g_last_created;
 }
 
+void AudioManager::Shutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(audio_thread_);
+
+  if (audio_thread_->GetTaskRunner()->BelongsToCurrentThread()) {
+    ShutdownOnAudioThread();
+  } else {
+    audio_thread_->GetTaskRunner()->PostTask(
+        FROM_HERE, base::Bind(&AudioManager::ShutdownOnAudioThread,
+                              base::Unretained(this)));
+  }
+  audio_thread_->Stop();

[o1ka, 2017/04/28 13:24:20]

Have you verified that hang monitor (l.308) will work correctly after this
change?

[alokp, 2017/04/28 17:31:12]

Hang monitor runs on the file thread, which gets stopped before AM is shutdown,
so it should be OK.

[o1ka, 2017/05/02 16:16:00]

I would say hang monitor *currently in content* runs on the file thread:
StartHangMonitorIfNeeded() receives task runner as a parameter; media code has
no guarantee here, has it? Should we take care of |monitor_task_runner_| and
tasks posted to it?

[alokp, 2017/05/02 17:11:28]

You are right - it runs on IO thread - I misread the code. What do you mean by
"take care of tasks posted to it"?

It is currently safe now, since IO thread is stopped before the AM is  shutdown.
But may be we should explicitly suspend it to be more explicit?

[o1ka, 2017/05/02 21:42:23]

"take care of tasks posted to it" - I mean this
https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?dr=CSs&l=78.
We may want to cancel all the tasks holding a pointer to AM.

Yes - I believe we should suspend it.

I strongly believe that media/ should be as much self-sufficient as possible and
do not rely implicitly on lifetime of threads/objects hosted by content/.
Because all the implicit assumptions we make about that lead to complex code
which is hard to read and review. And if something goes wrong we end up with
hard-to-debug crashes.

[alokp, 2017/05/09 19:06:15]

I looked at this more closely. It would be easier to suspend hang monitoring if
we moved the ownership of hang monitor to audio manager. I will do this in a
separate patch though. For now I have added a TODO.

[o1ka, 2017/05/10 15:57:56]

sgtm

+  audio_thread_.reset();

[o1ka, 2017/04/28 13:24:19]

I believe we should only Stop() here and use another way to DCHEK if
|audio_thread_| is stopped.

See my comment to BrowserMainLoop::DestroyAudioManager()

[alokp, 2017/04/28 17:31:12]

Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would still
return null because base::Thread::task_runner() returns null after it has been
stopped.

I am fine with not deleting the audio_thread here, but am not sure if it buys us
anything.

[o1ka, 2017/05/02 16:16:00]

Could you clarify?
It would buy us that AM remains valid until it's deleted, and clients in media/
(like AudioSystemImpl) which are not aware of their lifetime with respect to
AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result for
null.
It simplifies the code quite a lot. Our guarantee becomes "after Shutdown() no
operations are run on AudioManager thread, whatever clients do" - instead of
"..and clients must not post to that thread"

[alokp, 2017/05/02 17:11:28]

base::Thread::task_runner() returns NULL after it has been stopped. So I was
following the same model. Do we know if there are clients that call
AM::GetTaskRunner() after it has been shutdown? May be revisit it if there are
such clients? WDYT?

[o1ka, 2017/05/02 21:42:23]

I'm not sure. To answer this question I need to dig into BrowserMainLoop
shutdown sequence and make sure no tasks using AM can be posted to the main
thread before AM::Shutdown and get executed before AM deletion. And that's
exactly what I want to avoid: I want us to stop worrying about the correct usage
of AM and make AM to take care about it.

While it's nice to follow base::Thread contract, I don't see why should we if it
complicates things: we are not inheriting from it, right? And AM does not have
to follow it as well.

[alokp, 2017/05/09 19:06:15]

Done.

[o1ka, 2017/05/10 15:57:56]

Thanks!

+}
+
 }  // namespace media