Assistant: Set audio owner delegate nullptr when Stop

When media Stop is called, AudioStreamHandler::OnStopped will delete both audio_stream_handler_ and device_owner_. However, device_owner_ is posted to a background thread to be deleted. It is possible that device_owner_ still using audio_stream_handler_ after audio_stream_handler_ has been deleted. Bug: b/113707507 Test: manual. Change-Id: I20a48a5193b78608e58c3a911af77af7ff8d647e Reviewed-on: https://chromium-review.googlesource.com/1200572 Reviewed-by: Xiaohui Chen <[email protected]> Commit-Queue: Tao Wu <[email protected]> Cr-Commit-Position: refs/heads/master@{#588928}
feiyue1206 · Sep 5, 2018 · 4c51d36 · 4c51d36
1 parent 20b2061
commit 4c51d36
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 7 deletions.
diff --git a/chromeos/services/assistant/platform/audio_output_provider_impl.cc b/chromeos/services/assistant/platform/audio_output_provider_impl.cc
@@ -131,6 +131,7 @@ class AudioOutputImpl : public assistant_client::AudioOutput {
 
   void Stop() override {
     if (IsEncodedFormat(format_)) {
+      device_owner_->SetDelegate(nullptr);
       main_thread_task_runner_->PostTask(
           FROM_HERE,
           base::BindOnce(&AudioStreamHandler::OnStopped,
@@ -306,9 +307,12 @@ void AudioDeviceOwner::StartOnMainThread(
 
 void AudioDeviceOwner::StopOnBackgroundThread() {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
+  base::AutoLock lock(lock_);
   output_device_.reset();
-  delegate_->OnStopped();
-  delegate_ = nullptr;
+  if (delegate_) {
+    delegate_->OnStopped();
+    delegate_ = nullptr;
+  }
 }
 
 void AudioDeviceOwner::StartDeviceOnBackgroundThread(
@@ -360,10 +364,17 @@ int AudioDeviceOwner::Render(base::TimeDelta delay,
 
 void AudioDeviceOwner::OnRenderError() {
   DVLOG(1) << "OnRenderError()";
+  base::AutoLock lock(lock_);
   if (delegate_)
     delegate_->OnError(assistant_client::AudioOutput::Error::FATAL_ERROR);
 }
 
+void AudioDeviceOwner::SetDelegate(
+    assistant_client::AudioOutput::Delegate* delegate) {
+  base::AutoLock lock(lock_);
+  delegate_ = delegate;
+}
+
 void AudioDeviceOwner::ScheduleFillLocked(const base::TimeTicks& time) {
   lock_.AssertAcquired();
   if (is_filling_)

diff --git a/chromeos/services/assistant/platform/audio_output_provider_impl.h b/chromeos/services/assistant/platform/audio_output_provider_impl.h
@@ -106,6 +106,8 @@ class AudioDeviceOwner : public media::AudioRendererSink::RenderCallback {
 
   void OnRenderError() override;
 
+  void SetDelegate(assistant_client::AudioOutput::Delegate* delegate);
+
  private:
   void StartDeviceOnBackgroundThread(
       std::unique_ptr<service_manager::Connector> connector);

diff --git a/chromeos/services/assistant/platform/audio_stream_handler.cc b/chromeos/services/assistant/platform/audio_stream_handler.cc
@@ -116,11 +116,9 @@ void AudioStreamHandler::OnDecoderInitializedOnThread(bool success,
           : assistant_client::OutputStreamEncoding::STREAM_PCM_S32,
       /*pcm_sample_rate=*/samples_per_second,
       /*pcm_num_channels=*/channels};
-  if (!device_owner_started_) {
-    DCHECK(start_device_owner_on_main_thread_);
+  if (start_device_owner_on_main_thread_) {
     DCHECK(!on_filled_);
     std::move(start_device_owner_on_main_thread_).Run(format);
-    device_owner_started_ = true;
   }
 }
 

diff --git a/chromeos/services/assistant/platform/audio_stream_handler.h b/chromeos/services/assistant/platform/audio_stream_handler.h
@@ -94,8 +94,6 @@ class AudioStreamHandler : public mojom::AssistantAudioDecoderClient,
   // Temporary storage of |on_filled| passed by |FillBuffer|.
   assistant_client::Callback1<int> on_filled_;
 
-  // True after |start_device_owner_on_main_thread_| is called.
-  bool device_owner_started_ = false;
   InitCB start_device_owner_on_main_thread_;
 
   base::circular_deque<std::vector<uint8_t>> decoded_data_;