Data race in media::AudioOutputDevice::Initialize |
|||||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5571520444497920 Fuzzer: inferno_twister_c Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Data race WRITE 8 Crash Address: 0x7b4c00004128 Crash State: media::AudioOutputDevice::Initialize content::RendererWebAudioDeviceImpl::Start blink::AudioDestination::Start Sanitizer: thread (TSAN) Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5571520444497920 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Nov 10 2017
Predator and CL could not provide any possible suspects. Using the code search for the file, “renderer_webaudiodevice_impl.cc” assigning to concern owner from GIT blame. Suspecting Commit# https://chromium.googlesource.com/chromium/src/+/4f85155e4b7ae25e1dfb21cf686890fb8634eb6f @rtoy -- Could you please look into this issue. Assigning it to you as issue as you were the part of reviewer and as issue can not be assigned to the actual suspect. Note: CC'd the owner of the commit. Thank You.
,
Nov 10 2017
Hello! I'd be happy to take a look but would need access to the test case, the error I get when trying to access it now says that I need to be Owner on the bug though. Any tips?
,
Nov 10 2017
Clusterfuzz says, in part: WARNING: ThreadSanitizer: data race (pid=17437) Write of size 8 at 0x7b4c00006268 by main thread: #0 0x7fc4bc1ba741 in #media::AudioOutputDevice::Initialize(media::AudioParameters const&, #media::AudioRendererSink::RenderCallback*) #media/audio/audio_output_device.cc:94:13 #1 0x7fc4c496cd11 in content::RendererWebAudioDeviceImpl::Start() #content/renderer/media/renderer_webaudiodevice_impl.cc:175:10 Previous read of size 8 at 0x7b4c00006268 by thread T6: #0 0x7fc4bc1bbe2f in media::AudioOutputDevice::OnDeviceAuthorized(media::OutputDeviceStatus, media::AudioParameters const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) media/audio/audio_output_device.cc:380:9 #1 0x7fc4bc1bbf15 in non-virtual thunk to #media::AudioOutputDevice::OnDeviceAuthorized(media::OutputDeviceStatus, #media::AudioParameters const&, std::__1::basic_string<char, #std::__1::char_traits<char>, std::__1::allocator<char> > const&) #media/audio/audio_output_device.cc So the main thread is setting callback_ while thread T6 is reading callback_. Reassigning to quidou@ who last touched OnDeviceAuthorized which is trying to read callback_.
,
Nov 10 2017
maxmorin@: Can you take a look?
,
Nov 10 2017
Since this bug appeared now, it's probably related to switching stream control messages to mojo IPC (which I made default on the bots recently in https://chromium-review.googlesource.com/c/chromium/src/+/741717). I'll have a look.
,
Nov 10 2017
Ah, |callback_| is used on both threads. This is completely broken. I guess the reason that we never saw this before was that it's only set once, in Initialize, but with traditional IPC we could never get an authorization response fast enough to find this race with TSAN. The mojo code fails fast if a stream is requested for an invalid frame, or the frame is destructed while authorization is in progress. This allows for OnDeviceAuthorized to be called immediately.
,
Nov 10 2017
The minimized test case seems to do exactly this: create an AudioContext in a frame and quickly remove the frame:
> P ] : / p k p 1 [ h 7 O ` f / M U 5<script type=text/javascript>
var iframe = document.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
document.body.appendChild(iframe);
var frameWin = iframe.contentWindow;
new frameWin.AudioContext();
document.body.removeChild(iframe);
new frameWin.AudioContext();
</script>
I'm really impressed by ClusterFuzz :).
To actually fix the bug, I think we could post the Initialize the call to the IO thread as well.
,
Nov 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/237ad913ef44e075240ead90fea4cc2731d45cae commit 237ad913ef44e075240ead90fea4cc2731d45cae Author: Max Morin <maxmorin@chromium.org> Date: Wed Nov 15 08:27:58 2017 Serialize member access in Audio{In,Out}putDevice. Some members are accessed on both the owning thread and the IO thread. A caveat: it's now more likely that authorization finishes before |callback_| is set, meaning that authorization failure won't signal an error. An error will still be signaled when trying to start the stream, so it should be fine. Bug: 783129 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ibf90bf60632b9d2893b1e80852dbfaf01fa717a1 Reviewed-on: https://chromium-review.googlesource.com/765451 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#516636} [modify] https://crrev.com/237ad913ef44e075240ead90fea4cc2731d45cae/media/audio/audio_input_device.cc [modify] https://crrev.com/237ad913ef44e075240ead90fea4cc2731d45cae/media/audio/audio_input_device.h [modify] https://crrev.com/237ad913ef44e075240ead90fea4cc2731d45cae/media/audio/audio_output_device.cc [modify] https://crrev.com/237ad913ef44e075240ead90fea4cc2731d45cae/media/audio/audio_output_device.h
,
Nov 18 2017
ClusterFuzz has detected this issue as fixed in range 514498:517698. Detailed report: https://clusterfuzz.com/testcase?key=5571520444497920 Fuzzer: inferno_twister_c Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Data race WRITE 8 Crash Address: 0x7b4c00004128 Crash State: media::AudioOutputDevice::Initialize content::RendererWebAudioDeviceImpl::Start blink::AudioDestination::Start Sanitizer: thread (TSAN) Fixed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=514498:517698 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5571520444497920 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Nov 18 2017
ClusterFuzz testcase 5571520444497920 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Nov 20 2017
Henrik: I was thinking this should be merged to M63. Do you want to review the CL first?
,
Nov 20 2017
Fine with me; I trust Olga and Dale. (If you want me to review, I can do that.)
,
Nov 20 2017
Shouldn't be needed. Requesting merge to M63: This race could lead to crashes, and the fix is quite simple.
,
Nov 20 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 20 2017
,
Nov 20 2017
Is the change listed at #9 well baked/verified in canary, having enough automation tests coverage and safe to merge to M63?
,
Nov 20 2017
Yes.
,
Nov 20 2017
Approving merge to M63 branch 3239 based on comments #14, #18 and per offline chat with maxmorin@. Please merge ASAP. Thank you.
,
Nov 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/944b993374f7015dffa293e493bef21ec103ba37 commit 944b993374f7015dffa293e493bef21ec103ba37 Author: Max Morin <maxmorin@chromium.org> Date: Mon Nov 20 16:23:07 2017 [M63] Serialize member access in Audio{In,Out}putDevice. Some members are accessed on both the owning thread and the IO thread. A caveat: it's now more likely that authorization finishes before |callback_| is set, meaning that authorization failure won't signal an error. An error will still be signaled when trying to start the stream, so it should be fine. Bug: 783129 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ibf90bf60632b9d2893b1e80852dbfaf01fa717a1 Reviewed-on: https://chromium-review.googlesource.com/765451 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#516636}(cherry picked from commit 237ad913ef44e075240ead90fea4cc2731d45cae) TBR: dalecurtis@chromium.org Change-Id: Ibf90bf60632b9d2893b1e80852dbfaf01fa717a1 Reviewed-on: https://chromium-review.googlesource.com/779579 Reviewed-by: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#541} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/944b993374f7015dffa293e493bef21ec103ba37/media/audio/audio_input_device.cc [modify] https://crrev.com/944b993374f7015dffa293e493bef21ec103ba37/media/audio/audio_input_device.h [modify] https://crrev.com/944b993374f7015dffa293e493bef21ec103ba37/media/audio/audio_output_device.cc [modify] https://crrev.com/944b993374f7015dffa293e493bef21ec103ba37/media/audio/audio_output_device.h |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by dtapu...@chromium.org
, Nov 10 2017