Initialization of audio manager for Android is now done on the audio thread

media/audio/android/audio_android_unittest.cc

 // Copyright 2013 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 "base/android/build_info.h"
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
@@ skipping 490 matching lines @@
     tests.push_back(true);
   return tests;
 }
 
 // Test fixture class for tests which exercise the input path, or both input and
 // output paths. It is value-parameterized to test against both the Java
 // AudioRecord (when true) and native OpenSLES (when false) input paths.
 class AudioAndroidInputTest : public AudioAndroidOutputTest,
                               public testing::WithParamInterface<bool> {
  public:
-  AudioAndroidInputTest() {}
+  AudioAndroidInputTest()
+      : event_(false, false),
+        audio_input_stream_(NULL) {}
 
  protected:
+  AudioInputStream* audio_input_stream() { return audio_input_stream_; }
+
   AudioParameters GetInputStreamParameters() {
     AudioParameters input_params = audio_manager()->GetInputStreamParameters(
         AudioManagerBase::kDefaultDeviceId);
     // Override the platform effects setting to use the AudioRecord or OpenSLES
     // path as requested.
     int effects = GetParam() ? AudioParameters::ECHO_CANCELLER :
                                AudioParameters::NO_EFFECTS;
     AudioParameters params(input_params.format(),
                            input_params.channel_layout(),
                            input_params.input_channels(),
@@ skipping 36 matching lines @@
     VLOG(0) << "expected time between callbacks: "
             << expected_time_between_callbacks_ms << " ms";
     VLOG(0) << "average time between callbacks: "
             << average_time_between_callbacks_ms << " ms";
     EXPECT_GE(average_time_between_callbacks_ms,
               0.70 * expected_time_between_callbacks_ms);
     EXPECT_LE(average_time_between_callbacks_ms,
               1.30 * expected_time_between_callbacks_ms);
   }
 
+  void CreateAndCloseAISOnAudioThread(const AudioParameters& params) {
+    if (!audio_manager()->GetTaskRunner()->BelongsToCurrentThread()) {
+      audio_manager()->GetTaskRunner()->PostTask(
+          FROM_HERE,
+          base::Bind(&AudioAndroidInputTest::CreateAndCloseAISOnAudioThread,
+                     base::Unretained(this),
+                     params));
+      event_.Wait();
+    } else {
+      audio_input_stream_ = audio_manager()->MakeAudioInputStream(
+          params, AudioManagerBase::kDefaultDeviceId);
+      EXPECT_TRUE(audio_input_stream_);
+      audio_input_stream_->Close();
+      event_.Signal();
+    }
+  }
 
  private:
+  base::WaitableEvent event_;

[tommi (sloooow) - chröme, 2014/01/30 15:26:04]

This pattern is racy and could throw TSAN off.
CreateAndCloseAISOnAudioThread is now touching this variable on two different
threads (at least) and there's no DCHECK inside the |if
(!audio_manager()->GetTaskRunner()->BelongsToCurrentThread())| check that we're
always on the same thread there, so we're open to even more threads being in the
picture and that's a bug.

I realize that this is test code with very specific intentions but the bug is
not obvious so it's possible that the assumptions that are in here (and not
documented) go by unnoticed and someone copies this pattern into production
code.

[henrika (OOO until Aug 14), 2014/01/30 17:20:23]

Uploaded new and improved version after offline discussion with you. Please
check proposal of new pattern in next patch. Thanks.

+  AudioInputStream* audio_input_stream_;
   DISALLOW_COPY_AND_ASSIGN(AudioAndroidInputTest);
 };
 
 // Get the default audio input parameters and log the result.
 TEST_P(AudioAndroidInputTest, GetDefaultInputStreamParameters) {
   // We don't go through AudioAndroidInputTest::GetInputStreamParameters() here
   // so that we can log the real (non-overridden) values of the effects.
   AudioParameters params = audio_manager()->GetInputStreamParameters(
       AudioManagerBase::kDefaultDeviceId);
   EXPECT_TRUE(params.IsValid());
@@ skipping 30 matching lines @@
   if (!audio_manager()->HasAudioOutputDevices())
     return;
   AudioDeviceNames devices;
   audio_manager()->GetAudioOutputDeviceNames(&devices);
   CheckDeviceNames(devices);
 }
 
 // Ensure that a default input stream can be created and closed.
 TEST_P(AudioAndroidInputTest, CreateAndCloseInputStream) {
   AudioParameters params = GetInputStreamParameters();
-  AudioInputStream* ais = audio_manager()->MakeAudioInputStream(
-      params, AudioManagerBase::kDefaultDeviceId);
-  EXPECT_TRUE(ais);
-  ais->Close();
+  CreateAndCloseAISOnAudioThread(params);
 }
 
 // Ensure that a default output stream can be created and closed.
 // TODO(henrika): should we also verify that this API changes the audio mode
 // to communication mode, and calls RegisterHeadsetReceiver, the first time
 // it is called?
 TEST_F(AudioAndroidOutputTest, CreateAndCloseOutputStream) {
   AudioParameters params = GetDefaultOutputStreamParameters();
   AudioOutputStream* aos = audio_manager()->MakeAudioOutputStream(
       params, std::string(), std::string());
@@ skipping 226 matching lines @@
   aos->Stop();
   ais->Stop();
   aos->Close();
   ais->Close();
 }
 
 INSTANTIATE_TEST_CASE_P(AudioAndroidInputTest, AudioAndroidInputTest,
     testing::ValuesIn(RunAudioRecordInputPathTests()));
 
 }  // namespace media