1406328 - Don't shut down MediaCache::mThread too early

Status: RESOLVED FIXED

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

Description

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

https://reviewboard-hg.mozilla.org/gecko/rev/228b08c8c93437a938ca3e3b6f24bf597690e65d#l2.52

It is possible for an nsIChannel to live longer than a MediaCache. Shutting down the thread too early will cause Dispatch() to fail [1] and result in leaks [2].

[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/netwerk/ipc/ChannelEventQueue.cpp#178

[2] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/xpcom/threads/ThreadEventTarget.cpp#128

Comment 1

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

Attachment: Bug 1406328 - shut down the MediaCache thread in ShutdownThreads phase.

To avoid leaks caused by Dispatch() failures. See comment 0 for the detail.

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

Comment 2

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

Comment on attachment 8916467 [details]
Bug 1406328 - shut down the MediaCache thread in ShutdownThreads phase.

https://reviewboard.mozilla.org/r/187586/#review192594

Clever way to (ab)use ClearOnShutdown! :-)

::: commit-message-50846:1
(Diff revision 1)
> +Bug 1406328 - shut down the MediaCache thread in ShutdownThreads phase.
> +
> +To avoid leaks caused by Dispatch() failures. See comment 0 for the detail.

I think you should also mention that this patch introduces a new (and I think, better) behavior: Only one thread is ever created, and is shared between all MediaCaches.

Comment 3

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

Comment on attachment 8916467 [details]
Bug 1406328 - shut down the MediaCache thread in ShutdownThreads phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187586/diff/1-2/

Comment 4

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

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ceb919e7e82
shut down the MediaCache thread in ShutdownThreads phase. r=gerald

Comment 6

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

Backed out for build bustage at dom/media/MediaCache.cpp:301: 'ShutdownPhase' has not been declared:

https://hg.mozilla.org/integration/autoland/rev/224b59f8a361ddbd51489dc3b74b9d53e66f1df2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ceb919e7e829c358e18ca234c1a100871e29963&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135626892&repo=autoland

/builds/worker/workspace/build/src/dom/media/MediaCache.cpp:301:29: error: 'ShutdownPhase' has not been declared
/builds/worker/workspace/build/src/dom/media/MediaCache.cpp:301:59: error: 'ClearOnShutdown' was not declared in this scope

Comment 7

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

Comment on attachment 8916467 [details]
Bug 1406328 - shut down the MediaCache thread in ShutdownThreads phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187586/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e858715590b
shut down the MediaCache thread in ShutdownThreads phase. r=gerald

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/2e858715590b