From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Raymond Toy <rtoy@chromium.org>
Date: Tue, 29 Sep 2020 17:56:16 +0000
Subject: Add mutex for allowing the graph to be pulled

Basically add a mutex to protect access to
allow_pulling_audio_graph_.  The main thread waits until it has the
lock before setting this.  This prevents the main thread from deleting
things while the audio thread is pulling on the graph.

A try lock is used so as not to block the audio thread if it can't get
lock.

This is applied to both real time and offline contexts which required
moving the original real-time-only implementation to
audio_destination_node so we can use the same methods for the offline
context.

Tested the repro case from 1125635, and the issue does not reproduce.
We're assuming this will fix 1115901, but I've not been able to
reproduce that locally.

(cherry picked from commit 6785a406fd652f7a2f40933620ef4c979643c56b)

Bug: 1125635, 1115901
Change-Id: I1037d8c44225c6dcc8fe906c29a5a86740a15e1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410743
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#809393}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436324
Reviewed-by: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/branch-heads/4240@{#1083}
Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}

diff --git a/third_party/blink/renderer/modules/webaudio/audio_destination_node.cc b/third_party/blink/renderer/modules/webaudio/audio_destination_node.cc
index c85f6d2e26f2e34b8633c29754028afbd8f2cbb9..9b1a5ec495f8f3614e8e77fdeaf9eaa159b3e3ae 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_destination_node.cc
+++ b/third_party/blink/renderer/modules/webaudio/audio_destination_node.cc
@@ -30,7 +30,8 @@
 namespace blink {
 
 AudioDestinationHandler::AudioDestinationHandler(AudioNode& node)
-    : AudioHandler(kNodeTypeDestination, node, 0) {
+    : AudioHandler(kNodeTypeDestination, node, 0),
+      allow_pulling_audio_graph_(false) {
   AddInput();
 }
 
diff --git a/third_party/blink/renderer/modules/webaudio/audio_destination_node.h b/third_party/blink/renderer/modules/webaudio/audio_destination_node.h
index c35722c9296b6c98832444799124334d382225df..ce7bf61ef9ad18a5aa9669edb990542d2899acc2 100644
--- a/third_party/blink/renderer/modules/webaudio/audio_destination_node.h
+++ b/third_party/blink/renderer/modules/webaudio/audio_destination_node.h
@@ -71,6 +71,53 @@ 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,
diff --git a/third_party/blink/renderer/modules/webaudio/offline_audio_destination_node.cc b/third_party/blink/renderer/modules/webaudio/offline_audio_destination_node.cc
index 2b0d87395cef31793273ef69f61eb540be897fcf..697a3e684e9bc582a93b20ac163ceb790dfb62cc 100644
--- a/third_party/blink/renderer/modules/webaudio/offline_audio_destination_node.cc
+++ b/third_party/blink/renderer/modules/webaudio/offline_audio_destination_node.cc
@@ -92,6 +92,7 @@ void OfflineAudioDestinationHandler::Uninitialize() {
 
   render_thread_.reset();
 
+  DisablePullingAudioGraph();
   AudioHandler::Uninitialize();
 }
 
@@ -111,6 +112,7 @@ 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(
@@ -292,20 +294,34 @@ bool OfflineAudioDestinationHandler::RenderIfNotSuspended(
 
   DCHECK_GE(NumberOfInputs(), 1u);
 
-  // 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);
+  {
+    // 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);
+      }
 
-  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);
+    } else {
+      // Not allowed to pull on the graph or couldn't get the lock.
+      destination_bus->Zero();
+    }
   }
 
-  // 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);
 
diff --git a/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.cc b/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.cc
index 743cddb2d4bd7f04d8bdef7ce70241cbe2255430..dd183350f603d2c5997e89a62756a92c8ad566dc 100644
--- a/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.cc
+++ b/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.cc
@@ -52,8 +52,7 @@ RealtimeAudioDestinationHandler::RealtimeAudioDestinationHandler(
     base::Optional<float> sample_rate)
     : AudioDestinationHandler(node),
       latency_hint_(latency_hint),
-      sample_rate_(sample_rate),
-      allow_pulling_audio_graph_(false) {
+      sample_rate_(sample_rate) {
   // Node-specific default channel count and mixing rules.
   channel_count_ = 2;
   SetInternalChannelCountMode(kExplicit);
@@ -198,28 +197,38 @@ 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.
-  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.
+  {
+    // 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.
       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();
diff --git a/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.h b/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.h
index 9c8653880c37a02c87b2e82558dd2940a1b9a425..89e33ec6a9eb2b60ad1c2d46dcb748415bf2be33 100644
--- a/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.h
+++ b/third_party/blink/renderer/modules/webaudio/realtime_audio_destination_node.h
@@ -81,10 +81,6 @@ class RealtimeAudioDestinationHandler final : public AudioDestinationHandler,
   // 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);
-  }
-
  private:
   explicit RealtimeAudioDestinationHandler(AudioNode&,
                                            const WebAudioLatencyHint&,
@@ -94,32 +90,12 @@ class RealtimeAudioDestinationHandler final : public AudioDestinationHandler,
   void StartPlatformDestination();
   void StopPlatformDestination();
 
-  // 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.
   scoped_refptr<AudioDestination> platform_destination_;
 
   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_;
 };
 
 // -----------------------------------------------------------------------------
