1846854 - Crash in Resolve() from [@ mozilla::AudioSinkWrapper::OnAudioEnded]

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

https://crash-stats.mozilla.org/report/index/b73fe3ef-cdd7-48af-bad5-50a110230801 indicates that the changes made for bug 1845811 were not sufficient to prevent this crash occurring.

Earliest know crash report was https://crash-stats.mozilla.org/report/index/a975fa2b-5d64-4ccf-ac59-b55a60230620

+++ This bug was initially created as a clone of Bug #1845811 +++

Crash report: https://crash-stats.mozilla.org/report/index/45fad15b-864c-4525-ac1c-1a9eb0230727

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::Private::Resolve<const bool&>  xpcom/threads/MozPromise.h:1229
1  xul.dll  mozilla::MozPromiseHolderBase<mozilla::MozPromise<bool, nsresult, 0>, mozilla::MozPromiseHolder<mozilla::MozPromise<bool, nsresult, 0> > >::Resolve  xpcom/threads/MozPromise.h:1388
1  xul.dll  mozilla::AudioSinkWrapper::OnAudioEnded  dom/media/mediasink/AudioSinkWrapper.cpp:537
2  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::InvokeMethod  xpcom/threads/MozPromise.h:654
2  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::InvokeCallbackMethod  xpcom/threads/MozPromise.h:685
2  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::ThenValue<mozilla::AudioSinkWrapper*, void   xpcom/threads/MozPromise.h:801
3  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::ThenValueBase::DoResolveOrReject  xpcom/threads/MozPromise.h:623
3  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::ThenValueBase::ResolveOrRejectRunnable::Run  xpcom/threads/MozPromise.h:490
4  xul.dll  mozilla::TaskQueue::Runner::Run  xpcom/threads/TaskQueue.cpp:257
5  xul.dll  nsThreadPool::Run  xpcom/threads/nsThreadPool.cpp:343

Comment 1

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

Set release status flags based on info from the regressing bug 1829054

Comment 2

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/jobs?repo=try&revision=ba7a84aa8a5b0c77c73da3834231c146e29602e5

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1846854 call GetPosition() before deciding whether a new AudioSink is needed r?padenot

This avoids a crash where mEndedPromiseHolder was resolved in GetPosition() as
well in OnAudioEnded().

GetPosition() calls DropAudioPacketsIfNeeded() if necessary so the additional
call is removed.

The GetPosition() call is avoided when mAudioSink is set to avoid
mLastClockSource side-effects, which I don't fully understand.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1846854 test async AudioSink init with end of audio r?padenot

Also remove a comment from AsyncInitFailureWithSyncInitSuccess that was left
over from a earlier iteration of that test but is no longer relevant,
and remove an unnecessary 0.5 -> 0.0 volume transition.

Depends on D185397

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa233991f9ea
call GetPosition() before deciding whether a new AudioSink is needed r=padenot

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a1c7d4ba6be
test async AudioSink init with end of audio r=padenot

Comment 7

[Sandor Molnar[:smolnar]]

Backed out for causing assertion failures in dom/media/mediasink/AudioSinkWrapper.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/8b83e1318e78c20166a0142dfd355945c7cdc8c0

Push with failures

Failure log -> Assertion failure: mIsStarted (Must be called after playback starts.), at /builds/worker/checkouts/gecko/dom/media/mediasink/AudioSinkWrapper.cpp:91

Comment 8

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/jobs?repo=try&revision=8548d92719ae4614f30d4f701c3fca2f890fd01b

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1846854 avoid side effect of GetPosition() resolving mEndedPromiseHolder r?padenot

This avoids a crash where mEndedPromiseHolder was resolved in GetPosition() as
well as in OnAudioEnded().

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e2f5566e6dd
avoid side effect of GetPosition() resolving mEndedPromiseHolder r=padenot

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1e2f5566e6dd

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a369921ff340
test async AudioSink init with end of audio r=padenot

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1846854 avoid side effect of GetPosition() resolving mEndedPromiseHolder r?padenot

This avoids a crash where mEndedPromiseHolder was resolved in GetPosition() as
well as in OnAudioEnded().

Original Revision: https://phabricator.services.mozilla.com/D185513

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: Bug 1846854 test async AudioSink init with end of audio r?padenot

Also remove a comment from AsyncInitFailureWithSyncInitSuccess that was left
over from a earlier iteration of that test but is no longer relevant,
and remove an unnecessary 0.5 -> 0.0 volume transition.

Original Revision: https://phabricator.services.mozilla.com/D185398

Comment 15

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: none
• Fix verified in Nightly: no
• User impact if declined: crash when unmuting immediately before end of video or audio
• Code covered by automated testing: yes
• Steps to reproduce for manual QE testing: don't have manual steps
• Needs manual QE test: no
• Is Android affected?: yes
• Risk associated with taking this patch: small
• Explanation of risk level: the code is a small change in the logic in code used in unmuting media.

Comment 16

[Phabricator Automation]

Uplift Approval Request

• Steps to reproduce for manual QE testing: don't have reproducible manual steps
• Needs manual QE test: no
• Is Android affected?: yes
• Fix verified in Nightly: no
• String changes made/needed: none
• Code covered by automated testing: yes
• User impact if declined: crash when unmuting immediately before end of video or audio
• Explanation of risk level: this is is a small change in the logic in code used in unmuting media
• Risk associated with taking this patch: small

Comment 17

[Natalia Csoregi [:nataliaCs]]

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

Comment 18

[Donal Meehan [:dmeehan]]

:karlt there are conflicts with the uplift requests here and beta caused by:
https://hg.mozilla.org/mozilla-central/rev/1e2f5566e6dd

Comment 19

[Karl Tomlinson (:karlt)]

Conflicts should be resolved, thanks.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9348071 [details]
Bug 1846854 avoid side effect of GetPosition() resolving mEndedPromiseHolder r?padenot

Approved for 117.0b7.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a4f12ab15d56
https://hg.mozilla.org/releases/mozilla-beta/rev/4cf69f7029b8