884654 - Deadlock because of OmxDecoder::statusChanged()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

When OmxDecoder::statusChanged() is called. At first the function grab OMXCodecProxy's mutex and then nsBuiltinDecoder's ReentrantMonitor. The order is opposite of other places. This could make dead lock.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: Call stacks of deadlock related threads

Comment 2

[Jason Smith [:jsmith]]

Can this bug impact watching of YouTube videos off of YouTube's HTML 5 mobile site significantly?

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Jason Smith [:jsmith] from comment #2)
> Can this bug impact watching of YouTube videos off of YouTube's HTML 5
> mobile site significantly?

It happens sometimes not significantly. But when the bug happens, only way is kill the app.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - fix deadlock come from OmxDecoder::statusChanged()

Confirmed the patch fix the deadlock on v1.1 leo.

Comment 5

[Sotaro Ikeda [:sotaro]]

The deadlock could be relatively easily regenerated by STR in Bug 884182 comment #0.

Comment 6

[cajbir (:cajbir)]

Comment on attachment 764544 [details] [diff] [review]
patch - fix deadlock come from OmxDecoder::statusChanged()

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

::: content/media/omx/OmxDecoder.cpp
@@ +767,5 @@
> +    case kNotifyPostReleaseVideoBuffer:
> +    {
> +      Mutex::Autolock autoLock(mSeekLock);
> +      // Free pending video buffers when OmxDecoder is not seeking video.
> +      // If OmxDecoder is in seeking video, the buffers are freed on seek exit. 

Remove trailing whitespace. Remove 'in'.

@@ +768,5 @@
> +    {
> +      Mutex::Autolock autoLock(mSeekLock);
> +      // Free pending video buffers when OmxDecoder is not seeking video.
> +      // If OmxDecoder is in seeking video, the buffers are freed on seek exit. 
> +      if (mIsVideoSeeking != true) {

if (!mIsVideoSeeking)

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - fix deadlock come from OmxDecoder::statusChanged()

Apply the comment. Carry "chris.double: review+".

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 for b2g18 - fix deadlock come from OmxDecoder::statusChanged()

A patch for b2g18. Carry "chris.double: review+".

Comment 9

[Sotaro Ikeda [:sotaro]]

Comment on attachment 764803 [details] [diff] [review]
patch v2 for b2g18 - fix deadlock come from OmxDecoder::statusChanged()

carry r+.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/879afe4b00b7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/879afe4b00b7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5720f3900d23

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5720f3900d23