Factor out AudioNodeInput/AudioNodeOutput wiring logic and fix an ordering issue.

Previously, AudioNodeOutput::RemoveInput could call AudioHandler::BreakConnectionWithLock
at a time when one of the mutual sets of raw pointers has been updated but the other has
not. This causes the Disable logic to go awry, leading to DCHECK failures and, after
another recent change, ASAN violations, since it can be entered as part of
AudioHandler::DisableOutputsIfNecessary during BreakConnectionWithLock.

This was not obvious by inspection in part because this logic was distributed across
several methods in different classes. To make the order in which these adjustments are
made more clear, factor out the wiring logic from AudioNodeInput and AudioNodeOutput
(and strictly for consistency, AudioParamHandler) into a collection of dedicated
functions.

Call sites are rewritten to call this code, which manages all changes to inputs_,
outputs_ and disabled_outputs_ centrally.

Bug: 910098
Change-Id: Ide367d89cf6601e823169082833d7a41f1019f81
Reviewed-on: https://chromium-review.googlesource.com/c/1359134
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614755}

third_party/blink/renderer/modules/webaudio/BUILD.gn

@@ -30,6 +30,8 @@ blink_modules_sources("webaudio") {
     "audio_node_input.h",
     "audio_node_output.cc",
     "audio_node_output.h",
+    "audio_node_wiring.cc",
+    "audio_node_wiring.h",
     "audio_param.cc",
     "audio_param.h",
     "audio_param_map.cc",

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

@@ -27,6 +27,7 @@
 #include "third_party/blink/renderer/modules/webaudio/audio_node_input.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_node_options.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_node_output.h"
+#include "third_party/blink/renderer/modules/webaudio/audio_node_wiring.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_param.h"
 #include "third_party/blink/renderer/modules/webaudio/base_audio_context.h"
 #include "third_party/blink/renderer/platform/bindings/exception_messages.h"
@@ -729,9 +730,8 @@ AudioNode* AudioNode::connect(AudioNode* destination,
     return nullptr;
   }
 
-  destination->Handler()
-      .Input(input_index)
-      .Connect(Handler().Output(output_index));
+  AudioNodeWiring::Connect(Handler().Output(output_index),
+                           destination->Handler().Input(input_index));
   if (!connected_nodes_[output_index]) {
     connected_nodes_[output_index] =
         MakeGarbageCollected<HeapHashSet<Member<AudioNode>>>();
@@ -779,7 +779,7 @@ void AudioNode::connect(AudioParam* param,
     return;
   }
 
-  param->Handler().Connect(Handler().Output(output_index));
+  AudioNodeWiring::Connect(Handler().Output(output_index), param->Handler());
   if (!connected_params_[output_index]) {
     connected_params_[output_index] =
         MakeGarbageCollected<HeapHashSet<Member<AudioParam>>>();
@@ -802,9 +802,9 @@ bool AudioNode::DisconnectFromOutputIfConnected(
   AudioNodeOutput& output = Handler().Output(output_index);
   AudioNodeInput& input =
       destination.Handler().Input(input_index_of_destination);
-  if (!output.IsConnectedToInput(input))
+  if (!AudioNodeWiring::IsConnected(output, input))
     return false;
-  output.DisconnectInput(input);
+  AudioNodeWiring::Disconnect(output, input);
   connected_nodes_[output_index]->erase(&destination);
   return true;
 }
@@ -812,9 +812,9 @@ bool AudioNode::DisconnectFromOutputIfConnected(
 bool AudioNode::DisconnectFromOutputIfConnected(unsigned output_index,
                                                 AudioParam& param) {
   AudioNodeOutput& output = Handler().Output(output_index);
-  if (!output.IsConnectedToAudioParam(param.Handler()))
+  if (!AudioNodeWiring::IsConnected(output, param.Handler()))
     return false;
-  output.DisconnectAudioParam(param.Handler());
+  AudioNodeWiring::Disconnect(output, param.Handler());
   connected_params_[output_index]->erase(&param);
   return true;
 }

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

@@ -30,6 +30,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_node_output.h"
+#include "third_party/blink/renderer/modules/webaudio/audio_node_wiring.h"
 
 namespace blink {
 
@@ -42,85 +43,13 @@ AudioNodeInput::AudioNodeInput(AudioHandler& handler)
 }
 
 AudioNodeInput::~AudioNodeInput() {
-  GetDeferredTaskHandler().AssertGraphOwner();
-
-  for (AudioNodeOutput* output : outputs_)
-    output->RemoveInput(*this);
-  for (AudioNodeOutput* output : disabled_outputs_)
-    output->RemoveInput(*this);
-  outputs_.clear();
-  disabled_outputs_.clear();
-  ChangedOutputs();
+  AudioNodeWiring::WillBeDestroyed(*this);
 }
 
 std::unique_ptr<AudioNodeInput> AudioNodeInput::Create(AudioHandler& handler) {
   return base::WrapUnique(new AudioNodeInput(handler));
 }
 
-void AudioNodeInput::Connect(AudioNodeOutput& output) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-
-  // Check if we're already connected to this output.
-  if (outputs_.Contains(&output))
-    return;
-
-  output.AddInput(*this);
-  outputs_.insert(&output);
-  ChangedOutputs();
-}
-
-void AudioNodeInput::Disconnect(AudioNodeOutput& output) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-
-  // First try to disconnect from "active" connections.
-  if (outputs_.Contains(&output)) {
-    outputs_.erase(&output);
-    ChangedOutputs();
-    output.RemoveInput(*this);
-    // Note: it's important to return immediately after removeInput() calls
-    // since the node may be deleted.
-    return;
-  }
-
-  // Otherwise, try to disconnect from disabled connections.
-  if (disabled_outputs_.Contains(&output)) {
-    disabled_outputs_.erase(&output);
-    output.RemoveInput(*this);
-    // Note: it's important to return immediately after all removeInput() calls
-    // since the node may be deleted.
-    return;
-  }
-
-  NOTREACHED();
-}
-
-void AudioNodeInput::Disable(AudioNodeOutput& output) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  DCHECK(outputs_.Contains(&output));
-
-  disabled_outputs_.insert(&output);
-  outputs_.erase(&output);
-  ChangedOutputs();
-
-  // Propagate disabled state to outputs.
-  Handler().DisableOutputsIfNecessary();
-}
-
-void AudioNodeInput::Enable(AudioNodeOutput& output) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-
-  // Move output from disabled list to active list.
-  outputs_.insert(&output);
-  if (disabled_outputs_.size() > 0) {
-    DCHECK(disabled_outputs_.Contains(&output));
-    disabled_outputs_.erase(&output);
-  }
-  ChangedOutputs();
-
-  // Propagate enabled state to outputs.
-  Handler().EnableOutputsIfNecessary();
-}
-
 void AudioNodeInput::DidUpdate() {
   Handler().CheckNumberOfChannelsForInput(this);
 }

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

