Crash in mozilla::MediaDecoderStateMachine::InitiateSeek

RESOLVED FIXED in Firefox 49

Status

()

Core
Audio/Video: Playback
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: marcia, Assigned: kaku)

Tracking

({crash, regression})

Trunk
mozilla50
Unspecified
Windows 7
crash, regression
Points:
---

Firefox Tracking Flags

(firefox48 unaffected, firefox49 fixed, firefox50 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-f24ae9fa-5889-49d7-b6bf-eba212160606.
=============================================================

Seen while looking at crash-stats. http://bit.ly/1YaTiWF has the full set of crashes so far.

Crashes begin on trunk with build ID 20160531030258

ni on kaku@mozilla.com since it appears that he worked in that part of the code.
Flags: needinfo?(kaku)
(Assignee)

Comment 1

2 years ago
JW, 

Is it possible that a "AccurateVideoOnly" seek be queued into the MDSM::mQueuedSeek anyhow and then be resumed again later in the MDSM::StartDecoding() which calls MDSM::InitialSeek()?

MDSM::InitialSeek() does not handle the "AccurateVideoOnly" seek type so that the MDSM::mSeekTask is a nullptr at the crashing point.
Assignee: nobody → kaku
Flags: needinfo?(kaku) → needinfo?(jwwang)
|AccurateVideoOnly| is used in InitiateVideoDecodeRecoverySeek() only which never touches mQueuedSeek. I can't see how this is possible.
Flags: needinfo?(jwwang)
(Assignee)

Comment 3

2 years ago
Per discussion offline, we still suspect that it is an "AccurateVideoOnly" seek which does not handled in the MDSM::InitialSeek() so that the MDSM::mSeekTask is a nullptr at the crashing point. The possibility comes from that an "AccurateVideoOnly" is suspended while the MDSM is going to change to dormant state. 

We would like to land a patch with MOZ_DIAGNOSTIC_ASSERT() to confirm our guess first. Patch is following.
(Assignee)

Comment 4

2 years ago
Created attachment 8760617 [details]
Bug 1278228 - use MOZ_DIAGNOSTIC_ASSERT() to confirm that AccurateVideoOnly seek might reach MDSM::InitiateSeek();

Review commit: https://reviewboard.mozilla.org/r/58156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58156/
Attachment #8760617 - Flags: review?(jwwang)
Comment on attachment 8760617 [details]
Bug 1278228 - use MOZ_DIAGNOSTIC_ASSERT() to confirm that AccurateVideoOnly seek might reach MDSM::InitiateSeek();

https://reviewboard.mozilla.org/r/58156/#review55012
Attachment #8760617 - Flags: review?(jwwang) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b1777f5fe3
use MOZ_DIAGNOSTIC_ASSERT() to confirm that AccurateVideoOnly seek might reach MDSM::InitiateSeek(); r=jwwang
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48b1777f5fe3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 8

2 years ago
New crash reports crashed at the MOZ_DIAGNOSTIC_ASSERT(). So, our guess is confirmed that  "AccurateVideoOnly" seek should be handled by the MDSM::InitialSeek() method. Reopen this issue and handle it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

2 years ago
The MDSM::InitiateSeek() is only called in two cases:
(1) In MDSM::Seek(): pass a new seek task which could only be AccurateSeek, FastSeek or NextFrameSeek.
(2) In MDSM::MaybeFinishDecodeFirstFrame() and MDSM::StartDecoding(): the argument is the MDSM::mQueuedSeek which could by any types include the "AccurateVideoOnly".

And, the MDSM::mQueuedSeek is only initialized in two cases:
(1) In MDSM::Seek(); this would never be the "AccurateVideoOnly" type.
(2) In MDSM::SetDormant(); this could be anything again includes the "AccurateVideoOnly".

So, the problem we faced here is that a "AccurateVideoOnly" type seek task was queued in the MDSM::SetDormant() and then be resumed later in the MDSM::MaybeFinishDecodeFirstFrame() or MDSM::StartDecoding() which pass the queued seek task as an argument into the MDSM::InitiateSeek().

@JW, there are two possible solutions:
(1) As you suggested, treat the "AccurateVideoOnly" as normal "AccurateSeek" and handle it in the MDSM::InitiateSeek(). This is the easier one.
(2) While a "AccurateVideoOnly" seek task is going to be queued, we don't queue it as an "AccurateVideoOnly" but queue it as an "AccurateSeek" instead. I think this solution is more explicit and keep the MDSM::InitiateSeek() away from "AccurateVideoOnly" type.

WDYT?
Flags: needinfo?(jwwang)
IIUC, the problem is that SetDormant() is called before an AccurateVideoOnly seek is completed and therefore AccurateVideoOnly is stored in |mQueuedSeek|, right?

I think we should always do an AccurateSeek after waking up from dormant state because we reset both audio and video decoders when entering the dormant state.
Flags: needinfo?(jwwang)
(Assignee)

Comment 11

2 years ago
Created attachment 8763750 [details]
Bug 1278228 - handle VideoOnly seek as an accurate seek;

Review commit: https://reviewboard.mozilla.org/r/59900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59900/
Attachment #8763750 - Flags: review?(jwwang)
(Assignee)

Updated

2 years ago
Attachment #8760617 - Attachment is obsolete: true
Comment on attachment 8763750 [details]
Bug 1278228 - handle VideoOnly seek as an accurate seek;

https://reviewboard.mozilla.org/r/59900/#review56836

I think we should store SeekTarget with mType==Accurate into mQueuedSeek instead in SetDormant() to prevet MediaFormatReader from doing something interesting when it sees SeekTarget::IsVideoOnly() is true.
Attachment #8763750 - Flags: review?(jwwang) → review-
(Assignee)

Comment 13

2 years ago
Comment on attachment 8763750 [details]
Bug 1278228 - handle VideoOnly seek as an accurate seek;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59900/diff/1-2/
Attachment #8763750 - Flags: review- → review?(jwwang)
Comment on attachment 8763750 [details]
Bug 1278228 - handle VideoOnly seek as an accurate seek;

https://reviewboard.mozilla.org/r/59900/#review56842
Attachment #8763750 - Flags: review?(jwwang) → review+
(Assignee)

Comment 15

2 years ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60712b1cffba

Thanks for the review.
Keywords: checkin-needed, regression

Comment 16

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b486db198015
handle VideoOnly seek as an accurate seek; r=jwwang
Keywords: checkin-needed
Priority: -- → P1

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b486db198015
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

2 years ago
Remind myself to check the crash reports and uplift to 49.
Flags: needinfo?(kaku)
(Assignee)

Comment 19

2 years ago
No more crash is reported after landed this patch. Will uplift this patch to 49.
Flags: needinfo?(kaku)
(Assignee)

Comment 20

2 years ago
Comment on attachment 8763750 [details]
Bug 1278228 - handle VideoOnly seek as an accurate seek;

Approval Request Comment
[Feature/regressing bug #]: bug 1235301
[User impact if declined]: crash.  see the crash report in the comment 1.
[Describe test coverage new/current, TreeHerder]: No more crash is reported after this patch landed.  http://bit.ly/1YaTiWF 
[Risks and why]: Uplifting this patch have few risk.
[String/UUID change made/needed]: None.
Attachment #8763750 - Flags: approval-mozilla-aurora?
Comment on attachment 8763750 [details]
Bug 1278228 - handle VideoOnly seek as an accurate seek;

Crash fix, worked on nightly, let's uplift it! Thanks!
Attachment #8763750 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox48: --- → unaffected
has problem applying to aurora:

merging dom/media/MediaDecoderStateMachine.cpp
warning: conflicts while merging dom/media/MediaDecoderStateMachine.cpp! 
can you take a look, thanks!
Flags: needinfo?(kaku)
(Assignee)

Comment 23

2 years ago
Created attachment 8768670 [details] [diff] [review]
Bug 1278228 - handle VideoOnly seek as an accurate seek; r=jwwang; for aurora (FF49).patch

For uplifting to aurora FF49.
Flags: needinfo?(kaku)
(Assignee)

Comment 24

2 years ago
Carsten, here you are, thanks for helping uplifting.

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2921bfa5a494
status-firefox49: affected → fixed
Crash volume for signature 'mozilla::MediaDecoderStateMachine::InitiateSeek':
 - nightly (version 50): 32 crashes from 2016-06-06.
 - aurora  (version 49): 0 crashes from 2016-06-07.
 - beta    (version 48): 0 crashes from 2016-06-06.
 - release (version 47): 1 crash from 2016-05-31.
 - esr     (version 45): 0 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       1       2       1       1       7       9
 - aurora        0       0       0       0       0       0       0
 - beta          0       0       0       0       0       0       0
 - release       1       0       0       0       0       0       0
 - esr           0       0       0       0       0       0       0

Affected platforms: Windows, Mac OS X, Linux
You need to log in before you can comment on or make changes to this bug.