1779036 - Crash in [@ RtlAcquireSRWLockExclusive | mozilla::MozPromise<T>::Private::Resolve<T>]

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

~400 shutdown crashes from ~300 Windows installations of Firefox 102.0 + 102.0.1

Crash report: https://crash-stats.mozilla.org/report/index/5b88fa01-ced0-4233-960f-ea2280220711

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0 ntdll.dll RtlAcquireSRWLockExclusive 
1 xul.dll mozilla::MozPromise<bool, nsresult, 0>::Private::Resolve<bool> xpcom/threads/MozPromise.h:1210
2 xul.dll mozilla::AudioSinkWrapper::GetPosition dom/media/mediasink/AudioSinkWrapper.cpp:110
3 xul.dll mozilla::VideoSink::SetPlaying dom/media/mediasink/VideoSink.cpp:200
4 xul.dll mozilla::MediaDecoderStateMachine::CompletedState::Step dom/media/MediaDecoderStateMachine.cpp:2069
5 xul.dll mozilla::MediaDecoderStateMachine::RunStateMachine dom/media/MediaDecoderStateMachine.cpp:3583
6 xul.dll mozilla::detail::RunnableMethodImpl<HTMLContentSink*, void  xpcom/threads/nsThreadUtils.h:1200
7 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:230
8 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:259
9 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:310

Comment 1

[Paul Adenot (:padenot)]

We want to switch this to a ResolveIfExists most probably.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1779036 - Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r?alwu

Comment 3

[Donal Meehan [:dmeehan]]

:padenot do you think this will be fixed in the 103 cycle or will need to wait until 104.
The final 103 beta builds tomorrow EOD, was wondering if we'll get a beta uplift request before then.
The patch looks like a simple fix but it's not reviewed, were you hoping to land it in central soon?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c3388d86aa3
Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r=ng

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9c3388d86aa3

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 9285089 [details]
Bug 1779036 - Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r?alwu

Beta/Release Uplift Approval Request

• User impact if declined: Shutdown crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a trivial fix with the cause being clear.
• String changes made/needed:
• Is Android affected?: Yes

Comment 7

[Donal Meehan [:dmeehan]]

Comment on attachment 9285089 [details]
Bug 1779036 - Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r?alwu

We are out of the beta cycle and mozilla-beta was merged to mozilla-release. Morphing this into a release uplift request.

Comment 8

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

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

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

For more information, please visit auto_nag documentation.

Comment 9

[Donal Meehan [:dmeehan]]

Clearing the ni - There is already a pending uplift request for 103 (release instead of beta)

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9285089 [details]
Bug 1779036 - Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r?alwu

Approved for 103.0.1, thanks.

Comment 11

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/d78ebc66880e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR102 approval when you get a chance.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Nico, can you take care of the ESR102 uplift request?

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 9285089 [details]
Bug 1779036 - Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r?alwu

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes a crash and is a trivial fix.
• User impact if declined: Crashes.
• Fix Landed on Version: 103 and up
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This really is a trivial fix and it's been confirmed for some time that it fixes the crash (including on current release).

Comment 15

[Paul Adenot (:padenot)]

Clearing Nico's NI.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9285089 [details]
Bug 1779036 - Account for the fact that the ended promise might already have been resolved in GetPosition, during shutdown. r?alwu

Approved for 102.3esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/83f431182d83