Resuming a suspended reader sometimes causes crash

RESOLVED FIXED in Firefox 51

Status

()

Firefox for Android
Audio/Video
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwwang, Assigned: esawin)

Tracking

50 Branch
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
The feature is added by bug 1278574.

repro steps:
1. add a integer pref: media.decoder.limit=1 to about:config
2. open http://people.mozilla.org/~jwwang/test_reader_suspend.html

result:
Sometimes the 1st video is not resumed and sometimes it just crashes.
(Reporter)

Updated

a year ago
Depends on: 1278574
(Assignee)

Updated

a year ago
Assignee: nobody → esawin
(Assignee)

Comment 1

a year ago
Created attachment 8774776 [details] [diff] [review]
0001-Bug-1289016-1.1-Don-t-initialize-decoder-if-reader-i.patch

I see two (intermittent) things going wrong here:
1. Assertion "Suspended reader must be in mSuspended". This is triggered when a reader is added and removed from the ReaderQueue in quick succession, before its resuming is processed on the task queue. This assertion is wrong.
2. Given some unlucky timing, we may end up in a situation where MFR::EnsureDecoderInitialized keeps spamming *Decoder::Init on a suspended decoders, which eventually overwhelms the Android media service and breaks further decoding attempts. We shouldn't call Init on suspended decoders.

That's all I can see so far concerning the ReaderQueue. Can you take the r? or should I redirect to jya?
Attachment #8774776 - Flags: review?(jwwang)
(Assignee)

Comment 2

a year ago
Created attachment 8774961 [details] [diff] [review]
0001-Bug-1289016-1.2-Don-t-initialize-suspended-decoders-.patch

Updated patch with proper ReaderQueue::Remove mechanics which do not falsely depend on the reader's IsSuspended state.
Attachment #8774776 - Attachment is obsolete: true
Attachment #8774776 - Flags: review?(jwwang)
Attachment #8774961 - Flags: review?(jwwang)
(Reporter)

Comment 3

a year ago
Comment on attachment 8774961 [details] [diff] [review]
0001-Bug-1289016-1.2-Don-t-initialize-suspended-decoders-.patch

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

https://hg.mozilla.org/mozilla-central/file/ceb63dec9267e9bb62f5e5e1f4c9d32d3ac1fbac/dom/media/MediaFormatReader.cpp#l468
So can we remove this check?
Attachment #8774961 - Flags: review?(jwwang) → review?(jyavenard)
(Reporter)

Comment 4

a year ago
Does Android use MediaOmxReader? Do we need to also check IsSuspended() inside the class?
JW do you have a backtrace available?
Flags: needinfo?(jwwang)
(Reporter)

Comment 6

a year ago
No. I didn't see any crash dumps in the logcat. It just exited Fennec silently.
Flags: needinfo?(jwwang)
(Assignee)

Comment 7

a year ago
(In reply to JW Wang [:jwwang] from comment #3)
> https://hg.mozilla.org/mozilla-central/file/
> ceb63dec9267e9bb62f5e5e1f4c9d32d3ac1fbac/dom/media/MediaFormatReader.cpp#l468
> So can we remove this check?

I think this has to stay. We don't want to schedule an update in the case where the decoder is suspended after its initialization.

(In reply to JW Wang [:jwwang] from comment #4)
> Does Android use MediaOmxReader? Do we need to also check IsSuspended()
> inside the class?

MediaOmxReader is only used on Gonk.
Comment on attachment 8774961 [details] [diff] [review]
0001-Bug-1289016-1.2-Don-t-initialize-suspended-decoders-.patch

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

TBH, I don't really understand what that does. but LGTM otherwise
Attachment #8774961 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 9

a year ago
Try looks OK
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed788d6d32d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2858e003f1

Comment 10

a year ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02f8c8bd057d
[1.2] Don't initialize suspended decoders and fix removing readers from the queue.

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02f8c8bd057d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Reporter)

Updated

a year ago
Blocks: 1292047
Created attachment 8779725 [details]
Screenshot_20160810-145131.png

Tested using:
Device: Nexus 9 (Android 6.0)
Build: Firefox for Android 51.0a1 (2016-08-10)

The first video is still displayed blank, after loading the page. 
Is this issue fixed?
(Assignee)

Comment 13

a year ago
This patch fixes the potential crashes with reader suspending and resuming, other usability issues are being addressed in different bugs.

That said, the video may be paused, clicking on play should finish playing it in that case.
You need to log in before you can comment on or make changes to this bug.