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

Issue 786739 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Lock-order-inversion in blink::HTMLMediaElement::AudioSourceProviderImpl::SetClient

Project Member Reported by ClusterFuzz, Nov 18 2017

Issue description

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

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

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: msrchandra@chromium.org pnangunoori@chromium.org
Labels: M-64 Test-Predator-Wrong
Owner: lucmult@chromium.org
Status: Assigned (was: Untriaged)
Using the provided regression range assigning to the possible suspect as per the change made for the file, “MediaElementAudioSourceNode.cpp”
Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/0908d7c68c20dc3614dcfcf75a68c13286b16421

@lucmult  -- Could you please look into this issue, kindly reassign if it has nothing to do with your changes.

Cc: hajimehoshi@chromium.org rtoy@chromium.org lucmult@chromium.org haraken@chromium.org
Owner: haraken@chromium.org
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.
Owner: hongchan@chromium.org
hongchan: Would you mind taking a look at this?

Issue 460843 has been merged into this issue.
Cc: sriram...@samsung.com ligimole@google.com tasak@chromium.org
 Issue 527569  has been merged into this issue.
Labels: -Pri-1 Pri-2
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.
Project Member

Comment 7 by ClusterFuzz, Jan 19 2018

Components: Blink>Internals>WTF Blink>Media
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Comment 8 by yutak@chromium.org, Jan 19 2018

Components: -Blink>Internals>WTF
Status: Started (was: Assigned)
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().
Components: Blink>WebAudio
Project Member

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

Project Member

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

Comment 13 by ClusterFuzz, Jan 30 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
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