@@ -43,7 +43,9 @@ class AudioNodeOutput;
 // input will act as a unity-gain summing junction, mixing all the outputs.  The
 // number of channels of the input's bus is the maximum of the number of
 // channels of all its connections.
-
+//
+// Use AudioNodeWiring to connect the AudioNodeOutput of another node to this,
+// and to disconnect, enable or disable that connection afterward.
 class MODULES_EXPORT AudioNodeInput final : public AudioSummingJunction {
   USING_FAST_MALLOC(AudioNodeInput);
 
@@ -57,17 +59,6 @@ class MODULES_EXPORT AudioNodeInput final : public AudioSummingJunction {
   // Can be called from any thread.
   AudioHandler& Handler() const { return handler_; }
 
-  // Must be called with the context's graph lock.
-  void Connect(AudioNodeOutput&);
-  void Disconnect(AudioNodeOutput&);
-
-  // disable() will take the output out of the active connections list and set
-  // aside in a disabled list.
-  // enable() will put the output back into the active connections list.
-  // Must be called with the context's graph lock.
-  void Enable(AudioNodeOutput&);
-  void Disable(AudioNodeOutput&);
-
   // pull() processes all of the AudioNodes connected to us.
   // In the case of multiple connections it sums the result into an internal
   // summing bus.  In the single connection case, it allows in-place processing
@@ -112,6 +103,9 @@ class MODULES_EXPORT AudioNodeInput final : public AudioSummingJunction {
   void SumAllConnections(AudioBus* summing_bus, uint32_t frames_to_process);
 
   scoped_refptr<AudioBus> internal_summing_bus_;
+
+  // Used to connect inputs and outputs together.
+  friend class AudioNodeWiring;
 };
 
 }  // namespace blink

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

@@ -9,6 +9,7 @@
 #include "third_party/blink/renderer/core/testing/dummy_page_holder.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_node_input.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_node_output.h"
+#include "third_party/blink/renderer/modules/webaudio/audio_node_wiring.h"
 #include "third_party/blink/renderer/modules/webaudio/delay_node.h"
 #include "third_party/blink/renderer/modules/webaudio/offline_audio_context.h"
 
@@ -28,7 +29,7 @@ TEST(AudioNodeInputTest, InputDestroyedBeforeOutput) {
 
   {
     BaseAudioContext::GraphAutoLocker graph_lock(context);
-    input->Connect(*output);
+    AudioNodeWiring::Connect(*output, *input);
     ASSERT_TRUE(output->IsConnected());
 
     // This should not crash.
@@ -52,7 +53,7 @@ TEST(AudioNodeInputTest, OutputDestroyedBeforeInput) {
 
   {
     BaseAudioContext::GraphAutoLocker graph_lock(context);
-    input->Connect(*output);
+    AudioNodeWiring::Connect(*output, *input);
     ASSERT_TRUE(output->IsConnected());
 
     // This should not crash.

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

@@ -29,6 +29,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_node_input.h"
+#include "third_party/blink/renderer/modules/webaudio/audio_node_wiring.h"
 #include "third_party/blink/renderer/modules/webaudio/base_audio_context.h"
 #include "third_party/blink/renderer/platform/wtf/threading.h"
 
@@ -162,54 +163,26 @@ unsigned AudioNodeOutput::RenderingFanOutCount() const {
   return rendering_fan_out_count_;
 }
 
-void AudioNodeOutput::AddInput(AudioNodeInput& input) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  inputs_.insert(&input);
-  input.Handler().MakeConnection();
-}
-
-void AudioNodeOutput::RemoveInput(AudioNodeInput& input) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  input.Handler().BreakConnectionWithLock();
-  inputs_.erase(&input);
-}
-
 void AudioNodeOutput::DisconnectAllInputs() {
   GetDeferredTaskHandler().AssertGraphOwner();
 
-  // AudioNodeInput::disconnect() changes m_inputs by calling removeInput().
-  while (!inputs_.IsEmpty())
-    (*inputs_.begin())->Disconnect(*this);
-}
-
-void AudioNodeOutput::DisconnectInput(AudioNodeInput& input) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  DCHECK(IsConnectedToInput(input));
-  input.Disconnect(*this);
-}
-
-void AudioNodeOutput::DisconnectAudioParam(AudioParamHandler& param) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  DCHECK(IsConnectedToAudioParam(param));
-  param.Disconnect(*this);
-}
-
-void AudioNodeOutput::AddParam(AudioParamHandler& param) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  params_.insert(&param);
-}
-
-void AudioNodeOutput::RemoveParam(AudioParamHandler& param) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  params_.erase(&param);
+  // Disconnect changes inputs_, so we can't iterate directly over the hash set.
+  Vector<AudioNodeInput*, 4> inputs;
+  CopyToVector(inputs_, inputs);
+  for (AudioNodeInput* input : inputs)
+    AudioNodeWiring::Disconnect(*this, *input);
+  DCHECK(inputs_.IsEmpty());
 }
 
 void AudioNodeOutput::DisconnectAllParams() {
   GetDeferredTaskHandler().AssertGraphOwner();
 
-  // AudioParam::disconnect() changes m_params by calling removeParam().
-  while (!params_.IsEmpty())
-    (*params_.begin())->Disconnect(*this);
+  // Disconnect changes params_, so we can't iterate directly over the hash set.
+  Vector<AudioParamHandler*, 4> params;
+  CopyToVector(params_, params);
+  for (AudioParamHandler* param : params)
+    AudioNodeWiring::Disconnect(*this, *param);
+  DCHECK(params_.IsEmpty());
 }
 
 void AudioNodeOutput::DisconnectAll() {
@@ -217,23 +190,13 @@ void AudioNodeOutput::DisconnectAll() {
   DisconnectAllParams();
 }
 
-bool AudioNodeOutput::IsConnectedToInput(AudioNodeInput& input) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  return inputs_.Contains(&input);
-}
-
-bool AudioNodeOutput::IsConnectedToAudioParam(AudioParamHandler& param) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-  return params_.Contains(&param);
-}
-
 void AudioNodeOutput::Disable() {
   GetDeferredTaskHandler().AssertGraphOwner();
 
   if (is_enabled_) {
     is_enabled_ = false;
-    for (AudioNodeInput* i : inputs_)
-      i->Disable(*this);
+    for (AudioNodeInput* input : inputs_)
+      AudioNodeWiring::Disable(*this, *input);
   }
 }
 
