Fix deadlock in processing audio

The fix for issue 1125635 and 1115901 causes a deadlock because
pulling on the audio graph that has a ScriptProcessorNode calls the
main thread to do the processing.  If the context goes away, the
destructor needs tries to get the lock which is being held by the
audio thread, waiting for the ScriptProcessorNode to return so the
lock can be released.  Hence a deadlock.

We revert this fix.  Local tests indicate that the repro case from
1125635 no longer triggers for whatever reason when it was very
reliable before.

While the deadlock is gone, this is also a speculative revert for
1125635 and 1115901.


Bug: 1136571, 1125635, 1115901
Change-Id: I7de5cd8e764272a7a320c4b89d2c187663f0ff5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2514806Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823418}

third_party/blink/renderer/modules/webaudio/audio_destination_node.cc

@@ -30,8 +30,7 @@
 namespace blink {
 
 AudioDestinationHandler::AudioDestinationHandler(AudioNode& node)
-    : AudioHandler(kNodeTypeDestination, node, 0),
-      allow_pulling_audio_graph_(false) {
+    : AudioHandler(kNodeTypeDestination, node, 0) {
   AddInput();
 }
 

third_party/blink/renderer/modules/webaudio/audio_destination_node.h

@@ -71,53 +71,6 @@ class AudioDestinationHandler : public AudioHandler {
     return is_execution_context_destroyed_;
   }
 
-  // Should only be called from
-  // RealtimeAudioDestinationHandler::StartPlatformDestination for a realtime
-  // context or OfflineAudioDestinationHandler::StartRendering for an offline
-  // context---basically wherever the context has started rendering.
-  // TODO(crbug.com/1128121): Consider removing this if possible
-  void EnablePullingAudioGraph() {
-    MutexLocker lock(allow_pulling_audio_graph_mutex_);
-    allow_pulling_audio_graph_.store(true, std::memory_order_release);
-  }
-
-  // Should only be called from
-  // RealtimeAudioDestinationHandler::StopPlatformDestination for a realtime
-  // context or from OfflineAudioDestinationHandler::Uninitialize for an offline
-  // context---basically whenever the context is being stopped.
-  // TODO(crbug.com/1128121): Consider removing this if possible
-  void DisablePullingAudioGraph() {
-    MutexLocker lock(allow_pulling_audio_graph_mutex_);
-    allow_pulling_audio_graph_.store(false, std::memory_order_release);
-  }
-
-  // TODO(crbug.com/1128121): Consider removing this if possible
-  bool IsPullingAudioGraphAllowed() const {
-    return allow_pulling_audio_graph_.load(std::memory_order_acquire);
-  }
-
-  // If true, the audio graph will be pulled to get new data.  Otherwise, the
-  // graph is not pulled, even if the audio thread is still running and
-  // requesting data.
-  //
-  // For an AudioContext, this MUST be modified only in
-  // RealtimeAudioDestinationHandler::StartPlatformDestination (via
-  // AudioDestinationHandler::EnablePullingAudioGraph) or
-  // RealtimeAudioDestinationHandler::StopPlatformDestination (via
-  // AudioDestinationHandler::DisablePullingAudioGraph), including destroying
-  // the the realtime context.
-  //
-  // For an OfflineAudioContext, this MUST be modified only when the the context
-  // is started or it has stopped rendering, including destroying the offline
-  // context.
-
-  // TODO(crbug.com/1128121): Consider removing this if possible
-  std::atomic_bool allow_pulling_audio_graph_;
-
-  // Protects allow_pulling_audio_graph_ from race conditions.  Must use try
-  // lock on the audio thread.
-  mutable Mutex allow_pulling_audio_graph_mutex_;
-
  protected:
   void AdvanceCurrentSampleFrame(size_t number_of_frames) {
     current_sample_frame_.fetch_add(number_of_frames,

third_party/blink/renderer/modules/webaudio/offline_audio_destination_node.cc

@@ -96,7 +96,6 @@ void OfflineAudioDestinationHandler::Uninitialize() {
   if (render_thread_)
     render_thread_.reset();
 
-  DisablePullingAudioGraph();
   AudioHandler::Uninitialize();
 }
 
@@ -116,7 +115,6 @@ void OfflineAudioDestinationHandler::StartRendering() {
   // Rendering was not started. Starting now.
   if (!is_rendering_started_) {
     is_rendering_started_ = true;
-    EnablePullingAudioGraph();
     PostCrossThreadTask(
         *render_thread_task_runner_, FROM_HERE,
         CrossThreadBindOnce(
@@ -298,34 +296,20 @@ bool OfflineAudioDestinationHandler::RenderIfNotSuspended(
 
   DCHECK_GE(NumberOfInputs(), 1u);
 
-  {
-    // The entire block that relies on |IsPullingAudioGraphAllowed| needs
-    // locking to prevent pulling audio graph being disallowed (i.e. a
-    // destruction started) in the middle of processing
-    MutexTryLocker try_locker(allow_pulling_audio_graph_mutex_);
-
-    if (IsPullingAudioGraphAllowed() && try_locker.Locked()) {
-      // This will cause the node(s) connected to us to process, which in turn
-      // will pull on their input(s), all the way backwards through the
-      // rendering graph.
-      scoped_refptr<AudioBus> rendered_bus =
-          Input(0).Pull(destination_bus, number_of_frames);
-
-      if (!rendered_bus) {
-        destination_bus->Zero();
-      } else if (rendered_bus != destination_bus) {
-        // in-place processing was not possible - so copy
-        destination_bus->CopyFrom(*rendered_bus);
-      }
+  // This will cause the node(s) connected to us to process, which in turn will
+  // pull on their input(s), all the way backwards through the rendering graph.
+  scoped_refptr<AudioBus> rendered_bus =
+      Input(0).Pull(destination_bus, number_of_frames);
 
-    } else {
-      // Not allowed to pull on the graph or couldn't get the lock.
-      destination_bus->Zero();
-    }
+  if (!rendered_bus) {
+    destination_bus->Zero();
+  } else if (rendered_bus != destination_bus) {
+    // in-place processing was not possible - so copy
+    destination_bus->CopyFrom(*rendered_bus);
   }
 
-  // Process nodes which need a little extra help because they are not
-  // connected to anything, but still need to process.
+  // Process nodes which need a little extra help because they are not connected
+  // to anything, but still need to process.
   Context()->GetDeferredTaskHandler().ProcessAutomaticPullNodes(
       number_of_frames);
 

third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.cc

@@ -53,6 +53,7 @@ RealtimeAudioDestinationHandler::RealtimeAudioDestinationHandler(
     : AudioDestinationHandler(node),
       latency_hint_(latency_hint),
       sample_rate_(sample_rate),
+      allow_pulling_audio_graph_(false),
       task_runner_(Context()->GetExecutionContext()->GetTaskRunner(
           TaskType::kInternalMediaRealTime)) {
   // Node-specific default channel count and mixing rules.
@@ -199,38 +200,28 @@ void RealtimeAudioDestinationHandler::Render(
   // Only pull on the audio graph if we have not stopped the destination.  It
   // takes time for the destination to stop, but we want to stop pulling before
   // the destination has actually stopped.
-  {
-    // The entire block that relies on |IsPullingAudioGraphAllowed| needs
-    // locking to prevent pulling audio graph being disallowed (i.e. a
-    // destruction started) in the middle of processing.
-    MutexTryLocker try_locker(allow_pulling_audio_graph_mutex_);
-
-    if (IsPullingAudioGraphAllowed() && try_locker.Locked()) {
-      // Renders the graph by pulling all the inputs to this node. This will in
-      // turn pull on their inputs, all the way backwards through the graph.
-      scoped_refptr<AudioBus> rendered_bus =
-          Input(0).Pull(destination_bus, number_of_frames);
-
-      DCHECK(rendered_bus);
-      if (!rendered_bus) {
-        // AudioNodeInput might be in the middle of destruction. Then the
-        // internal summing bus will return as nullptr. Then zero out the
-        // output.
-        destination_bus->Zero();
-      } else if (rendered_bus != destination_bus) {
-        // In-place processing was not possible. Copy the rendered result to the
-        // given |destination_bus| buffer.
-        destination_bus->CopyFrom(*rendered_bus);
-      }
-    } else {
-      // Not allowed to pull on the graph or couldn't get the lock.
+  if (IsPullingAudioGraphAllowed()) {
+    // Renders the graph by pulling all the inputs to this node. This will in
+    // turn pull on their inputs, all the way backwards through the graph.
+    scoped_refptr<AudioBus> rendered_bus =
+        Input(0).Pull(destination_bus, number_of_frames);
+
+    DCHECK(rendered_bus);
+    if (!rendered_bus) {
+      // AudioNodeInput might be in the middle of destruction. Then the internal
+      // summing bus will return as nullptr. Then zero out the output.
       destination_bus->Zero();
+    } else if (rendered_bus != destination_bus) {
+      // In-place processing was not possible. Copy the rendered result to the
+      // given |destination_bus| buffer.
+      destination_bus->CopyFrom(*rendered_bus);
     }
+  } else {
+    destination_bus->Zero();
   }
 
-  // Processes "automatic" nodes that are not connected to anything. This
-  // can be done after copying because it does not affect the rendered
-  // result.
+  // Processes "automatic" nodes that are not connected to anything. This can
+  // be done after copying because it does not affect the rendered result.
   context->GetDeferredTaskHandler().ProcessAutomaticPullNodes(number_of_frames);
 
   context->HandlePostRenderTasks();

third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.h

@@ -84,6 +84,10 @@ class RealtimeAudioDestinationHandler final
   // Returns a given frames-per-buffer size from audio infra.
   int GetFramesPerBuffer() const;
 
+  bool IsPullingAudioGraphAllowed() const {
+    return allow_pulling_audio_graph_.load(std::memory_order_acquire);
+  }
+
   // Sets the detect silence flag for the platform destination.
   void SetDetectSilence(bool detect_silence);
 
@@ -101,6 +105,16 @@ class RealtimeAudioDestinationHandler final
   // Render() method.
   void SetDetectSilenceIfNecessary(bool has_automatic_pull_nodes);
 
+  // Should only be called from StartPlatformDestination.
+  void EnablePullingAudioGraph() {
+    allow_pulling_audio_graph_.store(true, std::memory_order_release);
+  }
+
+  // Should only be called from StopPlatformDestination.
+  void DisablePullingAudioGraph() {
+    allow_pulling_audio_graph_.store(false, std::memory_order_release);
+  }
+
   const WebAudioLatencyHint latency_hint_;
 
   // Holds the audio device thread that runs the real time audio context.
@@ -108,6 +122,16 @@ class RealtimeAudioDestinationHandler final
 
   base::Optional<float> sample_rate_;
 
+  // If true, the audio graph will be pulled to get new data.  Otherwise, the
+  // graph is not pulled, even if the audio thread is still running and
+  // requesting data.
+  //
+  // Must be modified only in StartPlatformDestination (via
+  // EnablePullingAudioGraph) or StopPlatformDestination (via
+  // DisablePullingAudioGraph) .  This is modified only by the main threda and
+  // the audio thread only reads this.
+  std::atomic_bool allow_pulling_audio_graph_;
+
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   // Represents the current condition of silence detection. By default, the