Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854463002:
Fix data race in AudioDestination.cpp (Closed)
DescriptionFix data race in AudioDestination.cpp
** This CL is replaced by https://codereview.chromium.org/2853923002/ **
This CL fixes the data race in AudioDestination.cpp, which is caused by
the change in the WebAudio rendering architecture.
The newly introduced WebThread is scheduled by the device thread, and
the WebThread still tries to access the WebAudio graph even after the
AudioContext starts the tear-down process.
The fix is to call .reset() on the WebThread when the AudioDestination
is shutting down.
BUG=716358
TEST=(ran the local TSAN for 30 minutes and no data race reported.)
Patch Set 1 #
Total comments: 23
Patch Set 2 : Adding DCHECK(IsMainThread()) in getters #Patch Set 3 : SampleRate() fix after l-g-t-m #
Total comments: 2
Messages
Total messages: 21 (14 generated)
hongchan@chromium.org changed reviewers: + nhiroki@chromium.org, rtoy@chromium.org
PTAL.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:73: Platform::Current()->CreateThread("WebAudio Rendering Thread"))), Why change the order? https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: // TODO(hongchan): this check might be redundant, so consider removing later. Which checks are redundant? Both? Is it possible for Pull() to return 0? And how can rendering_thread_ not be set here? https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:193: rendering_thread_.reset(); Is this synchronous? Are we guaranteed the thread is stopped when reset() returns?
https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:73: Platform::Current()->CreateThread("WebAudio Rendering Thread"))), On 2017/04/28 19:53:56, Raymond Toy wrote: > Why change the order? I rearranged the class member by the thread type. Thus, the order of initializers also should be changed. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: // TODO(hongchan): this check might be redundant, so consider removing later. On 2017/04/28 19:53:56, Raymond Toy wrote: > Which checks are redundant? Both? Is it possible for Pull() to return 0? And Yes, theoretically it's possible when there is enough data to cover all the requested frames from the device. > how can rendering_thread_ not be set here? Perhaps I should use rendering_thread_.get()? https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:193: rendering_thread_.reset(); On 2017/04/28 19:53:56, Raymond Toy wrote: > Is this synchronous? Are we guaranteed the thread is stopped when reset() > returns? 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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: // TODO(hongchan): this check might be redundant, so consider removing later. On 2017/04/28 20:24:54, hongchan wrote: > On 2017/04/28 19:53:56, Raymond Toy wrote: > > Which checks are redundant? Both? Is it possible for Pull() to return 0? And > > Yes, theoretically it's possible when there is enough data to cover all the > requested frames from the device. > > > how can rendering_thread_ not be set here? > > Perhaps I should use rendering_thread_.get()? If |rendering_thread_| is nullptr here, DCHECK at line 106 should crash. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:176: void AudioDestination::Start() { Can we have DCHECK(WTF::IsMainThread()) here because of |web_audio_device_| access? https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:183: void AudioDestination::Stop() { ditto. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:193: rendering_thread_.reset(); On 2017/04/28 20:24:54, hongchan wrote: > On 2017/04/28 19:53:56, Raymond Toy wrote: > > Is this synchronous? Are we guaranteed the thread is stopped when reset() > > returns? > > 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? I'm not 100% sure but I think this is safe. Worker does the same thing here: https://chromium.googlesource.com/chromium/src/+/e578e69e97678337cd294fafc9d0... https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:88: size_t CallbackBufferSize() const { return callback_buffer_size_; } |callback_buffer_size_| must be accessed by the main thread. Can we have a comment about it and/or a thread check (i.e., DCHECK(WTF::IsMainThread()))? https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:89: bool IsPlaying() { return is_playing_; } ditto. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:91: double SampleRate() const { return web_audio_device_->SampleRate(); } ditto. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:95: int FramesPerBuffer() const { return web_audio_device_->FramesPerBuffer(); } ditto.
https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: // TODO(hongchan): this check might be redundant, so consider removing later. On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > On 2017/04/28 20:24:54, hongchan wrote: > > On 2017/04/28 19:53:56, Raymond Toy wrote: > > > Which checks are redundant? Both? Is it possible for Pull() to return 0? > And > > > > Yes, theoretically it's possible when there is enough data to cover all the > > requested frames from the device. > > > > > how can rendering_thread_ not be set here? > > > > Perhaps I should use rendering_thread_.get()? > > If |rendering_thread_| is nullptr here, DCHECK at line 106 should crash. True, but it cannot be caught in the release build. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:176: void AudioDestination::Start() { On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > Can we have DCHECK(WTF::IsMainThread()) here because of |web_audio_device_| > access? Done. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:183: void AudioDestination::Stop() { On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > ditto. Done. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:193: rendering_thread_.reset(); On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > On 2017/04/28 20:24:54, hongchan wrote: > > On 2017/04/28 19:53:56, Raymond Toy wrote: > > > Is this synchronous? Are we guaranteed the thread is stopped when reset() > > > returns? > > > > 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? > > I'm not 100% sure but I think this is safe. Worker does the same thing here: > https://chromium.googlesource.com/chromium/src/+/e578e69e97678337cd294fafc9d0... Yes, that's my reference for this fix. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:88: size_t CallbackBufferSize() const { return callback_buffer_size_; } On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > |callback_buffer_size_| must be accessed by the main thread. Can we have a > comment about it and/or a thread check (i.e., DCHECK(WTF::IsMainThread()))? Done. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:89: bool IsPlaying() { return is_playing_; } On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > ditto. Done. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:91: double SampleRate() const { return web_audio_device_->SampleRate(); } On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > ditto. Done. https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:95: int FramesPerBuffer() const { return web_audio_device_->FramesPerBuffer(); } On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > ditto. Done.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2854463002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:124: // TODO(hongchan): this check might be redundant, so consider removing later. On 2017/05/01 16:43:08, hongchan wrote: > On 2017/05/01 01:56:39, nhiroki (ooo May 3-7) wrote: > > On 2017/04/28 20:24:54, hongchan wrote: > > > On 2017/04/28 19:53:56, Raymond Toy wrote: > > > > Which checks are redundant? Both? Is it possible for Pull() to return 0? > > And > > > > > > Yes, theoretically it's possible when there is enough data to cover all the > > > requested frames from the device. > > > > > > > how can rendering_thread_ not be set here? > > > > > > Perhaps I should use rendering_thread_.get()? > > > > If |rendering_thread_| is nullptr here, DCHECK at line 106 should crash. > > True, but it cannot be caught in the release build. 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()); https://codereview.chromium.org/2854463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2854463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:70: fifo_(WTF::WrapUnique( MakeUnique? https://codereview.chromium.org/2854463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2854463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.h:90: bool IsPlaying(); bool IsPlaying() const; ?
Description was changed from ========== Fix data race in AudioDestination.cpp This CL fixes the data race in AudioDestination.cpp, which is caused by the change in the WebAudio rendering architecture. The newly introduced WebThread is scheduled by the device thread, and the WebThread still tries to access the WebAudio graph even after the AudioContext starts the tear-down process. The fix is to call .reset() on the WebThread when the AudioDestination is shutting down. BUG=716358 TEST=(ran the local TSAN for 30 minutes and no data race reported.) ========== to ========== Fix data race in AudioDestination.cpp ** This CL is replaced by https://codereview.chromium.org/2853923002/ ** This CL fixes the data race in AudioDestination.cpp, which is caused by the change in the WebAudio rendering architecture. The newly introduced WebThread is scheduled by the device thread, and the WebThread still tries to access the WebAudio graph even after the AudioContext starts the tear-down process. The fix is to call .reset() on the WebThread when the AudioDestination is shutting down. BUG=716358 TEST=(ran the local TSAN for 30 minutes and no data race reported.) ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
