Crash in mozilla::MediaDecoderReaderWrapper::RequestAudioData

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: marcia, Assigned: jwwang)

Tracking

({crash})

Trunk
mozilla50
Unspecified
Mac OS X
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox50 fixed)

Details

(crash signature)

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-f3ce2f76-5ac8-40ba-9291-40a7b2160707.
=============================================================

Seen while looking at nightly crash stats. Low volume Mac and Windows crash seen on both Nightly and Beta 5 - http://bit.ly/29t6RhM

ni on jwwang since he worked in this code area.
Flags: needinfo?(jwwang)

Updated

2 years ago
Component: Audio/Video → Audio/Video: Playback
See Also: → bug 1285374
(Assignee)

Comment 1

2 years ago
Created attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

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

Comment 2

2 years ago
Hi Gerald,
Taipei office is closed due to typhoon today. Can you review this patch or find someone else?
Thanks!
Flags: needinfo?(jwwang)
Good luck with the weather! I'll have a look...
Assignee: nobody → jwwang
Attachment #8768995 - Flags: review?(gsquelart) → review+
Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

https://reviewboard.mozilla.org/r/62972/#review59916

Seems reasonable to me as a band-aid.
Please make sure you open a new bug to come back to this issue.

If it's not too late in the cycle, please consider this:

::: dom/media/AccurateSeekTask.cpp:110
(Diff revision 1)
> -  MOZ_ASSERT(!mDoneAudioSeeking);
> -  MOZ_ASSERT(!mReader->IsRequestingAudioData());
> -  MOZ_ASSERT(!mReader->IsWaitingAudioData());
> +  //MOZ_ASSERT(!mDoneAudioSeeking);
> +  //MOZ_ASSERT(!mReader->IsRequestingAudioData());
> +  //MOZ_ASSERT(!mReader->IsWaitingAudioData());

Wouldn't it be worth keeping these for a few weeks, and actually changing them to MOZ_DIAGNOSTIC_ASSERT's, so that we can get more information about what's actually wrong, in Nightly&Aurora?
(Assignee)

Comment 5

2 years ago
Good idea! I will add more assertions to help debugging.
(Assignee)

Comment 6

2 years ago
Created attachment 8769074 [details]
Bug 1285248. Part 2 - fix logic for video-only seek.

Review commit: https://reviewboard.mozilla.org/r/63044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63044/
Attachment #8768995 - Attachment description: Bug 1285248 - add a band-aid fix to prevent the assertion. → Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.
(Assignee)

Comment 7

2 years ago
Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62972/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8769074 - Flags: review?(gsquelart)
(Assignee)

Updated

2 years ago
Attachment #8768995 - Flags: review+ → review?(gsquelart)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

https://reviewboard.mozilla.org/r/62972/#review59968
Attachment #8768995 - Flags: review?(jwwang)
(Assignee)

Comment 9

2 years ago
https://reviewboard.mozilla.org/r/62972/#review59970
(Assignee)

Updated

2 years ago
Attachment #8768995 - Flags: review?(jwwang)
Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

https://reviewboard.mozilla.org/r/62972/#review59972

Confirming r+ for this fix.

::: dom/media/AccurateSeekTask.cpp:48
(Diff revision 2)
>                                     const media::TimeUnit& aEnd,
>                                     int64_t aCurrentMediaTime)
>    : SeekTask(aDecoderID, aThread, aReader, Move(aSeekJob))
>    , mCurrentTimeBeforeSeek(media::TimeUnit::FromMicroseconds(aCurrentMediaTime))
>    , mAudioRate(aInfo.mAudio.mRate)
> -  , mDoneAudioSeeking(!aInfo.HasAudio() || aSeekJob.mTarget.IsVideoOnly())
> +  , mDoneAudioSeeking(!aInfo.HasAudio() || mSeekJob.mTarget.IsVideoOnly())

Good find!
We really need static analysis to find use-after-move issues.
Attachment #8768995 - Flags: review?(gsquelart) → review+
Attachment #8769074 - Flags: review?(gsquelart) → review+
Comment on attachment 8769074 [details]
Bug 1285248. Part 2 - fix logic for video-only seek.

https://reviewboard.mozilla.org/r/63044/#review59974

::: dom/media/AccurateSeekTask.cpp:312
(Diff revision 1)
>    // resolved.
>  
>    SAMPLE_LOG("OnAudioDecoded [%lld,%lld] disc=%d",
>      audio->mTime, audio->GetEndTime(), audio->mDiscontinuity);
>  
> +  // video-only seek doesn't reset audio decoder. There might be pending audio

Nit: 'video' -> 'Video' with capital 'V'.
(Assignee)

Comment 12

2 years ago
Thanks for the review!
(Assignee)

Comment 13

2 years ago
Comment on attachment 8769074 [details]
Bug 1285248. Part 2 - fix logic for video-only seek.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63044/diff/1-2/

Comment 14

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32de23e9cca6
Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor. r=gerald
https://hg.mozilla.org/integration/autoland/rev/0aa6ee3eb1d8
Part 2 - fix logic for video-only seek. r=gerald
(Assignee)

Updated

2 years ago
Blocks: 1282658

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/32de23e9cca6
https://hg.mozilla.org/mozilla-central/rev/0aa6ee3eb1d8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1285374
Because the volume of crash is small, we don't need to uplift the patch in beta. Let's let it ride on 50.
status-firefox48: affected → wontfix
You need to log in before you can comment on or make changes to this bug.