bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Potential deadlock detected in MediaSystemResourceManager and MediaCodecProxy

RESOLVED FIXED in Firefox 46

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwwang, Assigned: sotaro)

Tracking

unspecified
mozilla46
Unspecified
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Hit this assertion when running media tests on emulator-x86-kk.

###!!! ERROR: Potential deadlock detected:
=== Cyclical dependency starts at
--- ReentrantMonitor : MediaSystemResourceManager.mReentrantMonitor calling context
  [stack trace unavailable]
--- Next dependency:
--- Mutex : MediaCodecProxy::mMediaCodecLock (currently acquired)
 calling context
  [stack trace unavailable]
=== Cycle completed at
--- ReentrantMonitor : MediaSystemResourceManager.mReentrantMonitor calling context
  [stack trace unavailable]
Deadlock may happen for some other execution
(Reporter)

Updated

3 years ago
OS: Unspecified → Gonk (Firefox OS)
(Assignee)

Comment 1

3 years ago
The problem seems to happen when codec allocation failed.
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 2

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> The problem seems to happen when codec allocation failed.

cycle could happen only during callback of MediaSystemResourceClient.
(Assignee)

Comment 3

3 years ago
From the source ,the problem does not exit to OMXCodecProxy.
(Assignee)

Comment 4

3 years ago
Created attachment 8707782 [details] [diff] [review]
patch - Do not call ReleaseMediaCodec() during callback
(Assignee)

Updated

3 years ago
Attachment #8707782 - Flags: review?(bwu)
Comment on attachment 8707782 [details] [diff] [review]
patch - Do not call ReleaseMediaCodec() during callback

Review of attachment 8707782 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little worried about the |ReleaseMediaCodec| will be invoked on other thread when destroy MediaCodecProxy object.
Since the ImageBridge thread doesn't support tail dispatch, is it possible the |ReleaseMediaCodec| and |ResourceReserved| run simultaneously when the promise be rejected?
Comment on attachment 8707782 [details] [diff] [review]
patch - Do not call ReleaseMediaCodec() during callback

Forward this review to :bechen since he is also looking into this bug.
Attachment #8707782 - Flags: review?(bwu) → review?(bechen)
(Assignee)

Comment 7

3 years ago
I rechecked the code. mMediaCodecLock seems unnecessary anymore that was added by Bug 1132832.
(Assignee)

Updated

3 years ago
See Also: → bug 1132832
(Assignee)

Comment 8

3 years ago
(In reply to Benjamin Chen [:bechen] from comment #5)
> 
> Review of attachment 8707782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little worried about the |ReleaseMediaCodec| will be invoked on other
> thread when destroy MediaCodecProxy object.
> Since the ImageBridge thread doesn't support tail dispatch, is it possible
> the |ReleaseMediaCodec| and |ResourceReserved| run simultaneously when the
> promise be rejected?

Current implementation seems to have a problem. I am going to update MediaCodecProxy::ReleaseMediaCodec().
(Assignee)

Updated

3 years ago
Attachment #8707782 - Flags: review?(bechen)
(In reply to JW Wang [:jwwang] from comment #0)
> Hit this assertion when running media tests on emulator-x86-kk.
> 
> ###!!! ERROR: Potential deadlock detected:
> === Cyclical dependency starts at
> --- ReentrantMonitor : MediaSystemResourceManager.mReentrantMonitor calling
> context
>   [stack trace unavailable]
> --- Next dependency:
> --- Mutex : MediaCodecProxy::mMediaCodecLock (currently acquired)
>  calling context
>   [stack trace unavailable]
> === Cycle completed at
> --- ReentrantMonitor : MediaSystemResourceManager.mReentrantMonitor calling
> context
>   [stack trace unavailable]
> Deadlock may happen for some other execution

mReentrantMonitor is a reentrantMonitor, so the codec allocation failed won't deadlock.
(Assignee)

Comment 10

3 years ago
From code, it would cause deadlock. lock is held opposite order like the following.

// MediaCodecProxy::ReleaseMediaCodec()
[1] MediaCodecProxy::mMediaCodecLock
[2] MediaSystemResourceManager.mReentrantMonitor

// MediaCodecProxy::ResourceReserved() // callback
[1] MediaSystemResourceManager.mReentrantMonitor is already held
[2] MediaCodecProxy::mMediaCodecLock
(Assignee)

Comment 11

3 years ago
Created attachment 8708192 [details] [diff] [review]
patch - Do not call ReleaseMediaCodec() during callback
Attachment #8707782 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8708193 [details] [diff] [review]
patch - Fix potential deadlock and race condition
Attachment #8708192 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8708193 - Flags: review?(bechen)
Comment on attachment 8708193 [details] [diff] [review]
patch - Fix potential deadlock and race condition

Review of attachment 8708193 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/omx/MediaCodecProxy.cpp
@@ +89,5 @@
>                                   bool aEncoder)
>    : mCodecLooper(aLooper)
>    , mCodecMime(aMime)
>    , mCodecEncoder(aEncoder)
> +  , mPromiseMonitor("MediaCodecProxy::::mPromiseMonitor")

Too many colons.
Attachment #8708193 - Flags: review?(bechen) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8708393 [details] [diff] [review]
patch - Fix potential deadlock and race conditio

Apply the comment. Carry "r=bechen".
Attachment #8708193 - Attachment is obsolete: true
Attachment #8708393 - Flags: review+

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10e7bf5884cf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.