1278228 - Crash in mozilla::MediaDecoderStateMachine::InitiateSeek

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

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 [e-mail] since it appears that he worked in that part of the code.

Comment 1

[Tzuhao Kuo [: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.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

|AccurateVideoOnly| is used in InitiateVideoDecodeRecoverySeek() only which never touches mQueuedSeek. I can't see how this is possible.

Comment 3

[Tzuhao Kuo [:kaku]]

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 4

[Tzuhao Kuo [:kaku]]

Attachment: 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/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b1777f5fe3
use MOZ_DIAGNOSTIC_ASSERT() to confirm that AccurateVideoOnly seek might reach MDSM::InitiateSeek(); r=jwwang

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48b1777f5fe3

Comment 8

[Tzuhao Kuo [:kaku]]

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.

Comment 9

[Tzuhao Kuo [:kaku]]

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?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 11

[Tzuhao Kuo [:kaku]]

Attachment: 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/

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 13

[Tzuhao Kuo [:kaku]]

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/

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

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

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

Comment 15

[Tzuhao Kuo [:kaku]]

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

Thanks for the review.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b486db198015
handle VideoOnly seek as an accurate seek; r=jwwang

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b486db198015

Comment 18

[Tzuhao Kuo [:kaku]]

Remind myself to check the crash reports and uplift to 49.

Comment 19

[Tzuhao Kuo [:kaku]]

No more crash is reported after landed this patch. Will uplift this patch to 49.

Comment 20

[Tzuhao Kuo [: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.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Crash fix, worked on nightly, let's uplift it! Thanks!

Comment 22

[Carsten Book [:Tomcat]]

has problem applying to aurora:

merging dom/media/MediaDecoderStateMachine.cpp
warning: conflicts while merging dom/media/MediaDecoderStateMachine.cpp! 
can you take a look, thanks!

Comment 23

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1278228 - handle VideoOnly seek as an accurate seek; r=jwwang; for aurora (FF49).patch

For uplifting to aurora FF49.

Comment 24

[Tzuhao Kuo [:kaku]]

Carsten, here you are, thanks for helping uplifting.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2921bfa5a494

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

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