[pulse] Add error checking while waiting for pa_operations.

The pulseaudio source code checks pa_context and pa_stream state
while waiting for operation completion. Lets do the same thing on
the off chance that's the source of our hangs.

See CHECK_DEAD_GOTO macro upstream:
https://github.com/pulseaudio/pulseaudio/blob/master/src/pulse/simple.c#L68

This change modifies the WaitForOperationCompletion() method to
take an optional pa_stream and/or pa_context and checks their
state while waiting. In cases where failure could be signaled as
an error upstream, a bool status code is used to do so.

BUG=986021
R=tguilbert

Change-Id: I7075293d1f74ad12f6c596bce39bbf0bba2c9a73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1811698
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697806}

media/audio/pulse/audio_manager_pulse.cc

@@ -31,6 +31,7 @@ constexpr int kMinimumOutputBufferSize = 512;
 constexpr int kMaximumOutputBufferSize = 8192;
 constexpr int kDefaultInputBufferSize = 1024;
 constexpr int kDefaultSampleRate = 48000;
+constexpr int kDefaultChannelCount = 2;
 
 AudioManagerPulse::AudioManagerPulse(std::unique_ptr<AudioThread> audio_thread,
                                      AudioLogFactory* audio_log_factory,
@@ -39,9 +40,9 @@ AudioManagerPulse::AudioManagerPulse(std::unique_ptr<AudioThread> audio_thread,
     : AudioManagerBase(std::move(audio_thread), audio_log_factory),
       input_mainloop_(pa_mainloop),
       input_context_(pa_context),
-      devices_(NULL),
-      native_input_sample_rate_(0),
-      native_channel_count_(0),
+      devices_(nullptr),
+      native_input_sample_rate_(kDefaultSampleRate),
+      native_channel_count_(kDefaultChannelCount),
       default_source_is_monitor_(false) {
   DCHECK(input_mainloop_);
   DCHECK(input_context_);
@@ -84,7 +85,7 @@ void AudioManagerPulse::GetAudioDeviceNames(
     operation = pa_context_get_sink_info_list(
         input_context_, OutputDevicesInfoCallback, this);
   }
-  WaitForOperationCompletion(input_mainloop_, operation);
+  WaitForOperationCompletion(input_mainloop_, operation, input_context_);
 
   // Prepend the default device if the list is not empty.
   if (!device_names->empty())
@@ -111,7 +112,7 @@ AudioParameters AudioManagerPulse::GetInputStreamParameters(
   auto* operation = pa_context_get_source_info_by_name(
       input_context_, default_source_name_.c_str(), DefaultSourceInfoCallback,
       this);
-  WaitForOperationCompletion(input_mainloop_, operation);
+  WaitForOperationCompletion(input_mainloop_, operation, input_context_);
 
   // We don't want to accidentally open a monitor device, so return invalid
   // parameters for those.
@@ -252,18 +253,9 @@ void AudioManagerPulse::UpdateNativeAudioHardwareInfo() {
   DCHECK(input_mainloop_);
   DCHECK(input_context_);
   AutoPulseLock auto_lock(input_mainloop_);
-
-  // Ensure the context hasn't gone bad after creation.
-  pa_context_state_t context_state = pa_context_get_state(input_context_);
-  if (!PA_CONTEXT_IS_GOOD(context_state)) {
-    LOG(ERROR) << "Can't retrieve hardware parameters. pa_context is bad: "
-               << context_state;
-    return;
-  }
-
   pa_operation* operation = pa_context_get_server_info(
       input_context_, AudioHardwareInfoCallback, this);
-  WaitForOperationCompletion(input_mainloop_, operation);
+  WaitForOperationCompletion(input_mainloop_, operation, input_context_);
 
   // Be careful about adding OS calls to this method.
   // GetPreferredOutputStreamParameters() calls this method on a critical path.

media/audio/pulse/pulse.sigs

@@ -30,6 +30,7 @@ pa_operation* pa_context_set_source_volume_by_index(pa_context* c, uint32_t idx,
 void pa_context_set_state_callback(pa_context* c, pa_context_notify_cb_t cb, void* userdata);
 pa_operation_state_t pa_operation_get_state(pa_operation* o);
 void pa_context_unref(pa_context* c);
+void pa_operation_cancel(pa_operation* o)
 void pa_operation_unref(pa_operation* o);
 int pa_stream_begin_write(pa_stream* p, void** data, size_t* nbytes);
 int pa_stream_connect_playback(pa_stream* s, const char* dev, const pa_buffer_attr* attr, pa_stream_flags_t flags, const pa_cvolume* volume,pa_stream* sync_stream);

media/audio/pulse/pulse_input.cc

@@ -88,7 +88,11 @@ void PulseAudioInputStream::Start(AudioInputCallback* callback) {
 
   pa_operation* operation =
       pa_stream_cork(handle_, 0, &pulse::StreamSuccessCallback, pa_mainloop_);
-  WaitForOperationCompletion(pa_mainloop_, operation);
+
+  if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                  handle_)) {
+    callback_->OnError();
+  }
 }
 
 void PulseAudioInputStream::Stop() {
@@ -106,16 +110,21 @@ void PulseAudioInputStream::Stop() {
   pa_stream_drop(handle_);
   fifo_.Clear();
 
-  pa_operation* operation = pa_stream_flush(handle_,
-                                            &pulse::StreamSuccessCallback,
-                                            pa_mainloop_);
-  WaitForOperationCompletion(pa_mainloop_, operation);
+  pa_operation* operation =
+      pa_stream_flush(handle_, &pulse::StreamSuccessCallback, pa_mainloop_);
+  if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                  handle_)) {
+    callback_->OnError();
+  }
 
   // Stop the stream.
   pa_stream_set_read_callback(handle_, NULL, NULL);
-  operation = pa_stream_cork(handle_, 1, &pulse::StreamSuccessCallback,
-                             pa_mainloop_);
-  WaitForOperationCompletion(pa_mainloop_, operation);
+  operation =
+      pa_stream_cork(handle_, 1, &pulse::StreamSuccessCallback, pa_mainloop_);
+  if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                  handle_)) {
+    callback_->OnError();
+  }
   callback_ = NULL;
 }
 
@@ -126,9 +135,9 @@ void PulseAudioInputStream::Close() {
     if (handle_) {
       // Disable all the callbacks before disconnecting.
       pa_stream_set_state_callback(handle_, NULL, NULL);
-      pa_operation* operation = pa_stream_flush(
-          handle_, &pulse::StreamSuccessCallback, pa_mainloop_);
-      WaitForOperationCompletion(pa_mainloop_, operation);
+      pa_operation* operation =
+          pa_stream_flush(handle_, &pulse::StreamSuccessCallback, pa_mainloop_);
+      WaitForOperationCompletion(pa_mainloop_, operation, pa_context_, handle_);
 
       if (pa_stream_get_state(handle_) != PA_STREAM_UNCONNECTED)
         pa_stream_disconnect(handle_);
@@ -158,10 +167,11 @@ void PulseAudioInputStream::SetVolume(double volume) {
   if (!channels_) {
     // Get the number of channels for the source only when the |channels_| is 0.
     // We are assuming the stream source is not changed on the fly here.
-    operation = pa_context_get_source_info_by_index(
-        pa_context_, index, &VolumeCallback, this);
-    WaitForOperationCompletion(pa_mainloop_, operation);
-    if (!channels_) {
+    operation = pa_context_get_source_info_by_index(pa_context_, index,
+                                                    &VolumeCallback, this);
+    if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                    handle_) ||
+        !channels_) {
       DLOG(WARNING) << "Failed to get the number of channels for the source";
       return;
     }
@@ -351,8 +361,8 @@ bool PulseAudioInputStream::GetSourceInformation(pa_source_info_cb_t callback) {
   size_t index = pa_stream_get_device_index(handle_);
   pa_operation* operation =
       pa_context_get_source_info_by_index(pa_context_, index, callback, this);
-  WaitForOperationCompletion(pa_mainloop_, operation);
-  return true;
+  return WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                    handle_);
 }
 
 }  // namespace media

media/audio/pulse/pulse_output.cc

@@ -91,7 +91,8 @@ void PulseAudioOutputStream::Reset() {
       // Ensure all samples are played out before shutdown.
       pa_operation* operation = pa_stream_flush(
           pa_stream_, &pulse::StreamSuccessCallback, pa_mainloop_);
-      WaitForOperationCompletion(pa_mainloop_, operation);
+      WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                 pa_stream_);
 
       // Release PulseAudio structures.
       pa_stream_disconnect(pa_stream_);
@@ -214,7 +215,10 @@ void PulseAudioOutputStream::Start(AudioSourceCallback* callback) {
   // Uncork (resume) the stream.
   pa_operation* operation = pa_stream_cork(
       pa_stream_, 0, &pulse::StreamSuccessCallback, pa_mainloop_);
-  WaitForOperationCompletion(pa_mainloop_, operation);
+  if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                  pa_stream_)) {
+    callback->OnError();
+  }
 }
 
 void PulseAudioOutputStream::Stop() {
@@ -226,18 +230,25 @@ void PulseAudioOutputStream::Stop() {
 
   // Set |source_callback_| to NULL so all FulfillWriteRequest() calls which may
   // occur while waiting on the flush and cork exit immediately.
-  source_callback_ = NULL;
+  auto* callback = source_callback_;
+  source_callback_ = nullptr;
 
   // Flush the stream prior to cork, doing so after will cause hangs.  Write
   // callbacks are suspended while inside pa_threaded_mainloop_lock() so this
   // is all thread safe.
-  pa_operation* operation = pa_stream_flush(
-      pa_stream_, &pulse::StreamSuccessCallback, pa_mainloop_);
-  WaitForOperationCompletion(pa_mainloop_, operation);
+  pa_operation* operation =
+      pa_stream_flush(pa_stream_, &pulse::StreamSuccessCallback, pa_mainloop_);
+  if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                  pa_stream_)) {
+    callback->OnError();
+  }
 
   operation = pa_stream_cork(pa_stream_, 1, &pulse::StreamSuccessCallback,
                              pa_mainloop_);
