Ensures that audio tasks cannot run after AudioManager is deleted.

media/audio/audio_system_impl_unittest.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_system_impl.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #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"
 
 namespace {
 const char* kNonDefaultDeviceId = "non-default-device-id";
 }
 
 namespace media {
 
 bool operator==(const media::AudioDeviceDescription& lhs,
                 const media::AudioDeviceDescription& rhs) {
   return lhs.device_name == rhs.device_name && lhs.unique_id == rhs.unique_id &&
          lhs.group_id == rhs.group_id;
 }
 
 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,
                       16,
                       AudioParameters::kTelephoneSampleRate / 10),
         output_params_(AudioParameters::AUDIO_PCM_LINEAR,
                        CHANNEL_LAYOUT_MONO,
                        AudioParameters::kTelephoneSampleRate,
                        16,
                        AudioParameters::kTelephoneSampleRate / 20),
         default_output_params_(AudioParameters::AUDIO_PCM_LINEAR,
                                CHANNEL_LAYOUT_MONO,
                                AudioParameters::kTelephoneSampleRate,
                                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_);
 
     auto get_device_descriptions = [](const AudioDeviceDescriptions* source,
                                       AudioDeviceDescriptions* destination) {
       destination->insert(destination->end(), source->begin(), source->end());
     };
 
     audio_manager_->SetInputDeviceDescriptionsCallback(
         base::Bind(get_device_descriptions,
                    base::Unretained(&input_device_descriptions_)));
     audio_manager_->SetOutputDeviceDescriptionsCallback(
         base::Bind(get_device_descriptions,
                    base::Unretained(&output_device_descriptions_)));
 
     audio_system_ = media::AudioSystemImpl::Create(audio_manager_.get());
     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) {
     EXPECT_TRUE(thread_checker_.CalledOnValidThread());
     EXPECT_EQ(expected.AsHumanReadableString(),
               received.AsHumanReadableString());
     AudioParametersReceived();
   }
 
   void OnHasInputDevices(bool result) {
@@ skipping 28 matching lines @@
     EXPECT_EQ(expected_associated_device_id, associated_device_id);
     InputDeviceInfoReceived();
   }
 
   void WaitForCallback() {
     if (!use_audio_thread_) {
       base::RunLoop().RunUntilIdle();
       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
     // queued on the main thread.
     event.RunAndWait();
     base::RunLoop().RunUntilIdle();
   }
 
   // Mocks to verify that AudioSystem replied with an expected callback.
   MOCK_METHOD0(AudioParametersReceived, void(void));
   MOCK_METHOD1(HasInputDevicesCallback, void(bool));
   MOCK_METHOD1(HasOutputDevicesCallback, void(bool));
   MOCK_METHOD0(DeviceDescriptionsReceived, void(void));
   MOCK_METHOD1(AssociatedOutputDeviceIDReceived, void(const std::string&));
   MOCK_METHOD0(InputDeviceInfoReceived, void(void));
 
  protected:
   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_;
   AudioParameters default_output_params_;
   AudioDeviceDescriptions input_device_descriptions_;
   AudioDeviceDescriptions output_device_descriptions_;
 };
 
 TEST_P(AudioSystemImplTest, GetInputStreamParameters) {
   EXPECT_CALL(*this, AudioParametersReceived());
@@ skipping 176 matching lines @@
   audio_system_->GetInputDeviceInfo(
       kNonDefaultDeviceId, base::Bind(&AudioSystemImplTest::OnInputDeviceInfo,
                                       base::Unretained(this), input_params_,
                                       output_params_, associated_id));
   WaitForCallback();
 }
 
 INSTANTIATE_TEST_CASE_P(, AudioSystemImplTest, testing::Values(false, true));
 
 }  // namespace media