@@ -242,8 +205,8 @@ void AudioNodeOutput::Enable() {
 
   if (!is_enabled_) {
     is_enabled_ = true;
-    for (AudioNodeInput* i : inputs_)
-      i->Enable(*this);
+    for (AudioNodeInput* input : inputs_)
+      AudioNodeWiring::Enable(*this, *input);
   }
 }
 

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

@@ -70,7 +70,6 @@ class MODULES_EXPORT AudioNodeOutput final {
   void DisconnectAll();
 
   // Disconnect a specific input or AudioParam.
-  void DisconnectInput(AudioNodeInput&);
   void DisconnectAudioParam(AudioParamHandler&);
 
   void SetNumberOfChannels(unsigned);
@@ -79,10 +78,6 @@ class MODULES_EXPORT AudioNodeOutput final {
 
   bool IsConnected() { return FanOutCount() > 0 || ParamFanOutCount() > 0; }
 
-  // Probe if the output node is connected with a certain input or AudioParam
-  bool IsConnectedToInput(AudioNodeInput&);
-  bool IsConnectedToAudioParam(AudioParamHandler&);
-
   // Disable/Enable happens when there are still JavaScript references to a
   // node, but it has otherwise "finished" its work.  For example, when a note
   // has finished playing.  It is kept around, because it may be played again at
@@ -107,16 +102,6 @@ class MODULES_EXPORT AudioNodeOutput final {
   // object.
   AudioHandler& handler_;
 
-  friend class AudioNodeInput;
-  friend class AudioParamHandler;
-
-  // These are called from AudioNodeInput.
-  // They must be called with the context's graph lock.
-  void AddInput(AudioNodeInput&);
-  void RemoveInput(AudioNodeInput&);
-  void AddParam(AudioParamHandler&);
-  void RemoveParam(AudioParamHandler&);
-
   // fanOutCount() is the number of AudioNodeInputs that we're connected to.
   // This method should not be called in audio thread rendering code, instead
   // renderingFanOutCount() should be used.
@@ -180,6 +165,8 @@ class MODULES_EXPORT AudioNodeOutput final {
   // This collection of raw pointers is safe because they are retained by
   // AudioParam objects retained by m_connectedParams of the owner AudioNode.
   HashSet<AudioParamHandler*> params_;
+
+  friend class AudioNodeWiring;
 };
 
 }  // namespace blink

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/modules/webaudio/audio_node_wiring.h"
+
+#include "third_party/blink/renderer/modules/webaudio/audio_node_input.h"
+#include "third_party/blink/renderer/modules/webaudio/audio_node_output.h"
+#include "third_party/blink/renderer/modules/webaudio/deferred_task_handler.h"
+#include "third_party/blink/renderer/platform/wtf/hash_set.h"
+
+namespace blink {
+
+namespace {
+
+using AudioNodeOutputSet = HashSet<AudioNodeOutput*>;
+
+struct FindOutputResult {
+  AudioNodeOutputSet& output_set;
+  AudioNodeOutputSet::const_iterator iterator;
+  bool is_disabled;
+};
+
+// Given a connected output, finds it in the "active" or "disabled" set (e.g.
+// of the outputs connected to an input). Produces the set in which it was
+// found, an iterator into that set (so that it can be erased), and whether or
+// not the set it was found in was the disabled set.
+//
+// It is an error to pass an output which is *not* connected (i.e. is neither
+// active nor disabled).
+FindOutputResult FindOutput(AudioNodeOutput& output,
+                            AudioNodeOutputSet& outputs,
+                            AudioNodeOutputSet& disabled_outputs) {
+  auto it = outputs.find(&output);
+  if (it != outputs.end())
+    return {outputs, it, false};
+
+  it = disabled_outputs.find(&output);
+  if (it != disabled_outputs.end())
+    return {disabled_outputs, it, true};
+
+  NOTREACHED() << "The output must be connected to the input.";
+  return {outputs, {}, false};
+}
+
+}  // namespace
+
+void AudioNodeWiring::Connect(AudioNodeOutput& output, AudioNodeInput& input) {
+  input.GetDeferredTaskHandler().AssertGraphOwner();
+
+  const bool input_connected_to_output =
+      input.outputs_.Contains(&output) ||
+      input.disabled_outputs_.Contains(&output);
+  const bool output_connected_to_input = output.inputs_.Contains(&input);
+  DCHECK_EQ(input_connected_to_output, output_connected_to_input);
+
+  // Do nothing if already connected.
+  if (input_connected_to_output)
+    return;
+
+  (output.is_enabled_ ? input.outputs_ : input.disabled_outputs_)
+      .insert(&output);
+  output.inputs_.insert(&input);
+
+  // If it has gained an active connection, the input may need to have its
+  // rendering state updated.
+  if (output.is_enabled_)
+    input.ChangedOutputs();
+
+  // The input node's handler needs to know about this connection. This may
+  // cause it to re-enable itself.
+  input.Handler().MakeConnection();
+}
+
+void AudioNodeWiring::Connect(AudioNodeOutput& output,
+                              AudioParamHandler& param) {
+  param.GetDeferredTaskHandler().AssertGraphOwner();
+
+  const bool param_connected_to_output = param.outputs_.Contains(&output);
+  const bool output_connected_to_param = output.params_.Contains(&param);
+  DCHECK_EQ(param_connected_to_output, output_connected_to_param);
+
+  // Do nothing if already connected.
+  if (param_connected_to_output)
+    return;
+
+  param.outputs_.insert(&output);
+  output.params_.insert(&param);
+
+  // The param may need to have its rendering state updated.
+  param.ChangedOutputs();
+}
+
+void AudioNodeWiring::Disconnect(AudioNodeOutput& output,
+                                 AudioNodeInput& input) {
+  input.GetDeferredTaskHandler().AssertGraphOwner();
+
+  // These must be connected.
+  DCHECK(output.inputs_.Contains(&input));
+  DCHECK(input.outputs_.Contains(&output) ||
+         input.disabled_outputs_.Contains(&output));
+
+  // Find the output in the appropriate place.
+  auto result = FindOutput(output, input.outputs_, input.disabled_outputs_);
+
+  // Erase the pointers from both sets.
+  result.output_set.erase(result.iterator);
+  output.inputs_.erase(&input);
+
+  // If an active connection was disconnected, the input may need to have its
+  // rendering state updated.
+  if (!result.is_disabled)
+    input.ChangedOutputs();
+
+  // The input node's handler may try to disable itself if this was the last
+  // connection. This must happen after the set erasures above, or the disabling
+  // logic would observe an inconsistent state.
+  input.Handler().BreakConnectionWithLock();
+}
+
+void AudioNodeWiring::Disconnect(AudioNodeOutput& output,
+                                 AudioParamHandler& param) {
+  param.GetDeferredTaskHandler().AssertGraphOwner();
+
+  DCHECK(param.outputs_.Contains(&output));
+  DCHECK(output.params_.Contains(&param));
+
+  // Erase the pointers from both sets.
+  param.outputs_.erase(&output);
+  output.params_.erase(&param);
+
+  // The param may need to have its rendering state updated.
+  param.ChangedOutputs();
+}
+
+void AudioNodeWiring::Disable(AudioNodeOutput& output, AudioNodeInput& input) {
+  input.GetDeferredTaskHandler().AssertGraphOwner();
+
+  // These must be connected.
+  DCHECK(output.inputs_.Contains(&input));
+  DCHECK(input.outputs_.Contains(&output) ||
+         input.disabled_outputs_.Contains(&output));
+
+  // The output should have been marked as disabled.
+  DCHECK(!output.is_enabled_);
+
+  // Move from the active list to the disabled list.
+  // Do nothing if this is the current state.
+  if (!input.disabled_outputs_.insert(&output).is_new_entry)
+    return;
+  input.outputs_.erase(&output);
+
+  // Since it has lost an active connection, the input may need to have its
+  // rendering state updated.
+  input.ChangedOutputs();
+
+  // Propagate disabled state downstream. This must happen after the set
+  // manipulations above, or the disabling logic could observe an inconsistent
+  // state.
+  input.Handler().DisableOutputsIfNecessary();
+}
+
+void AudioNodeWiring::Enable(AudioNodeOutput& output, AudioNodeInput& input) {
+  input.GetDeferredTaskHandler().AssertGraphOwner();
+
+  // These must be connected.
+  DCHECK(output.inputs_.Contains(&input));
+  DCHECK(input.outputs_.Contains(&output) ||
+         input.disabled_outputs_.Contains(&output));
+
+  // The output should have been marked as enabled.
+  DCHECK(output.is_enabled_);
+
+  // Move from the disabled list to the active list.
+  // Do nothing if this is the current state.
+  if (!input.outputs_.insert(&output).is_new_entry)
+    return;
+  input.disabled_outputs_.erase(&output);
+
+  // Since it has gained an active connection, the input may need to have its
+  // rendering state updated.
+  input.ChangedOutputs();
+
+  // Propagate enabled state downstream. This must happen after the set
+  // manipulations above, or the disabling logic could observe an inconsistent
+  // state.
+  input.Handler().EnableOutputsIfNecessary();
+}
+
+bool AudioNodeWiring::IsConnected(AudioNodeOutput& output,
+                                  AudioNodeInput& input) {
+  input.GetDeferredTaskHandler().AssertGraphOwner();
+
+  bool is_connected = output.inputs_.Contains(&input);
+  DCHECK_EQ(is_connected, input.outputs_.Contains(&output) ||
+                              input.disabled_outputs_.Contains(&output));
+  return is_connected;
+}
+
+bool AudioNodeWiring::IsConnected(AudioNodeOutput& output,
+                                  AudioParamHandler& param) {
+  param.GetDeferredTaskHandler().AssertGraphOwner();
+
+  bool is_connected = output.params_.Contains(&param);
+  DCHECK_EQ(is_connected, param.outputs_.Contains(&output));
+  return is_connected;
+}
+
+void AudioNodeWiring::WillBeDestroyed(AudioNodeInput& input) {
+  // This is more or less a streamlined version of calling Disconnect
+  // repeatedly. In particular it cannot happen while the input's handler is
+  // being destroyed, and so does not require any information about these final
+  // changes to its connections.
+  //
+  // What does matter, however, is ensuring that no AudioNodeOutput holds a
+  // dangling pointer to |input|.
+
+  input.GetDeferredTaskHandler().AssertGraphOwner();
+
+  for (AudioNodeOutput* output : input.outputs_)
+    output->inputs_.erase(&input);
+  for (AudioNodeOutput* output : input.disabled_outputs_)
+    output->inputs_.erase(&input);
+  input.outputs_.clear();
+  input.disabled_outputs_.clear();
+}
+
+}  // namespace blink

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_AUDIO_NODE_WIRING_H_
+#define THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_AUDIO_NODE_WIRING_H_
+
+#include "third_party/blink/renderer/modules/modules_export.h"
+#include "third_party/blink/renderer/platform/wtf/allocator.h"
+
+namespace blink {
+
+class AudioNodeInput;
+class AudioNodeOutput;
+class AudioParamHandler;
+
+// Utilities for connecting AudioHandlers to one another, via AudioNodeInput and
+// AudioNodeOutput. Gathered into one place that can see the internals of both,
+// to avoid both needing to call one another back and forth.
+//
+// An AudioNodeOutput can also be connected to an AudioParamHandler.
+//
+// All functions require the graph lock.
+class MODULES_EXPORT AudioNodeWiring {
+  STATIC_ONLY(AudioNodeWiring);
+
+ public:
+  // Make or break a connection from an output of one audio node to an input of
+  // another.
+  static void Connect(AudioNodeOutput&, AudioNodeInput&);
+  static void Connect(AudioNodeOutput&, AudioParamHandler&);
+  static void Disconnect(AudioNodeOutput&, AudioNodeInput&);
+  static void Disconnect(AudioNodeOutput&, AudioParamHandler&);
+
+  // Disable the connection from an output to an input, setting it aside in
+  // a separate list of disabled connections. Enabling does the reverse.
+  // Should be called only from AudioNodeOutput, when its state has changed.
+  static void Disable(AudioNodeOutput&, AudioNodeInput&);
+  static void Enable(AudioNodeOutput&, AudioNodeInput&);
+
+  // Queries whether a connection exists, disabled or not.
+  static bool IsConnected(AudioNodeOutput&, AudioNodeInput&);
+  static bool IsConnected(AudioNodeOutput&, AudioParamHandler&);
+
+  // Called before complete destruction to remove any remaining connections.
+  static void WillBeDestroyed(AudioNodeInput&);
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_AUDIO_NODE_WIRING_H_

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

@@ -298,27 +298,6 @@ void AudioParamHandler::CalculateTimelineValues(float* values,
       sample_rate, sample_rate, MinValue(), MaxValue()));
 }
 
-void AudioParamHandler::Connect(AudioNodeOutput& output) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-
-  if (outputs_.Contains(&output))
-    return;
-
-  output.AddParam(*this);
-  outputs_.insert(&output);
-  ChangedOutputs();
-}
-
-void AudioParamHandler::Disconnect(AudioNodeOutput& output) {
-  GetDeferredTaskHandler().AssertGraphOwner();
-
-  if (outputs_.Contains(&output)) {
-    outputs_.erase(&output);
-    ChangedOutputs();
-    output.RemoveParam(*this);
-  }
-}
-
 int AudioParamHandler::ComputeQHistogramValue(float new_value) const {
   // For the Q value, assume a useful range is [0, 25] and that 0.25 dB
   // resolution is good enough.  Then, we can map the floating point Q value (in

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

@@ -92,6 +92,8 @@ enum AudioParamType {
 // processing classes have additional references. An AudioParamHandler can
 // outlive the owner AudioParam, and it never dies before the owner AudioParam
 // dies.
+//
+// Connected to AudioNodeOutput using AudioNodeWiring.
 class AudioParamHandler final : public ThreadSafeRefCounted<AudioParamHandler>,
                                 public AudioSummingJunction {
  public:
@@ -194,10 +196,6 @@ class AudioParamHandler final : public ThreadSafeRefCounted<AudioParamHandler>,
   // Must be called in the context's render thread.
   void CalculateSampleAccurateValues(float* values, unsigned number_of_values);
 
-  // Connect an audio-rate signal to control this parameter.
-  void Connect(AudioNodeOutput&);
-  void Disconnect(AudioNodeOutput&);
-
   float IntrinsicValue() const {
     return intrinsic_value_.load(std::memory_order_relaxed);
   }
@@ -254,6 +252,8 @@ class AudioParamHandler final : public ThreadSafeRefCounted<AudioParamHandler>,
 
   // Audio bus to sum in any connections to the AudioParam.
   scoped_refptr<AudioBus> summing_bus_;
+
+  friend class AudioNodeWiring;
 };
 
 // AudioParam class represents web-exposed AudioParam interface.

third_party/blink/web_tests/webaudio/internals/self-connection-gc.html

+<!DOCTYPE html>
+<script src="../../resources/gc.js"></script>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script>
+function soon() { return new Promise(resolve => setTimeout(resolve, 0)); }
+async function becomesTrue(predicate) {
+  while (!predicate()) await soon();
+}
+
+function createSelfConnectedNode(context) {
+  const gain = context.createGain();
+  gain.connect(gain);
+}
+
+promise_test(async () => {
+  await asyncGC();
+  await becomesTrue(() => internals.audioHandlerCount() == 0);
+
+  let context = new OfflineAudioContext(2, 1024, 44100);
+  let initialCount = internals.audioHandlerCount();
+  createSelfConnectedNode(context);
+  assert_greater_than(internals.audioHandlerCount(), 0);
+
+  // Need to render to clean up a cycle on an offline context.
+  await context.startRendering();
+
+  // Wait until the gain node's handler has been destroyed to declare victory.
+  await asyncGC();
+  await becomesTrue(() => internals.audioHandlerCount() <= initialCount);
+}, "No crash should occur when a node connected to itself is collected.");
+</script>