Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853923002:
Improve thread creation in plaform/audio/AudioDestination (Closed)
DescriptionImprove thread creation in plaform/audio/AudioDestination
After the introduction of the new rendering thread for WebAudio in
AudioDestination, two racy situnations were observed by ClusterFuzz.
These race conditions become critical especially when the AudioContext
is in the tear-down stage; when the main thread is dumping its member
variables, the rendering thread is still trying to access them.
This CL moves the thread creation logic into Start() and Stop() methods
in AudioDestination. By doing so, the thread is always be in sync with
the associated audio device and the thread can be safely deleted when
the AudioContext goes away.
BUG=716358, 716945
TEST=(The local TSAN/ASAN passed the repro test cases.)
Review-Url: https://codereview.chromium.org/2853923002
Cr-Commit-Position: refs/heads/master@{#468726}
Committed: https://chromium.googlesource.com/chromium/src/+/d94da1744907ed1bb90e37756806841609b0cc52
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing feedback #
Messages
Total messages: 25 (11 generated)
hongchan@chromium.org changed reviewers: + haraken@chromium.org, nhiroki@chromium.org, rtoy@chromium.org
PTAL. haraken@, nhiroki@: Handling of WebThread. rtoy@: WebAudio.
On 2017/05/01 22:32:11, hongchan wrote: > PTAL. > > haraken@, nhiroki@: Handling of WebThread. > > rtoy@: WebAudio. lgtm, but please file a bug about how changing the channel count of the context.destination should be changed not to stop, create, and start a new device.
On 2017/05/01 22:53:19, Raymond Toy wrote: > On 2017/05/01 22:32:11, hongchan wrote: > > PTAL. > > > > haraken@, nhiroki@: Handling of WebThread. > > > > rtoy@: WebAudio. > > lgtm, but please file a bug about how changing the channel count of the > context.destination should be changed not to stop, create, and start a new > device. Done: crbug.com/717285
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.
https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:178: if (web_audio_device_ && !is_playing_) { When is |web_audio_device_| null? It looks like it's created at the ctor and never nullified. https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:205: return is_playing_; To simplify state management, how about replacing |is_playing| with |rendering_thread_| and removing |is_playing_| from AudioDestination?
On 2017/05/02 02:08:06, nhiroki (ooo May 3-7) wrote: > https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): > > https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:178: if > (web_audio_device_ && !is_playing_) { > When is |web_audio_device_| null? It looks like it's created at the ctor and > never nullified. > > https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:205: return > is_playing_; > To simplify state management, how about replacing |is_playing| with > |rendering_thread_| and removing |is_playing_| from AudioDestination? nhiroki@ I should have clarified what happened since I uploaded https://codereview.chromium.org/2854463002/ - the new security bug 716945 popped up this morning, and its root cause was also the life cycle of the thread. Somehow the fix for this security issue also resolve the previous WebThread data race issue. Sorry if you're confused about how two CLs are related. I will close the previous CL (https://codereview.chromium.org/2854463002/) once this is submitted to CQ.
LGTM after the comments are addressed. On 2017/05/02 02:31:45, hongchan wrote: > On 2017/05/02 02:08:06, nhiroki (ooo May 3-7) wrote: > > > https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): > > > > > https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:178: if > > (web_audio_device_ && !is_playing_) { > > When is |web_audio_device_| null? It looks like it's created at the ctor and > > never nullified. > > > > > https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/audio/AudioDestination.cpp:205: return > > is_playing_; > > To simplify state management, how about replacing |is_playing| with > > |rendering_thread_| and removing |is_playing_| from AudioDestination? > > > nhiroki@ > > I should have clarified what happened since I uploaded > https://codereview.chromium.org/2854463002/ - the new security bug 716945 popped > up this morning, and its root cause was also the life cycle of the thread. > Somehow the fix for this security issue also resolve the previous WebThread data > race issue. > > Sorry if you're confused about how two CLs are related. I will close the > previous CL (https://codereview.chromium.org/2854463002/) once this is submitted > to CQ. Thank you for the clarification. I understand the situation :)
LGTM https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:175: DCHECK(WTF::IsMainThread()); WTF:: would not be needed. https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:92: // TODO(hongchan): this should not be by the rendering thread. be called https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:117: // Accessed by the device thread. Rendering thread for WebAudio graph. The rendering_thread_ is accessed by the main thread, right?
Thanks for the review, everyone! https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:175: DCHECK(WTF::IsMainThread()); On 2017/05/02 05:02:07, haraken wrote: > > WTF:: would not be needed. Done. https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:178: if (web_audio_device_ && !is_playing_) { On 2017/05/02 02:08:06, nhiroki (ooo May 3-7) wrote: > When is |web_audio_device_| null? It looks like it's created at the ctor and > never nullified. Hmm. That part I haven't questioned so far. Even if so, I don't think we should fix it in this CL. https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:205: return is_playing_; On 2017/05/02 02:08:06, nhiroki (ooo May 3-7) wrote: > To simplify state management, how about replacing |is_playing| with > |rendering_thread_| and removing |is_playing_| from AudioDestination? I agree that they are related, but I rather want to change the method name to be more precise/descriptive. Having an active thread is a bit different from "the destination is playing." I believe this CL should just focus on fixing data race and use-after-free issue. However, with the feedback in this CL, I should revisit this code and refactor it in the future. https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:92: // TODO(hongchan): this should not be by the rendering thread. On 2017/05/02 05:02:07, haraken wrote: > > be called Done. https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:117: // Accessed by the device thread. Rendering thread for WebAudio graph. On 2017/05/02 05:02:07, haraken wrote: > > The rendering_thread_ is accessed by the main thread, right? > Its life cycle is governed by the main thread, but it is mostly used (accessed) by the device thread.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, nhiroki@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2853923002/#ps20001 (title: "Addressing feedback")
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
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_...)
https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:205: return is_playing_; On 2017/05/02 16:49:41, hongchan wrote: > On 2017/05/02 02:08:06, nhiroki (ooo May 3-7) wrote: > > To simplify state management, how about replacing |is_playing| with > > |rendering_thread_| and removing |is_playing_| from AudioDestination? > > I agree that they are related, but I rather want to change the method name to be > more precise/descriptive. Having an active thread is a bit different from "the > destination is playing." > > I believe this CL should just focus on fixing data race and use-after-free > issue. > > However, with the feedback in this CL, I should revisit this code and refactor > it in the future. I agree. But please also file a bug that we should clean up this. Otherwise, I'm going to forget.
The CQ bit was checked by hongchan@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493748724726010,
"parent_rev": "76d6d9c5e3be62df7beb5f047e26b292997904de", "commit_rev":
"d94da1744907ed1bb90e37756806841609b0cc52"}
Message was sent while issue was closed.
Description was changed from ========== Improve thread creation in plaform/audio/AudioDestination After the introduction of the new rendering thread for WebAudio in AudioDestination, two racy situnations were observed by ClusterFuzz. These race conditions become critical especially when the AudioContext is in the tear-down stage; when the main thread is dumping its member variables, the rendering thread is still trying to access them. This CL moves the thread creation logic into Start() and Stop() methods in AudioDestination. By doing so, the thread is always be in sync with the associated audio device and the thread can be safely deleted when the AudioContext goes away. BUG=716358, 716945 TEST=(The local TSAN/ASAN passed the repro test cases.) ========== to ========== Improve thread creation in plaform/audio/AudioDestination After the introduction of the new rendering thread for WebAudio in AudioDestination, two racy situnations were observed by ClusterFuzz. These race conditions become critical especially when the AudioContext is in the tear-down stage; when the main thread is dumping its member variables, the rendering thread is still trying to access them. This CL moves the thread creation logic into Start() and Stop() methods in AudioDestination. By doing so, the thread is always be in sync with the associated audio device and the thread can be safely deleted when the AudioContext goes away. BUG=716358, 716945 TEST=(The local TSAN/ASAN passed the repro test cases.) Review-Url: https://codereview.chromium.org/2853923002 Cr-Commit-Position: refs/heads/master@{#468726} Committed: https://chromium.googlesource.com/chromium/src/+/d94da1744907ed1bb90e37756806... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d94da1744907ed1bb90e37756806...
Message was sent while issue was closed.
https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.h (right): https://codereview.chromium.org/2853923002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.h:117: // Accessed by the device thread. Rendering thread for WebAudio graph. On 2017/05/02 16:49:41, hongchan wrote: > On 2017/05/02 05:02:07, haraken wrote: > > > > The rendering_thread_ is accessed by the main thread, right? > > > > Its life cycle is governed by the main thread, but it is mostly used (accessed) > by the device thread. Makes sense. We could probably unify terms between 'device thread' and 'rendering thread'. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
