New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 783129 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

Data race in media::AudioOutputDevice::Initialize

Project Member Reported by ClusterFuzz, Nov 9 2017

Issue description

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)

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5571520444497920

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Components: Internals>Media
Cc: msrchandra@chromium.org pfeldman@chromium.org pnangunoori@chromium.org andrew.macpherson@soundtrap.com
Labels: M-63 Test-Predator-Wrong
Owner: rtoy@chromium.org
Status: Assigned (was: Untriaged)
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.

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?

Comment 4 by rtoy@chromium.org, Nov 10 2017

Cc: rtoy@chromium.org
Owner: guidou@chromium.org
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_.


Comment 5 by guidou@chromium.org, Nov 10 2017

Cc: guidou@chromium.org olka@chromium.org
Components: -Internals>Media Internals>Media>Audio
Owner: maxmorin@chromium.org
maxmorin@: Can you take a look?
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.
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.
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.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by ClusterFuzz, 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.
Project Member

Comment 11 by ClusterFuzz, Nov 18 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
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.
Cc: grunell@chromium.org
Henrik: I was thinking this should be merged to M63. Do you want to review the CL first?
Fine with me; I trust Olga and Dale. (If you want me to review, I can do that.)
Labels: Merge-Request-63
Shouldn't be needed. Requesting merge to M63: This race could lead to crashes, and the fix is quite simple.
Project Member

Comment 15 by sheriffbot@chromium.org, Nov 20 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
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
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows
Is the change listed at #9 well baked/verified in canary, having enough automation tests coverage and safe to merge to M63?

Yes.
Labels: -Merge-Review-63 Merge-Approved-63
Approving merge to M63 branch 3239 based on comments #14, #18 and per offline chat with maxmorin@. Please merge ASAP. Thank you.
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 20 2017

Labels: -merge-approved-63 merge-merged-3239
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