Fix data race in CrasAudioHandler.

CrasAudioHandler is not thread safe. Its member variables should be only accessed from browser main thread. BUG=chromium:669239 TEST=Play youtube video. Plug/unplug headsets. Run media_unittests and chromeos_unittests. [email protected] (cherry picked from commit 88c1b13) Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I414b40eb09c7c0dcb45be2b78db93efe46162573 Reviewed-on: https://chromium-review.googlesource.com/637512 Commit-Queue: Wu-Cheng Li <[email protected]> Reviewed-by: Max Morin <[email protected]> Reviewed-by: Jenny Zhang <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#499393} Reviewed-on: https://chromium-review.googlesource.com/654402 Reviewed-by: Wu-Cheng Li <[email protected]> Cr-Commit-Position: refs/branch-heads/3202@{crosswalk-project#60} Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}
tenta-browser · Sep 7, 2017 · 0852b20 · 0852b20
1 parent c2f7710
commit 0852b20
Show file tree

Hide file tree

Showing 7 changed files with 207 additions and 21 deletions.
diff --git a/chromeos/audio/cras_audio_handler.cc b/chromeos/audio/cras_audio_handler.cc
@@ -141,6 +141,34 @@ CrasAudioHandler* CrasAudioHandler::Get() {
 }
 
 void CrasAudioHandler::OnVideoCaptureStarted(media::VideoFacingMode facing) {
+  DCHECK(main_task_runner_);
+  if (!main_task_runner_->BelongsToCurrentThread()) {
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&CrasAudioHandler::OnVideoCaptureStartedOnMainThread,
+                       weak_ptr_factory_.GetWeakPtr(), facing));
+    return;
+  }
+  // Unittest may call this from the main thread.
+  OnVideoCaptureStartedOnMainThread(facing);
+}
+
+void CrasAudioHandler::OnVideoCaptureStopped(media::VideoFacingMode facing) {
+  DCHECK(main_task_runner_);
+  if (!main_task_runner_->BelongsToCurrentThread()) {
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&CrasAudioHandler::OnVideoCaptureStoppedOnMainThread,
+                       weak_ptr_factory_.GetWeakPtr(), facing));
+    return;
+  }
+  // Unittest may call this from the main thread.
+  OnVideoCaptureStoppedOnMainThread(facing);
+}
+
+void CrasAudioHandler::OnVideoCaptureStartedOnMainThread(
+    media::VideoFacingMode facing) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   // Do nothing if the device doesn't have both front and rear microphones.
   if (!HasDualInternalMic())
     return;
@@ -178,7 +206,9 @@ void CrasAudioHandler::OnVideoCaptureStarted(media::VideoFacingMode facing) {
   ActivateMicForCamera(facing);
 }
 
