Move value setter warning code completely to AudioParamTimeline

Move the implemetation completely to the AudioParamTimeline class so
we can lock the event list completely to prevent the audio thread from
mutating the event list.

The original implementation had two function calls: one to find an
event index and another to print the warning.  However, the audio thread
could have mutated the event list between these calls, so the event
index could be invalid.

Bug: 778927
Test: Repro case doesn't fail DCHECK in debug build
Change-Id: Ied1d8d5128a316aba3c8d59589e1f305a5a2a26b
Reviewed-on: https://chromium-review.googlesource.com/741450
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513163}

third_party/WebKit/Source/modules/webaudio/AudioParam.cpp

@@ -363,26 +363,12 @@ void AudioParam::setValue(float value) {
 
 // TODO(crbug.com/764396): Remove this when fixed.
 void AudioParam::WarnIfSetterOverlapsEvent() {
-  // Find if there's an event at the current time.
-  bool has_overlap;
-  size_t current_event_index;
-
-  std::tie(has_overlap, current_event_index) =
-      Handler().Timeline().EventAtFrame(Context()->CurrentSampleFrame(),
-                                        Context()->sampleRate());
-
-  Context()->CountValueSetterConflict(has_overlap);
-
-  // Print a depecation message once, and also a more detailed message
-  // about the conflict so the developer knows.
-  if (!s_value_setter_warning_done_) {
-    Deprecation::CountDeprecation(Context()->GetExecutionContext(),
-                                  WebFeature::kWebAudioValueSetterIsSetValue);
-    if (has_overlap) {
-      Handler().Timeline().WarnSetterOverlapsEvent(
-          Handler().GetParamName(), current_event_index, *Context());
-    }
-  }
+  DCHECK(IsMainThread());
+
+  // Check for overlap and print a warning only if we haven't already
+  // printed a warning.
+  Handler().Timeline().WarnIfSetterOverlapsEvent(
+      Context(), Handler().GetParamName(), !s_value_setter_warning_done_);
 }
 
 float AudioParam::defaultValue() const {

third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp

@@ -30,6 +30,7 @@
 #include "bindings/core/v8/ExceptionState.h"
 #include "build/build_config.h"
 #include "core/dom/ExceptionCode.h"
+#include "core/frame/Deprecation.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "platform/audio/AudioUtilities.h"
 #include "platform/wtf/CPU.h"
@@ -1784,10 +1785,39 @@ unsigned AudioParamTimeline::FillWithDefault(float* values,
 }
 
 // TODO(crbug.com/764396): Remove this when fixed.
+bool AudioParamTimeline::WarnIfSetterOverlapsEvent(BaseAudioContext* context,
+                                                   String param_name,
+                                                   bool print_warning) {
+  DCHECK(IsMainThread());
+
+  // Don't let the audio thread mutate the event list while we're
+  // examining the event list.
+  MutexLocker locker(events_lock_);
+
+  // Find if there's an event at the current time.
+  bool has_overlap;
+  size_t current_event_index;
+
+  std::tie(has_overlap, current_event_index) =
+      EventAtFrame(context->CurrentSampleFrame(), context->sampleRate());
+
+  context->CountValueSetterConflict(has_overlap);
+
+  // Print a depecation message once, and also a more detailed message
+  // about the conflict so the developer knows.
+  Deprecation::CountDeprecation(context->GetExecutionContext(),
+                                WebFeature::kWebAudioValueSetterIsSetValue);
+  if (print_warning && has_overlap) {
+    WarnSetterOverlapsEvent(param_name, current_event_index, *context);
+    return true;
+  }
+
+  return false;
+}
+
 std::tuple<bool, size_t> AudioParamTimeline::EventAtFrame(
     size_t current_frame,
     float sample_rate) const {
-  MutexLocker locker(events_lock_);
 
   size_t number_of_events = events_.size();
   ParamEvent* event = nullptr;
@@ -1867,7 +1897,6 @@ void AudioParamTimeline::WarnSetterOverlapsEvent(
     String param_name,
     size_t event_index,
     BaseAudioContext& context) const {
-  MutexLocker locker(events_lock_);
 
   DCHECK_LT(event_index, events_.size());
 

third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h

@@ -99,22 +99,13 @@ class AudioParamTimeline {
   float SmoothedValue() { return smoothed_value_; }
   void SetSmoothedValue(float v) { smoothed_value_ = v; }
 
-  // TODO(crbug.com/764396): Remove these two methods when the bug is
-  // fixed.
+  // TODO(crbug.com/764396): Remove this when the bug is fixed.
 
-  // |EventAtFrame| finds the current event that would run at the specified
-  // |frame|. The first return value is true if a setValueAtTime call would
-  // overlap some ongoing event.  The second return value is the index of the
-  // current event. The second value must be ignored if the first value is
-  // false.
-  std::tuple<bool, size_t> EventAtFrame(size_t frame, float sample_rate) const;
-
-  // Prints a console warning that a call to the AudioParam value setter
-  // overlaps the event at |event_index|.  |param_name| is the name of the
-  // AudioParam where the where this is happening.
-  void WarnSetterOverlapsEvent(String param_name,
-                               size_t event_index,
-                               BaseAudioContext&) const;
+  // Print a warning if the value setter overlaps an event.  Returns
+  // true if a warning was printed.
+  bool WarnIfSetterOverlapsEvent(BaseAudioContext*,
+                                 String param_name,
+                                 bool print_warning);
 
  private:
   class ParamEvent {
@@ -461,6 +452,22 @@ class AudioParamTimeline {
                            size_t end_frame,
                            unsigned write_index);
 
+  // TODO(crbug.com/764396): Remove these two methods when the bug is fixed.
+
+  // |EventAtFrame| finds the current event that would run at the specified
+  // |frame|. The first return value is true if a setValueAtTime call would
+  // overlap some ongoing event.  The second return value is the index of the
+  // current event. The second value must be ignored if the first value is
+  // false.
+  std::tuple<bool, size_t> EventAtFrame(size_t frame, float sample_rate) const;
+
+  // Prints a console warning that a call to the AudioParam value setter
+  // overlaps the event at |event_index|.  |param_name| is the name of the
+  // AudioParam where the where this is happening.
+  void WarnSetterOverlapsEvent(String param_name,
+                               size_t event_index,
+                               BaseAudioContext&) const;
+
   // Vector of all automation events for the AudioParam.  Access must
   // be locked via m_eventsLock.
   Vector<std::unique_ptr<ParamEvent>> events_;