Fix data race in AudioDestination.cpp

third_party/WebKit/Source/platform/audio/AudioDestination.cpp

Index: third_party/WebKit/Source/platform/audio/AudioDestination.cpp
diff --git a/third_party/WebKit/Source/platform/audio/AudioDestination.cpp b/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
index e71d5ec1d0382f3196048e39545da9686cd543b4..5565991515f91007a2f9bcb0d5fe7c8af30b4860 100644
--- a/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
+++ b/third_party/WebKit/Source/platform/audio/AudioDestination.cpp
@@ -67,10 +67,10 @@ AudioDestination::AudioDestination(AudioIOCallback& callback,
                                    PassRefPtr<SecurityOrigin> security_origin)
     : number_of_output_channels_(number_of_output_channels),
       is_playing_(false),
-      rendering_thread_(WTF::WrapUnique(
-          Platform::Current()->CreateThread("WebAudio Rendering Thread"))),
       fifo_(WTF::WrapUnique(
           new PushPullFIFO(number_of_output_channels, kFIFOSize))),
+      rendering_thread_(WTF::WrapUnique(
+          Platform::Current()->CreateThread("WebAudio Rendering Thread"))),

[Raymond Toy, 2017/04/28 19:53:56]

Why change the order?

[hongchan, 2017/04/28 20:24:54]

I rearranged the class member by the thread type. Thus, the order of
initializers also should be changed.

       output_bus_(AudioBus::Create(number_of_output_channels,
                                    AudioUtilities::kRenderQuantumFrames,
                                    false)),
@@ -121,12 +121,15 @@ void AudioDestination::Render(const WebVector<float*>& destination_data,
 
   size_t frames_to_render = fifo_->Pull(output_bus_.Get(), number_of_frames);
 
-  rendering_thread_->GetWebTaskRunner()->PostTask(
-      BLINK_FROM_HERE,
-      CrossThreadBind(&AudioDestination::RequestRenderOnWebThread,
-                      CrossThreadUnretained(this),
-                      number_of_frames, frames_to_render,
-                      delay, delay_timestamp, prior_frames_skipped));
+  // TODO(hongchan): this check might be redundant, so consider removing later.

[Raymond Toy, 2017/04/28 19:53:56]

Which checks are redundant? Both?  Is it possible for Pull() to return 0?  And
how can rendering_thread_ not be set here?

[hongchan, 2017/04/28 20:24:54]

Yes, theoretically it's possible when there is enough data to cover all the
requested frames from the device.
Perhaps I should use rendering_thread_.get()?

[nhiroki, 2017/05/01 01:56:39]

If |rendering_thread_| is nullptr here, DCHECK at line 106 should crash.

[hongchan, 2017/05/01 16:43:08]

True, but it cannot be caught in the release build.

[nhiroki, 2017/05/02 01:42:51]

If it's valid to call Render() when |render_thread_| is null, we should avoid
the crash even on the debug build. How about checking the validity of the render
thread at the beginning of Render() like this?

if (!rendering_thread_)
  return;
// This method is called by AudioDeviceThread.
DCHECK(!IsRenderingThread());

+  if (frames_to_render > 0 && rendering_thread_) {
+    rendering_thread_->GetWebTaskRunner()->PostTask(
+        BLINK_FROM_HERE,
+        CrossThreadBind(&AudioDestination::RequestRenderOnWebThread,
+                        CrossThreadUnretained(this), number_of_frames,
+                        frames_to_render, delay, delay_timestamp,
+                        prior_frames_skipped));
+  }
 }
 
 void AudioDestination::RequestRenderOnWebThread(size_t frames_requested,
@@ -179,8 +182,15 @@ void AudioDestination::Start() {
 
 void AudioDestination::Stop() {

[nhiroki, 2017/05/01 01:56:39]

ditto.

[hongchan, 2017/05/01 16:43:08]

Done.

   if (web_audio_device_ && is_playing_) {
+    // This stops the callback from the device thread synchronously.
     web_audio_device_->Stop();
+
     is_playing_ = false;
+
+    // Disable the WebAudio rendering thread. This is safe because the device
+    // thread is stopped at this point and |rendering_thread_| will not be
+    // accessed by it anymore.
+    rendering_thread_.reset();

[Raymond Toy, 2017/04/28 19:53:56]

Is this synchronous? Are we guaranteed the thread is stopped when reset()
returns?

[hongchan, 2017/04/28 20:24:54]

I believe so. FWIW, this is c++ unique_ptr's method.

But is this perfectly thread-safe in Blink? I think our thread/GC experts only
can answer that. nhiroki@ WDYT?

[nhiroki, 2017/05/01 01:56:39]

I'm not 100% sure but I think this is safe. Worker does the same thing here:
https://chromium.googlesource.com/chromium/src/+/e578e69e97678337cd294fafc9d0...

[hongchan, 2017/05/01 16:43:07]

Yes, that's my reference for this fix.

   }
 }