-void CrasAudioHandler::OnVideoCaptureStopped(media::VideoFacingMode facing) {
+void CrasAudioHandler::OnVideoCaptureStoppedOnMainThread(
+    media::VideoFacingMode facing) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   // Do nothing if the device doesn't have both front and rear microphones.
   if (!HasDualInternalMic())
     return;
@@ -321,7 +351,6 @@ const AudioDevice* CrasAudioHandler::GetDeviceByType(AudioDeviceType type) {
 }
 
 void CrasAudioHandler::GetDefaultOutputBufferSize(int32_t* buffer_size) const {
-  base::AutoLock auto_lock(default_output_buffer_size_lock_);
   *buffer_size = default_output_buffer_size_;
 }
 
@@ -669,6 +698,9 @@ CrasAudioHandler::CrasAudioHandler(
   if (DBusThreadManager::Get()->GetSessionManagerClient())
     DBusThreadManager::Get()->GetSessionManagerClient()->AddObserver(this);
   InitializeAudioState();
+  // Unittest may not have the task runner for the current thread.
+  if (base::ThreadTaskRunnerHandle::IsSet())
+    main_task_runner_ = base::ThreadTaskRunnerHandle::Get();
 }
 
 CrasAudioHandler::~CrasAudioHandler() {
@@ -1598,7 +1630,6 @@ void CrasAudioHandler::HandleGetDefaultOutputBufferSize(int32_t buffer_size,
     return;
   }
 
-  base::AutoLock auto_lock(default_output_buffer_size_lock_);
   default_output_buffer_size_ = buffer_size;
 }
 

diff --git a/chromeos/audio/cras_audio_handler.h b/chromeos/audio/cras_audio_handler.h
@@ -27,6 +27,8 @@ namespace chromeos {
 
 class AudioDevicesPrefHandler;
 
+// This class is not thread safe. The public functions should be called on
+// browser main thread.
 class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
                                          public AudioPrefObserver,
                                          public SessionManagerClient::Observer,
@@ -158,8 +160,6 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
   uint64_t GetPrimaryActiveInputNode() const;
 
   // Gets the audio devices back in |device_list|.
-  // This call can be invoked from I/O thread or UI thread because
-  // it does not need to access CrasAudioClient on DBus.
   void GetAudioDevices(AudioDeviceList* device_list) const;
 
   bool GetPrimaryActiveOutputDevice(AudioDevice* device) const;
@@ -480,6 +480,9 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
   // Handle dbus callback for GetDefaultOutputBufferSize.
   void HandleGetDefaultOutputBufferSize(int buffer_size, bool success);
 
+  void OnVideoCaptureStartedOnMainThread(media::VideoFacingMode facing);
+  void OnVideoCaptureStoppedOnMainThread(media::VideoFacingMode facing);
+
   scoped_refptr<AudioDevicesPrefHandler> audio_pref_handler_;
   base::ObserverList<AudioObserver> observers_;
 
@@ -527,11 +530,13 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
   bool front_camera_on_ = false;
   bool rear_camera_on_ = false;
 
-  mutable base::Lock default_output_buffer_size_lock_;
-
   // Default output buffer size in frames.
   int32_t default_output_buffer_size_;
 
+  // Task runner of browser main thread. All member variables should be accessed
+  // on this thread.
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+
   base::WeakPtrFactory<CrasAudioHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CrasAudioHandler);

diff --git a/media/audio/audio_manager.h b/media/audio/audio_manager.h
@@ -88,7 +88,7 @@ class MEDIA_EXPORT AudioManager {
   // was created.
   // Returns true on success but false if AudioManager could not be shutdown.
   // AudioManager instance must not be deleted if shutdown failed.
-  bool Shutdown();
+  virtual bool Shutdown();
 
   // Log callback used for sending log messages from a stream to the object
   // that manages the stream.

diff --git a/media/audio/cras/audio_manager_cras.cc b/media/audio/cras/audio_manager_cras.cc
@@ -16,7 +16,9 @@
 #include "base/nix/xdg_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/sys_info.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/audio/audio_device.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "media/audio/audio_device_description.h"
@@ -58,6 +60,26 @@ enum CrosBeamformingDeviceState {
   BEAMFORMING_STATE_MAX = BEAMFORMING_USER_DISABLED
 };
 
+bool HasKeyboardMic(const chromeos::AudioDeviceList& devices) {
+  for (const auto& device : devices) {
+    if (device.is_input && device.type == chromeos::AUDIO_TYPE_KEYBOARD_MIC) {
+      return true;
+    }
+  }
+  return false;
+}
+
+const chromeos::AudioDevice* GetDeviceFromId(
+    const chromeos::AudioDeviceList& devices,
+    uint64_t id) {
+  for (const auto& device : devices) {
+    if (device.id == id) {
+      return &device;
+    }
+  }
+  return nullptr;
+}
+
 // Process |device_list| that two shares the same dev_index by creating a
 // virtual device name for them.
 void ProcessVirtualDeviceName(AudioDeviceNames* device_names,
@@ -87,7 +109,7 @@ bool AudioManagerCras::HasAudioOutputDevices() {
 
 bool AudioManagerCras::HasAudioInputDevices() {
   chromeos::AudioDeviceList devices;
-  chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices);
+  GetAudioDevices(&devices);
   for (size_t i = 0; i < devices.size(); ++i) {
     if (devices[i].is_input && devices[i].is_for_simple_usage())
       return true;
@@ -97,7 +119,12 @@ bool AudioManagerCras::HasAudioInputDevices() {
 
 AudioManagerCras::AudioManagerCras(std::unique_ptr<AudioThread> audio_thread,
                                    AudioLogFactory* audio_log_factory)
-    : AudioManagerBase(std::move(audio_thread), audio_log_factory) {
+    : AudioManagerBase(std::move(audio_thread), audio_log_factory),
+      on_shutdown_(base::WaitableEvent::ResetPolicy::MANUAL,
+                   base::WaitableEvent::InitialState::NOT_SIGNALED),
+      main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_ptr_factory_(this) {
+  weak_this_ = weak_ptr_factory_.GetWeakPtr();
   SetMaxOutputStreamsAllowed(kMaxOutputStreams);
 }
 
@@ -111,7 +138,7 @@ void AudioManagerCras::GetAudioDeviceNamesImpl(bool is_input,
 
   if (base::FeatureList::IsEnabled(features::kEnumerateAudioDevices)) {
     chromeos::AudioDeviceList devices;
-    chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices);
+    GetAudioDevices(&devices);
 
     // |dev_idx_map| is a map of dev_index and their audio devices.
     std::map<int, chromeos::AudioDeviceList> dev_idx_map;
@@ -159,7 +186,9 @@ AudioParameters AudioManagerCras::GetInputStreamParameters(
   AudioParameters params(AudioParameters::AUDIO_PCM_LOW_LATENCY,
                          CHANNEL_LAYOUT_STEREO, kDefaultSampleRate, 16,
                          buffer_size);
-  if (chromeos::CrasAudioHandler::Get()->HasKeyboardMic())
+  chromeos::AudioDeviceList devices;
+  GetAudioDevices(&devices);
+  if (HasKeyboardMic(devices))
     params.set_effects(AudioParameters::KEYBOARD_MIC);
 
   return params;
@@ -171,16 +200,15 @@ std::string AudioManagerCras::GetAssociatedOutputDeviceID(
     return "";
 
   chromeos::AudioDeviceList devices;
-  chromeos::CrasAudioHandler* audio_handler = chromeos::CrasAudioHandler::Get();
-  audio_handler->GetAudioDevices(&devices);
+  GetAudioDevices(&devices);
 
   // At this point, we know we have an ordinary input device, so we look up its
   // device_name, which identifies which hardware device it belongs to.
   uint64_t device_id = 0;
   if (!base::StringToUint64(input_device_id, &device_id))
     return "";
   const chromeos::AudioDevice* input_device =
-      audio_handler->GetDeviceFromId(device_id);
+      GetDeviceFromId(devices, device_id);
   if (!input_device)
     return "";
 
@@ -198,14 +226,36 @@ std::string AudioManagerCras::GetAssociatedOutputDeviceID(
 }
 
 std::string AudioManagerCras::GetDefaultOutputDeviceID() {
-  return base::Uint64ToString(
-      chromeos::CrasAudioHandler::Get()->GetPrimaryActiveOutputNode());
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  uint64_t active_output_node_id = 0;
+  if (main_task_runner_->BelongsToCurrentThread()) {
+    // Unittest may use the same thread for audio thread.
+    GetPrimaryActiveOutputNodeOnMainThread(&active_output_node_id, &event);
+  } else {
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &AudioManagerCras::GetPrimaryActiveOutputNodeOnMainThread,
+            weak_this_, base::Unretained(&active_output_node_id),
+            base::Unretained(&event)));
+  }
+  WaitEventOrShutdown(&event);
+  return base::Uint64ToString(active_output_node_id);
 }
 
 const char* AudioManagerCras::GetName() {
   return "CRAS";
 }
 
+bool AudioManagerCras::Shutdown() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  on_shutdown_.Signal();
+  return AudioManager::Shutdown();
+}
+
 AudioOutputStream* AudioManagerCras::MakeLinearOutputStream(
     const AudioParameters& params,
     const LogCallback& log_callback) {
@@ -240,8 +290,22 @@ AudioInputStream* AudioManagerCras::MakeLowLatencyInputStream(
 }
 
 int AudioManagerCras::GetDefaultOutputBufferSizePerBoard() {
-  int32_t buffer_size;
-  chromeos::CrasAudioHandler::Get()->GetDefaultOutputBufferSize(&buffer_size);
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  int32_t buffer_size = 512;
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  if (main_task_runner_->BelongsToCurrentThread()) {
+    // Unittest may use the same thread for audio thread.
+    GetDefaultOutputBufferSizeOnMainThread(&buffer_size, &event);
+  } else {
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &AudioManagerCras::GetDefaultOutputBufferSizeOnMainThread,
+            weak_this_, base::Unretained(&buffer_size),
+            base::Unretained(&event)));
+  }
+  WaitEventOrShutdown(&event);
   return static_cast<int>(buffer_size);
 }
 
@@ -304,4 +368,56 @@ bool AudioManagerCras::IsDefault(const std::string& device_id, bool is_input) {
   return device_name.unique_id == device_id;
 }
 
+void AudioManagerCras::GetAudioDevices(chromeos::AudioDeviceList* devices) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  if (main_task_runner_->BelongsToCurrentThread()) {
+    GetAudioDevicesOnMainThread(devices, &event);
+  } else {
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&AudioManagerCras::GetAudioDevicesOnMainThread,
+                       weak_this_, base::Unretained(devices),
+                       base::Unretained(&event)));
+  }
+  WaitEventOrShutdown(&event);
+}
+
+void AudioManagerCras::GetAudioDevicesOnMainThread(
+    chromeos::AudioDeviceList* devices,
+    base::WaitableEvent* event) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  // CrasAudioHandler is shut down before AudioManagerCras.
+  if (chromeos::CrasAudioHandler::IsInitialized()) {
+    chromeos::CrasAudioHandler::Get()->GetAudioDevices(devices);
+  }
+  event->Signal();
+}
+
+void AudioManagerCras::GetPrimaryActiveOutputNodeOnMainThread(
+    uint64_t* active_output_node_id,
+    base::WaitableEvent* event) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  if (chromeos::CrasAudioHandler::IsInitialized()) {
+    chromeos::CrasAudioHandler::Get()->GetPrimaryActiveOutputNode();
+  }
+  event->Signal();
+}
+
+void AudioManagerCras::GetDefaultOutputBufferSizeOnMainThread(
+    int32_t* buffer_size,
+    base::WaitableEvent* event) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  if (chromeos::CrasAudioHandler::IsInitialized()) {
+    chromeos::CrasAudioHandler::Get()->GetDefaultOutputBufferSize(buffer_size);
+  }
+  event->Signal();
+}
+
+void AudioManagerCras::WaitEventOrShutdown(base::WaitableEvent* event) {
+  base::WaitableEvent* waitables[] = {event, &on_shutdown_};
+  base::WaitableEvent::WaitMany(waitables, arraysize(waitables));
+}
+
 }  // namespace media
