1845811 - Crash in [@ mozilla::AudioSinkWrapper::OnAudioEnded]

Status: RESOLVED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

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 2

[Karl Tomlinson (:karlt)]

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

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1845811 remove unused rv variable r?padenot

Unused since
https://hg.mozilla.org/mozilla-central/rev/203bf275e99e99882638a6b3357c0a6a2f97bffc#l3.124

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1845811 add some assertions r?padenot

Depends on D184792

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1845811 ignore a failed AudioSink initialization if the AudioSink is no longer needed r?padenot

This is consistent with skipping the initialization on mAsyncInitTaskQueue if
there is another initialization pending.

Importantly this avoids rejecting mEndedPromiseHolder when mAudioSink has
already been set synchronously to another AudioSink.

Depends on D184793

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28bc05b0d788
remove unused rv variable r=padenot
https://hg.mozilla.org/integration/autoland/rev/772647e88dde
add some assertions r=padenot
https://hg.mozilla.org/integration/autoland/rev/9aa988f521fe
ignore a failed AudioSink initialization if the AudioSink is no longer needed r=padenot

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/28bc05b0d788
https://hg.mozilla.org/mozilla-central/rev/772647e88dde
https://hg.mozilla.org/mozilla-central/rev/9aa988f521fe

Comment 8

[Karl Tomlinson (:karlt)]

[Tracking Requested - why for this release]:
Wanting to consider this if there is a 116 dot release.
We may have more information about efficacy then.
I have a GTest pending, but there have been some unified build issues to sort out.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1845811 undef macros at the end of TestAudioTrackGraph.cpp r?pehrsons

so as not to conflict with symbols of the same name used in other files such as
testing::internal::FunctionMocker::Invoke().
https://searchfox.org/mozilla-central/rev/83b6c564d257f02289a37b9f0f0c10aa8623c16c/third_party/googletest/googlemock/include/gmock/gmock-spec-builders.h#1481

Also don't risk implying that macro definitions are affected by namespace
blocks.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1845811 remove "using namespace dom" r?pehrsons

so as to avoid ambiguity between VideoFrame and dom::VideoFrame in subsquently
unified TestVideoFrameConverter.

Depends on D185019

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1845811 test async AudioSink init failure with sync init success r?padenot

Depends on D185020

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de6af576269d
undef macros at the end of TestAudioTrackGraph.cpp r=pehrsons,padenot
https://hg.mozilla.org/integration/autoland/rev/3dc2a537d884
remove "using namespace dom" r=pehrsons,padenot

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5dd68da43c93
test async AudioSink init failure with sync init success r=padenot

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/de6af576269d
https://hg.mozilla.org/mozilla-central/rev/3dc2a537d884

Comment 15

[Sandor Molnar[:smolnar]]

Backed out for causing build bustage in dom/media/gtest/TestAudioSinkWrapper.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/7dfd77da1dac3785fbb04ba757d0c8dc6c8e6e88

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/dom/media/gtest/TestAudioSinkWrapper.cpp:38:18: error: use of undeclared identifier 'TimeUnit'; did you mean 'mozilla::media::TimeUnit'?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1689b1a6b28f
test async AudioSink init failure with sync init success r=padenot

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Looks like we're still seeing Nightly and Beta crashes with this signature?

Comment 18

[Karl Tomlinson (:karlt)]

Yes, I'll investigate and follow-up in bug 1846854.
At this stage, there is no evidence that the patches here are worth uplifting to 116.

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5a416dbb0882
https://hg.mozilla.org/releases/mozilla-beta/rev/3ec792e66a4e
https://hg.mozilla.org/releases/mozilla-beta/rev/66c79b33b797

Comment 20

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1689b1a6b28f