1816852 - Crash in [@ mozilla::MediaFormatReader::OnVideoSeekCompleted]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/bca9f683-2fed-4cfe-840d-e9ee00230122

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0  ntdll.dll  RtlAcquireSRWLockExclusive  
1  xul.dll  mozilla::OffTheBooksMutex::Lock  xpcom/threads/Mutex.h:65
1  xul.dll  mozilla::detail::BaseAutoLock<mozilla::Mutex&>::BaseAutoLock  xpcom/threads/Mutex.h:236
1  xul.dll  mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1>::Private::Resolve<mozilla::media::TimeUnit>  xpcom/threads/MozPromise.h:1215
2  xul.dll  mozilla::MozPromiseHolderBase<mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1>, mozilla::MozPromiseHolder<mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::SeekRejectValue, 1> > >::Resolve  xpcom/threads/MozPromise.h:1372
2  xul.dll  mozilla::MediaFormatReader::OnVideoSeekCompleted  dom/media/MediaFormatReader.cpp:2992
3  xul.dll  mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::MediaResult, 1>::ThenValue<mozilla::MediaFormatReader*, void   xpcom/threads/MozPromise.h:730
4  xul.dll  mozilla::MozPromise<nsTAutoStringN<char16_t, 64>, nsresult, 0>::ThenValueBase::ResolveOrRejectRunnable::Run  xpcom/threads/MozPromise.h:489
5  xul.dll  mozilla::TaskQueue::Runner::Run  xpcom/threads/TaskQueue.cpp:259
6  xul.dll  nsThreadPool::Run  xpcom/threads/nsThreadPool.cpp:309

This crash seems to be caused by mSeekPromise being NULL when it's accessed. Note: there are no crashes under this signatures because we first need to land bug 1816846 for them to appear, the stack above however represents the problem. Once bug 1816846 several different signatures will collapse here amounting to several 10s of crashes per week.

Comment 1

[Gabriele Svelto [:gsvelto]]

Adding two more signatures that will appear here after bug 1816846. This seems to affect all platforms.

Comment 2

[C.M.Chang[:chunmin]]

Hi Gabriele, do you know how to reproduce it reliably? It seems OnVideoSeekCompleted has remained unchanged for years. If there is a particular test we can run or a regression window, that will be very helpful for further investigation of this issue.

Comment 3

[Gabriele Svelto [:gsvelto]]

Looking at the comments in the crash reports doesn't help, but these particular crashes started to have significant volume in version 109. Given they're all the same type of crash I suspect the reason must be the same everywhere and might be related to the asynchronous nature of this code. I can crack open a minidump to check to what object the NULL pointer being accessed belongs, though as I said in comment 0, it appears to be the promise itself.

Comment 4

[Gabriele Svelto [:gsvelto]]

I've opened a minidump and I can confirm that the issue is caused by mSeekPromise being NULL here. The other crashes that will fall under this signature once bug 1816846 lands are the same, the promise is NULL leading to the crash. This means that the promise has already been resolved, or rejected - which cleared the pointer in the MozPromiseHolder object, and we're rejecting or resolving it a second time leading to the crash.

Comment 5

[C.M.Chang[:chunmin]]

Hey Alastor, can you have a look?

Comment 6

[Alastor Wu [:alwu]]

No idea for now, but keep NI. Will take a look again later

Comment 7

[Alastor Wu [:alwu]]

I guess this is somehow related with bug 1815798. We can see the crash rate significantly dropped on Fx111 where we landed some changes. Let's see if the crash would be fixed completely after landing the another fix in bug 1815798.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Looks like we're still seeing crashes on Nightly and Beta builds with the most recent patch from bug 1815798 included.
https://crash-stats.mozilla.org/report/index/446d5b96-83c1-4c0e-b549-8520f0230324
https://crash-stats.mozilla.org/report/index/a73acf8d-8c86-41db-988e-4b7a80230329
https://crash-stats.mozilla.org/report/index/cde7e7a4-53a3-4804-9bdf-5e2090230329
https://crash-stats.mozilla.org/report/index/07f4c8c6-ec90-499e-b4b1-bb8070230329

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

I'm only going by an educated guess here, but I've been staring at this all day, and I think the only way this could happen is:

Our promise is exclusive (it is):
https://searchfox.org/mozilla-central/rev/504bd0a7ee3110ecb2b31f89b9ce54faf76e228b/dom/media/MediaFormatReader.h#88

So when we resolve it, it does a Move instead of a copy:
https://searchfox.org/mozilla-central/rev/504bd0a7ee3110ecb2b31f89b9ce54faf76e228b/xpcom/threads/MozPromise.h#718
https://searchfox.org/mozilla-central/rev/504bd0a7ee3110ecb2b31f89b9ce54faf76e228b/xpcom/threads/MozPromise.h#169

So that the second Then invocation gets a null pointer. That describes the situation we are in.

There is strong evidence that we are indeed violating the exclusive property of the promise, e.g. in the signature [@ mozilla::MediaFormatReader::RequestAudioData ]:
https://crash-stats.mozilla.org/report/index/0256b29f-ea17-4ccb-ba1f-728e50230325

and in the signature [@ mozilla::MozPromise<T>::ChainTo ]
https://crash-stats.mozilla.org/report/index/3cf83294-817c-4fca-8ec9-51e680230405

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

I posted in the wrong bug :|.

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1816852 - don't resolve/reject same promise twice.

When a new seeking starts, we always reset the demuxer which should be
able to prevent the former seeking callback from being invoked.
So ideally we shouldn't resolve/reject same promise twice.

The situation of causing this problem is still unknown. Therefore, use a
workaround to resolve/reject a promise only when it exists, and add
more assertions to help diagnose the problem.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aade104e461f
don't resolve/reject same promise twice. r=media-playback-reviewers,padenot

Comment 13

[Noemi Erli[:noemi_erli]]

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

Comment 14

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

The patch landed in nightly and beta is affected.
:alwu, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit BugBot documentation.

Comment 15

[Alastor Wu [:alwu]]

Crash volume is low, let it ride with the train of Fx114.