1354491 - Unify the locks in FileBlockCache

Status: RESOLVED WONTFIX

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

Description

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

FileBlockCache has 2 locks which are mDataMonitor and mFileMonitor.

http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/media/FileBlockCache.h#139
http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/media/FileBlockCache.h#161

Is there any reason to use 2 locks instead of one? Using 2 locks is deadlock prone and losing atomicity which prevents FileBlockCache from being a truly thread-safe class.

Comment 1

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

The idea was I didn't want to hold the data monitor while doing I/O, otherwise if the main thread wanted to take the lock it may block on IO on a non-main thread.

If you are careful to not hold the data monitor while doing I/O, and only touch the file descriptor and mFDCurrentPos on the worker thread, it can probably still be threadsafe.

Comment 2

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

Thanks for the explanation.

We have 2 places which release the data monitor and acquire the file monitor. The comments suggest that it has taken into account the changes happening when mDataMonitor is dropped. So we can safely assume FileBlockCache is a thread-safe class.

http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/dom/media/FileBlockCache.cpp#307
http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/dom/media/FileBlockCache.cpp#370