1336356 - Ensure MediaDecode::Shutdown() is called by MediaShutdownManager::BlockShutdown()

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/dom/media/MediaShutdownManager.cpp#148

We assume |owner| is null only after MediaDecoder::Shutdown() is called. If this assumption is broken (it is actually broken and fixed by bug 1333576), MediaDecoder::Shutdown() will never be called and we will encounter a shutdown hang.

To make MediaShutdownManager more robust, we want to assert MediaDecoder::IsShutdown() is true to ensure Shutdown() is called on each registered MediaDecoder.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1336356 - Ensure MediaDecode::Shutdown() is called by MediaShutdownManager::BlockShutdown().

Review commit: https://reviewboard.mozilla.org/r/110240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110240/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8834233 [details]
Bug 1336356 - Ensure MediaDecode::Shutdown() is called by MediaShutdownManager::BlockShutdown().

https://reviewboard.mozilla.org/r/110240/#review111360

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2d14d738d2c
Ensure MediaDecode::Shutdown() is called by MediaShutdownManager::BlockShutdown(). r=gerald

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8834233 [details]
Bug 1336356 - Ensure MediaDecode::Shutdown() is called by MediaShutdownManager::BlockShutdown().

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:This patch reduces the chance of shutdown hang.
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:1336345 must be uplifted first.
[Is the change risky?]:low risk.
[Why is the change risky/not risky?]:the change is simple and has been exercised in Central for 1 month without causing obvious regression.
[String changes made/needed]:none

Comment 7

[Liz Henry (:lizzard)]

Comment on attachment 8834233 [details]
Bug 1336356 - Ensure MediaDecode::Shutdown() is called by MediaShutdownManager::BlockShutdown().

This should be uplifted after the fix for bug 1336345. Crash fix.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ecc496aa06dc

Comment 9

[Andrei Vaida [:avaida]]

Setting qe-verify- based on JW Wang's assessment on manual testing needs (see Comment 6) and the fact that this fix has automated coverage.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1336356_esr52_fix.patch

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8847916 [details] [diff] [review]
1336356_esr52_fix.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crash
Fix Landed on Version:54
Risk to taking this patch (and alternatives if risky): The change is simple and has been landed to 54 and 53.
String or UUID changes made by this patch: none

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8847916 [details] [diff] [review]
1336356_esr52_fix.patch

crash fix, esr52+

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/00ba83ac39be