diff --git a/media/audio/cras/audio_manager_cras.h b/media/audio/cras/audio_manager_cras.h
@@ -14,6 +14,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "chromeos/audio/audio_device.h"
 #include "media/audio/audio_manager_base.h"
 
 namespace media {
@@ -35,6 +36,7 @@ class MEDIA_EXPORT AudioManagerCras : public AudioManagerBase {
       const std::string& input_device_id) override;
   std::string GetDefaultOutputDeviceID() override;
   const char* GetName() override;
+  bool Shutdown() override;
 
   // AudioManagerBase implementation.
   AudioOutputStream* MakeLinearOutputStream(
@@ -78,6 +80,28 @@ class MEDIA_EXPORT AudioManagerCras : public AudioManagerBase {
 
   void GetAudioDeviceNamesImpl(bool is_input, AudioDeviceNames* device_names);
 
+  void GetAudioDevices(chromeos::AudioDeviceList* devices);
+  void GetAudioDevicesOnMainThread(chromeos::AudioDeviceList* devices,
+                                   base::WaitableEvent* event);
+  void GetPrimaryActiveOutputNodeOnMainThread(uint64_t* active_output_node_id,
+                                              base::WaitableEvent* event);
+  void GetDefaultOutputBufferSizeOnMainThread(int32_t* buffer_size,
+                                              base::WaitableEvent* event);
+
+  void WaitEventOrShutdown(base::WaitableEvent* event);
+
+  // Signaled if AudioManagerCras is shutting down.
+  base::WaitableEvent on_shutdown_;
+
+  // Task runner of browser main thread. CrasAudioHandler should be only
+  // accessed on this thread.
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+
+  // For posting tasks from audio thread to |main_task_runner_|.
+  base::WeakPtr<AudioManagerCras> weak_this_;
+
+  base::WeakPtrFactory<AudioManagerCras> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioManagerCras);
 };