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

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: cpearce, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1][bhr])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

5 months ago
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!
(Reporter)

Comment 1

5 months ago
jw: are you free to look into this, or is there someone in Taipei who can look into it?
Flags: needinfo?(jwwang)
(Reporter)

Comment 2

5 months ago
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"
(Assignee)

Comment 3

5 months ago
Sure. I will take a look into it.
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
Whiteboard: [qf] → [qf:p1]
(Assignee)

Updated

5 months ago
Depends on: 1354444
(Assignee)

Updated

5 months ago
Depends on: 1354491
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

4 months ago
mozreview-review
Comment on attachment 8856383 [details]
Bug 1354389. P1 - drop the monitor before doing IO.

https://reviewboard.mozilla.org/r/128308/#review131130
Attachment #8856383 - Flags: review?(cpearce) → review+
(Reporter)

Comment 8

4 months ago
mozreview-review
Comment on attachment 8856384 [details]
Bug 1354389. P2 - remove the unused function.

https://reviewboard.mozilla.org/r/128310/#review131132
Attachment #8856384 - Flags: review?(cpearce) → review+
(Reporter)

Comment 9

4 months ago
mozreview-review
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
Attachment #8856385 - Flags: review?(cpearce) → review+
(Assignee)

Comment 10

4 months ago
Thanks for the review!

Comment 11

4 months ago
Pushed by jwwang@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d5a3c330358
https://hg.mozilla.org/mozilla-central/rev/f821fd07f34d
https://hg.mozilla.org/mozilla-central/rev/b31c9e6837ca
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [qf:p1] → [qf:p1][bhr]
You need to log in before you can comment on or make changes to this bug.