Closed Bug 1278228 Opened 3 years ago Closed 3 years ago

Crash in mozilla::MediaDecoderStateMachine::InitiateSeek

Categories

(Core :: Audio/Video: Playback, defect, P1, critical)

Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: marcia, Assigned: kaku)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 1 obsolete file)

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)
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)
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.
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/48b1777f5fe3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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 → ---
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)
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-
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+
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
https://hg.mozilla.org/mozilla-central/rev/b486db198015
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Remind myself to check the crash reports and uplift to 49.
Flags: needinfo?(kaku)
No more crash is reported after landed this patch. Will uplift this patch to 49.
Flags: needinfo?(kaku)
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+
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)
For uplifting to aurora FF49.
Flags: needinfo?(kaku)
Carsten, here you are, thanks for helping uplifting.
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.