-  WaitForOperationCompletion(pa_mainloop_, operation);
+  if (!WaitForOperationCompletion(pa_mainloop_, operation, pa_context_,
+                                  pa_stream_)) {
+    callback->OnError();
+  }
 }
 
 void PulseAudioOutputStream::SetVolume(double volume) {

media/audio/pulse/pulse_util.cc

@@ -292,17 +292,43 @@ pa_channel_map ChannelLayoutToPAChannelMap(ChannelLayout channel_layout) {
   return channel_map;
 }
 
-void WaitForOperationCompletion(pa_threaded_mainloop* pa_mainloop,
-                                pa_operation* operation) {
+bool WaitForOperationCompletion(pa_threaded_mainloop* mainloop,
+                                pa_operation* operation,
+                                pa_context* optional_context,
+                                pa_stream* optional_stream) {
   if (!operation) {
-    DLOG(WARNING) << "Operation is NULL";
-    return;
+    LOG(ERROR) << "pa_operation is nullptr.";
+    return false;
   }
 
-  while (pa_operation_get_state(operation) == PA_OPERATION_RUNNING)
-    pa_threaded_mainloop_wait(pa_mainloop);
+  while (pa_operation_get_state(operation) == PA_OPERATION_RUNNING) {
+    if (optional_context) {
+      pa_context_state_t context_state = pa_context_get_state(optional_context);
+      if (!PA_CONTEXT_IS_GOOD(context_state)) {
+        LOG(ERROR) << "pa_context went bad while waiting: state="
+                   << context_state << ", error="
+                   << pa_strerror(pa_context_errno(optional_context));
+        pa_operation_cancel(operation);
+        pa_operation_unref(operation);
+        return false;
+      }
+    }
+
+    if (optional_stream) {
+      pa_stream_state_t stream_state = pa_stream_get_state(optional_stream);
+      if (!PA_STREAM_IS_GOOD(stream_state)) {
+        LOG(ERROR) << "pa_stream went bad while waiting: " << stream_state;
+        pa_operation_cancel(operation);
+        pa_operation_unref(operation);
+        return false;
+      }
+    }
+
+    pa_threaded_mainloop_wait(mainloop);
+  }
 
   pa_operation_unref(operation);
+  return true;
 }
 
 base::TimeDelta GetHardwareLatency(pa_stream* stream) {
@@ -534,7 +560,7 @@ std::string GetBusOfInput(pa_threaded_mainloop* mainloop,
   InputBusData data(mainloop, name);
   pa_operation* operation =
       pa_context_get_source_info_list(context, InputBusCallback, &data);
-  WaitForOperationCompletion(mainloop, operation);
+  WaitForOperationCompletion(mainloop, operation, context);
   return data.bus_;
 }
 
@@ -547,7 +573,7 @@ std::string GetOutputCorrespondingTo(pa_threaded_mainloop* mainloop,
   OutputBusData data(mainloop, bus);
   pa_operation* operation =
       pa_context_get_sink_info_list(context, OutputBusCallback, &data);
-  WaitForOperationCompletion(mainloop, operation);
+  WaitForOperationCompletion(mainloop, operation, context);
   return data.name_;
 }
 
@@ -560,7 +586,7 @@ std::string GetRealDefaultDeviceId(pa_threaded_mainloop* mainloop,
   DefaultDevicesData data(mainloop);
   pa_operation* operation =
       pa_context_get_server_info(context, &GetDefaultDeviceIdCallback, &data);
-  WaitForOperationCompletion(mainloop, operation);
+  WaitForOperationCompletion(mainloop, operation, context);
   return (type == RequestType::INPUT) ? data.input_ : data.output_;
 }
 

media/audio/pulse/pulse_util.h

@@ -50,8 +50,13 @@ void ContextStateCallback(pa_context* context, void* mainloop);
 
 pa_channel_map ChannelLayoutToPAChannelMap(ChannelLayout channel_layout);
 
-void WaitForOperationCompletion(pa_threaded_mainloop* mainloop,
-                                pa_operation* operation);
+// Blocks until pa_operation completes. If |optional_context| and/or
+// |optional_stream| are provided, the method will cancel |operation| and return
+// false if either the context or stream enter a bad state while waiting.
+bool WaitForOperationCompletion(pa_threaded_mainloop* mainloop,
+                                pa_operation* operation,
+                                pa_context* optional_context = nullptr,
+                                pa_stream* optional_stream = nullptr);
 
 base::TimeDelta GetHardwareLatency(pa_stream* stream);