Closed Bug 878183 Opened 10 years ago Closed 9 years ago

[A/V] deadlock during refreshing a video content

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, firefox24 unaffected, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
firefox24 --- unaffected
b2g18 --- verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

()

Details

Attachments

(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #871485 +++

STR
- visit http://m.terratv.terra.com.br/
-  play video
-  refresh web page during playing video

expected
- restart video playback

result
- freeze video playback before refreshing the page.
  Not always happen. occasionally

prerequisite
 - Bug 876099 - re-enable 3gp support
 - Bug 871485 - [A/V] H/W decoder cannot be shared between applications/tasks
Depends on: 876099, 871485
blocking-b2g: leo+ → ---
blocking-b2g: --- → leo?
Sotaro - Can this affect media playback on YouTube? If so, link the bug up to bug 877024.
Yes, it could affect also to Youtube video playback.
Blocks: 877024
- nsBuiltinDecoderStateMachine's decode thread called OmxDecoder::Pause() with holding nsBuiltinDecoder's ReentrantMonitor.
- OmxDecoder::Pause() stopped by waiting OmxDecoder's mutex
- OmxDecoder's mutex was already hold by OMXCodec::drainInputBuffer()
- OMXCodec::drainInputBuffer() did not exit because ChannelMediaResource::Read() does not exit
- ChannelMediaResource::Read() needs data receive from HttpChannelChild.
- Data arrival needs to come from HttpChannelChild via main thread
- main thread was calling nsBuiltinDecoderStateMachine::SetVolume()
- nsBuiltinDecoderStateMachine::SetVolume() did not exit because the function waiting to hold holding nsBuiltinDecoder's ReentrantMonitor.
In comment #5, following is a problem. Need not to hold nsBuiltinDecoder's ReentrantMonitor.

- nsBuiltinDecoderStateMachine's decode thread called OmxDecoder::Pause() with holding nsBuiltinDecoder's ReentrantMonitor.
Attachment #756851 - Flags: review?(chris.double)
Depends on: 878987
Attachment #756851 - Flags: review?(chris.double) → review+
Committable patch. Carry "chris.double: review+".
Attachment #757964 - Flags: review+
Committable patch for b2g18. Carry "chris.double: review+".
Attachment #756851 - Attachment is obsolete: true
Attachment #757965 - Flags: review+
No longer depends on: 878987
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> https://tbpl.mozilla.org/?tree=Try&rev=a8c6709a0438

A lot of orange on all platforms except b2g.
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> 
> A lot of orange on all platforms except b2g.

I'll see if I can work out why the non-b2g platforms are orange.
Comment on attachment 757964 [details] [diff] [review]
patch v2 - exit ReentrantMonitor when calling OnDecodeThreadFinish()

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +516,5 @@
>      LOG(PR_LOG_DEBUG, ("%p Decode thread finished", mDecoder.get()));
>    }
>    
> +  {
> +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());

This exits the monitor when it's not been entered first.

The monitor is only held while the ReentrantMonitorAutoEnter created on line 495 is alive, and that goes out of scope and is destroyed before you enter the scope here.

That's why your patch is failing on try; the monitor is not held before the ReentrantMonitorAutoExit releases it.
(In reply to Chris Pearce (:cpearce) from comment #13)
> 
> The monitor is only held while the ReentrantMonitorAutoEnter created on line
> 495 is alive, and that goes out of scope and is destroyed before you enter
> the scope here.
> 
> That's why your patch is failing on try; the monitor is not held before the
> ReentrantMonitorAutoExit releases it.

I did not checked this is already fixed in master by Bug 799315. So, it is a problem of only b2g18.
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> (In reply to Chris Pearce (:cpearce) from comment #13)
> > 
> > The monitor is only held while the ReentrantMonitorAutoEnter created on line
> > 495 is alive, and that goes out of scope and is destroyed before you enter
> > the scope here.
> > 
> > That's why your patch is failing on try; the monitor is not held before the
> > ReentrantMonitorAutoExit releases it.
> 
> I did not checked this is already fixed in master by Bug 799315. So, it is a
> problem of only b2g18.

correction:
b2g18's one is incorrectly backported in Bug 860760.
Attachment #757964 - Attachment is obsolete: true
Attachment #757965 - Attachment is obsolete: true
Fix code same as in master.
Attachment #758562 - Flags: review?(chris.double)
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> https://tbpl.mozilla.org/?tree=Try&rev=c1f9b8bce3cd

All reftests failed. They are always failed on b2g18. See Bug 877400 comment #22.
Comment on attachment 758562 [details] [diff] [review]
patch v3 for b2g18 - exit ReentrantMonitor when calling OnDecodeThreadFinish()

Chris Pearce designed/developed the state machine threading mechanism so I'll pass review on to him.
Attachment #758562 - Flags: review?(chris.double) → review?(cpearce)
Attachment #758562 - Flags: review?(cpearce) → review+
Add header to the patch. Carry "cpearce: review+".
Attachment #758562 - Attachment is obsolete: true
Attachment #758918 - Flags: review+
This bug is only for b2g18. Master does not have the problem.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/eff45c60396e
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.1 QE2 (6jun)
Keywords: verifyme
QA Contact: jsmith
Flags: in-moztrap?
Depends on: 880743
Followup bug filed in bug 880743.
Flags: in-moztrap? → in-moztrap+
Added Browser Suite Test Case #8565 [Browser] Refreshing a video played through the browser restarts the video normally
Whiteboard: leorun3
Whiteboard: leorun3
Blocks: 885032
You need to log in before you can comment on or make changes to this bug.