Fix data race in AudioDestination.cpp

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

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 49 matching lines @@
       new AudioDestination(callback, number_of_output_channels, latency_hint,
                            std::move(security_origin)));
 }
 
 AudioDestination::AudioDestination(AudioIOCallback& callback,
                                    unsigned number_of_output_channels,
                                    const WebAudioLatencyHint& latency_hint,
                                    PassRefPtr<SecurityOrigin> security_origin)
     : number_of_output_channels_(number_of_output_channels),
       is_playing_(false),
+      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.

-      fifo_(WTF::WrapUnique(
-          new PushPullFIFO(number_of_output_channels, kFIFOSize))),
       output_bus_(AudioBus::Create(number_of_output_channels,
                                    AudioUtilities::kRenderQuantumFrames,
                                    false)),
       render_bus_(AudioBus::Create(number_of_output_channels,
                                    AudioUtilities::kRenderQuantumFrames)),
       callback_(callback),
       frames_elapsed_(0) {
   // Create WebAudioDevice. blink::WebAudioDevice is designed to support the
   // local input (e.g. loopback from OS audio system), but Chromium's media
   // renderer does not support it currently. Thus, we use zero for the number
@@ skipping 30 matching lines @@
   if (!fifo_ || fifo_->length() < number_of_frames)
     return;
 
   // Associate the destination data array with the output bus then fill the
   // FIFO.
   for (unsigned i = 0; i < number_of_output_channels_; ++i)
     output_bus_->SetChannelMemory(i, destination_data[i], number_of_frames);
 
   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,
                                                 size_t frames_to_render,
                                                 double delay,
                                                 double delay_timestamp,
                                                 size_t prior_frames_skipped) {
   // This method is called by WebThread.
   DCHECK(IsRenderingThread());
 
@@ skipping 23 matching lines @@
 
     // Process WebAudio graph and push the rendered output to FIFO.
     callback_.Render(nullptr, render_bus_.Get(),
                      AudioUtilities::kRenderQuantumFrames, output_position);
     fifo_->Push(render_bus_.Get());
   }
 
   frames_elapsed_ += frames_requested;
 }
 
 void AudioDestination::Start() {

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

Can we have DCHECK(WTF::IsMainThread()) here because of |web_audio_device_|
access?

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

Done.

   if (web_audio_device_ && !is_playing_) {
     web_audio_device_->Start();
     is_playing_ = true;
   }
 }
 
 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.

   }
 }
 
 size_t AudioDestination::HardwareBufferSize() {
   return Platform::Current()->AudioHardwareBufferSize();
 }
 
 float AudioDestination::HardwareSampleRate() {
   return static_cast<float>(Platform::Current()->AudioHardwareSampleRate());
 }
@@ skipping 24 matching lines @@
   DCHECK(is_buffer_size_valid);
   return is_buffer_size_valid;
 }
 
 bool AudioDestination::IsRenderingThread() {
   return static_cast<ThreadIdentifier>(rendering_thread_->ThreadId()) ==
          CurrentThread();
 }
 
 }  // namespace blink