Lock-order-inversion in blink::HTMLMediaElement::AudioSourceProviderImpl::SetClient |
||||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5375725703266304 Fuzzer: inferno_layout_test_unmodified Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Lock-order-inversion Crash Address: Crash State: blink::HTMLMediaElement::AudioSourceProviderImpl::SetClient blink::HTMLMediaElement::SetAudioSourceNode blink::MediaElementAudioSourceNode::Create Sanitizer: thread (TSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=514498:517698 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5375725703266304 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Nov 21 2017
Hi, With joelhockey@ help we managed to reproduce the error locally and we also tried reverting to older versions like c2619827dfb9573977792844b408c6ffd7e5c565 from Oct/13th and the test still fails. Thus this isn't related to my change on this file. I'm CCing some recent reviewers and authors to see if they have any idea. Is it possible that this fuzz test is new but the potential dead lock has been there for a while? Assigning to Kentaro for triaging if there is somebody more appropriate to own this bug.
,
Nov 21 2017
hongchan: Would you mind taking a look at this?
,
Nov 21 2017
Issue 460843 has been merged into this issue.
,
Nov 21 2017
Issue 527569 has been merged into this issue.
,
Nov 21 2017
This looks familiar. I thought it was fixed a while ago. The issue 460843 suggests CF seems having a hard time to reproduce this reliably. There are several locks in the interaction between HTMLMediaElement and MediaElementAudioSourceNode. 3 mutex in dead lock: M916336529641580072 => M675679257743472360 => M9987018733996360 => M916336529641580072 M916336529641580072: HTMLMediaElement::AudioSourceProviderImpl::SetClient https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp?l=4064 M675679257743472360: HTMLMediaElement::SetAudioSourceNode https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp?l=3805 M9987018733996360: WebAudioSourceProviderImpl::SetClient https://cs.chromium.org/chromium/src/media/blink/webaudiosourceprovider_impl.cc?l=122 I will investigate further. Since this is not a security bug, I will lower the prioirity to P2.
,
Jan 19 2018
Automatically applying components based on crash stacktrace and information from OWNERS files. If this is incorrect, please apply the Test-Predator-Wrong-Components label.
,
Jan 19 2018
,
Jan 19 2018
I believe I found the cause: [1] HTMLMediaElement::SetAudioSourceNode() locks MediaElementAudioSourceNode, but within that call [2] MediaElementAudioSourceNode::SetFormat() gets called as well. SetFormat() also claims the lock of MediaElementAudioSourceNode, which is held by [1] HTMLMediaElement at the moment. It seems like an obvious flaw and I am not really sure why it does not happen more often. Perhaps we are really lucky, or this code path does not get used very much? After the investigation of call path, I think MediaElementAudioSourceNode::SetFormat() is only triggered by HTMLMediaElement::SetAudioSourceNode(), thus we can safely remove the lock in MediaElementAudioSourceNode::SetFormat().
,
Jan 19 2018
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9108b5c0f8df4cea7e2e26fc6dff05eb39c12e56 commit 9108b5c0f8df4cea7e2e26fc6dff05eb39c12e56 Author: Hongchan Choi <hongchan@chromium.org> Date: Tue Jan 30 06:00:57 2018 Resolve lock-order-inversion in HTMLMediaElement and MediaElementAudioSourceNode This CL resolves wrongly placed locks at 3 different objects. * Observation Mutex M6756: HTMLMediaElement::provide_input_lock (Mutex) Mutex M9163: MediaElementAudioSourceHandler::process_lock_ (Mutex) Mutex M9987: WebAudioSourceProviderImpl::sink_lock_ (base::Lock) These 3 locks have a cyclic reference: M9163 => M6756 => M9987 => M9163 * Scenario 1. Entry point: MediaElementAudioSourceNode::Create() from BaseAudioContext's factory method 2. HTMLMediaElement::SetAudioSourceNode() M9163 locked 3. HTMLMediaElement::AudioSourceProviderImpl::SetClient() M6756 locked 4. WebAudioSourceProviderImpl::SetClient() M9987 locked 5. HTMLMediaElement::AudioClientImpl::SetFormat() 6. MediaElementAudioSourceNode::SetFormat() 7. MediaElementAudioSourceHandler::SetFormat() Acquiring M9163 fails because it is already held at step 2. * Attempted Solution in this CL HTMLMediaElement::SetAudioSourceNode() does not have to lock the MediaElementAudioSource node - because the source node and the handlers are locked locally in SetFormat() method. * Suppressed Error The patch fixes the lock-order-inversion, but it shows another TSAN suppression case: --- ThreadSanitizer: Matched 3 suppressions (pid=85351): 1 race:tzset_internal 2 deadlock:dbus::Bus::ShutdownAndBlock --- Note that this error is labled as a |won't fix|: https://cs.chromium.org/chromium/src/build/sanitizers/tsan_suppressions.cc?l=220 case instead. Bug: 786739 Test: CF repro tool cannot reproduce, showing one of TSAN suppression Change-Id: I8d9f6fe549562e39f7212f80aa2dea6c9527922e Reviewed-on: https://chromium-review.googlesource.com/879242 Reviewed-by: Raymond Toy <rtoy@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Mounir Lamouri (slow) <mlamouri@chromium.org> Commit-Queue: Hongchan Choi <hongchan@chromium.org> Cr-Commit-Position: refs/heads/master@{#532787} [modify] https://crrev.com/9108b5c0f8df4cea7e2e26fc6dff05eb39c12e56/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp
,
Jan 30 2018
ClusterFuzz has detected this issue as fixed in range 532784:532791. Detailed report: https://clusterfuzz.com/testcase?key=5375725703266304 Fuzzer: inferno_layout_test_unmodified Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Lock-order-inversion Crash Address: Crash State: blink::HTMLMediaElement::AudioSourceProviderImpl::SetClient blink::HTMLMediaElement::SetAudioSourceNode blink::MediaElementAudioSourceNode::Create Sanitizer: thread (TSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=514498:517698 Fixed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=532784:532791 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5375725703266304 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.
,
Jan 30 2018
ClusterFuzz testcase 5375725703266304 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by pnangunoori@chromium.org
, Nov 20 2017Labels: M-64 Test-Predator-Wrong
Owner: lucmult@chromium.org
Status: Assigned (was: Untriaged)