1354389 - MediaCache::Update() on the main thread can be blocked by I/O on a non-main thread

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Michael Layzell's BHR telemetry has one (1) report of the content processes' main thread being blocked for greater than 8 seconds.

Here's the main thread's stack.

WaitForSingleObjectEx (in kernelbase.pdb)
PR_MD_WAIT_CV (in nss3.pdb)
PR_WaitCondVar (in nss3.pdb)
PR_WaitCondVar (in nss3.pdb)
PR_EnterMonitor (in nss3.pdb)
mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter(mozilla::ReentrantMonitor &) (in xul.pdb)
mozilla::MediaCache::Update() (in xul.pdb)
mozilla::UpdateEvent::Run() (in xul.pdb)
mozilla::ValidatingDispatcher::Runnable::Run() (in xul.pdb)
nsThread::ProcessNextEvent(bool,bool *) (in xul.pdb)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) (in xul.pdb)
mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) (in xul.pdb)
MessageLoop::RunHandler() (in xul.pdb)
MessageLoop::Run() (in xul.pdb)
nsBaseAppShell::Run() (in xul.pdb)
nsAppShell::Run() (in xul.pdb)
XRE_RunAppShell() (in xul.pdb)
mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) (in xul.pdb)
MessageLoop::RunHandler() (in xul.pdb)
MessageLoop::Run() (in xul.pdb)
XRE_InitChildProcess(int,char * * const,XREChildData const *) (in xul.pdb)
content_process_main(mozilla::Bootstrap *,int,char * * const) (in firefox.pdb)
NS_internal_main(int,char * *,char * *) (in firefox.pdb)
wmain (in firefox.pdb)
__scrt_common_main_seh (in firefox.pdb)

https://people-mozilla.org/~mlayzell/bhr/20170327/26.html


We're blocked in MediaCache::Update() taking its reentrant monitor. It looks to me like we do IO on the media cache's thread while holding that monitor. For example:
https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/MediaCache.cpp#656

calls through to:
https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/FileBlockCache.cpp#369

Which will eventually do a read here:
https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/FileBlockCache.cpp#217

We shouldn't be blocking the main thread on I/O!

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

jw: are you free to look into this, or is there someone in Taipei who can look into it?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

For more reports of >8s jank, see https://people-mozilla.org/~mlayzell/bhr/20170327/0.html and just do an text search for "MediaCache::Update"

Comment 3

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

Sure. I will take a look into it.

Comment 4

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

Attachment: Bug 1354389. P1 - drop the monitor before doing IO.

We don't want to do IO while holding the monitor which might be acquired
on the main thread in MediaCache::Update().

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

Comment 5

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

Attachment: Bug 1354389. P2 - remove the unused function.

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

Comment 6

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

Attachment: Bug 1354389. P3 - fix the callers to handle changes when dropping the monitor.

No change is needed for MediaCacheStream::Read() which calls |mon.Wait()|
and has handled changes when dropping the monitor.

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

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8856383 [details]
Bug 1354389. P1 - drop the monitor before doing IO.

https://reviewboard.mozilla.org/r/128308/#review131130

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8856384 [details]
Bug 1354389. P2 - remove the unused function.

https://reviewboard.mozilla.org/r/128310/#review131132

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8856385 [details]
Bug 1354389. P3 - fix the callers to handle changes when dropping the monitor.

https://reviewboard.mozilla.org/r/128312/#review131136

Comment 10

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

Thanks for the review!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d5a3c330358
P1 - drop the monitor before doing IO. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/f821fd07f34d
P2 - remove the unused function. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b31c9e6837ca
P3 - fix the callers to handle changes when dropping the monitor. r=cpearce

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d5a3c330358
https://hg.mozilla.org/mozilla-central/rev/f821fd07f34d
https://hg.mozilla.org/mozilla-central/rev/b31c9e6837ca