Ensures that audio tasks cannot run after AudioManager is deleted.

media/audio/audio_system_impl_unittest.cc

Index: media/audio/audio_system_impl_unittest.cc
diff --git a/media/audio/audio_system_impl_unittest.cc b/media/audio/audio_system_impl_unittest.cc
index 618bf9298e7b37829324f4196926fd63ff181c0b..daf4c560e51ca93c867c6338e944a46962370a7c 100644
--- a/media/audio/audio_system_impl_unittest.cc
+++ b/media/audio/audio_system_impl_unittest.cc
@@ -11,7 +11,9 @@
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "media/audio/audio_device_description.h"
+#include "media/audio/audio_thread_impl.h"
 #include "media/audio/mock_audio_manager.h"
+#include "media/audio/test_audio_thread.h"
 #include "media/base/test_helpers.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -32,7 +34,6 @@ class AudioSystemImplTest : public testing::TestWithParam<bool> {
  public:
   AudioSystemImplTest()
       : use_audio_thread_(GetParam()),
-        audio_thread_("AudioSystemThread"),
         input_params_(AudioParameters::AUDIO_PCM_LINEAR,
                       CHANNEL_LAYOUT_MONO,
                       AudioParameters::kTelephoneSampleRate,
@@ -49,14 +50,12 @@ class AudioSystemImplTest : public testing::TestWithParam<bool> {
                                16,
                                AudioParameters::kTelephoneSampleRate / 30) {
     if (use_audio_thread_) {
-      audio_thread_.StartAndWaitForTesting();
-      audio_manager_.reset(
-          new media::MockAudioManager(audio_thread_.task_runner()));
+      audio_manager_ = base::MakeUnique<MockAudioManager>(
+          base::MakeUnique<AudioThreadImpl>());
     } else {
-      audio_manager_.reset(new media::MockAudioManager(
-          base::ThreadTaskRunnerHandle::Get().get()));
+      audio_manager_ = base::MakeUnique<MockAudioManager>(
+          base::MakeUnique<TestAudioThread>());

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

Can we have a bool parameter for AudioThreadImpl constructor instead?

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

I think this is cleaner. Did you have any specific concerns about
TestAudioThread being a separate class?

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

The problem with this particular test is that |use_audio_thread_| on Mac will
result in the same logic for true and false. But the intention is to test calls
when they are issued from another thread (it happens when |use_audio_thread_| is
true) / from the same (audio) thread (when it's false).

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

You are right, but what is the use for testing a codepath that is not exercised
in production? Also, there is a reason we use the main thread on mac. It is
because Core Audio needs to be used on the main thread. Isn't there a risk of
making the tests flaky if Core Audio runs on the audio thread?

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

AudioSystem can be accessed from any thread. In practice, it's usually accessed
from IO, sometimes from UI and potentially from AM thread - and on Mac AM thread
*is* UI thread. So on each platform we need to test AudioSystem access on AM
thread and on a non-AM thread.

It's a good point about CoreAudio not running on main thread on Mac here. I have
not seen any flakes so far though. We may want to change the tests to run AM on
the main thread and the rest of the logic on another thread. It will complicate
things a bit, so I would prefer to do it when we really see problems.

dalecurtis@ - WDYT?

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

I added a bool to TestAudioThread constructor and used it here.

     }
-
     audio_manager_->SetInputStreamParameters(input_params_);
     audio_manager_->SetOutputStreamParameters(output_params_);
     audio_manager_->SetDefaultOutputStreamParameters(default_output_params_);
@@ -77,13 +76,7 @@ class AudioSystemImplTest : public testing::TestWithParam<bool> {
     EXPECT_EQ(AudioSystem::Get(), audio_system_.get());
   }
 
-  ~AudioSystemImplTest() override {
-    // Deleting |audio_manager_| on its thread.
-    audio_system_.reset();
-    EXPECT_EQ(AudioSystem::Get(), nullptr);
-    audio_manager_.reset();
-    audio_thread_.Stop();
-  }
+  ~AudioSystemImplTest() override { audio_manager_->Shutdown(); }

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

Note that in this test the destruction sequence is different from the one in the
mail loop: Shutdown, AudioSystem deletion, AudioManager deletion.

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

This one is correct :) We need to fix BML as noted in the comment over there.

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

Right, that's what I thought :)

 
   void OnAudioParams(const AudioParameters& expected,
                      const AudioParameters& received) {
@@ -132,7 +125,7 @@ class AudioSystemImplTest : public testing::TestWithParam<bool> {
       return;
     }
     WaitableMessageLoopEvent event;
-    audio_thread_.task_runner()->PostTaskAndReply(
+    audio_manager_->GetTaskRunner()->PostTaskAndReply(
         FROM_HERE, base::Bind(&base::DoNothing), event.GetClosure());
     // Runs the loop and waits for the |audio_thread_| to call event's closure,
     // which means AudioSystem reply containing device parameters is already
@@ -153,8 +146,7 @@ class AudioSystemImplTest : public testing::TestWithParam<bool> {
   base::MessageLoop message_loop_;
   base::ThreadChecker thread_checker_;
   bool use_audio_thread_;
-  base::Thread audio_thread_;
-  MockAudioManager::UniquePtr audio_manager_;
+  std::unique_ptr<media::MockAudioManager> audio_manager_;
   std::unique_ptr<media::AudioSystem> audio_system_;
   AudioParameters input_params_;
   AudioParameters output_params_;