See bug 782460 comment 22. We *should not* have privileged content using the media cache, but if some emergency arises and we do, or someone wants to test a bug with in-process content, this footgun will go off. blocking+ not because this is a problem currently, but if it ever arises we'll have no way to fix this in the field. Media just flat-out won't work.

Also, what ensures that cache files are cleaned up in the event of a crash or an _exit(0)?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #23) > doublec, this is going to be a icky little footgun. Is there a reason we > create a directory in which to store the cache files? Just cleanliness? > > We might need to create this as |mozilla-media-cache-$UID|. Continuing the discussion started in bug 782460... All the media cache files are stored in a single directory for cleanliness. We can move them elsewhere if need be, that's fine. (In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > Also, what ensures that cache files are cleaned up in the event of a crash > or an _exit(0)? The cache files are opened with the nsIFile::DELETE_ON_CLOSE bit set. My understanding is DELETE_ON_CLOSE is supposed to use OS mechanisms to ensure the file is deleted even if we crash. DELETE_ON_CLOSE is (apparently) unreliable on windows, so we have an idle service observer (nsMediaCacheRemover) that runs on windows *only* to clean up old media cache files. Other B2G media cache issues I'm concerned about are: * We've limited the size of the media cache to 4MB on B2G. This is going to cause a crappy user experience, as much of the media can't buffer. * We're creating the media cache on B2G on the system partition. If my understanding of what Edwin and Chris Double are saying is correct, we only have about 50MB free space left on the Otoro device system partition? * We create one media cache per content process. So if multiple browser tabs/apps try to load <audio> or <video> we could run out of space on the system partition. It seems to me that on B2G we should put the media cache files on the SD card, if present. Roc also suggested remoting the media cache to the main process so that we can have a single (larger) media cache. I wonder how that would affect performance.

For the record, I think just remoting the media cache to the main process is probably not the way to go. Some sort of cross-process protocol for roughly bounding the sum of the media cache file sizes might be good.

(In reply to Chris Pearce (:cpearce) from comment #2) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #23) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > > Also, what ensures that cache files are cleaned up in the event of a crash > > or an _exit(0)? > > The cache files are opened with the nsIFile::DELETE_ON_CLOSE bit set. My > understanding is DELETE_ON_CLOSE is supposed to use OS mechanisms to ensure > the file is deleted even if we crash. > Gotcha. That's unlink() on POSIX. > Other B2G media cache issues I'm concerned about are: > * We've limited the size of the media cache to 4MB on B2G. This is going to > cause a crappy user experience, as much of the media can't buffer. The important case in the initial market will be playing on-device media, which means Blob and no cache. So agreed that this is bad but it's not an urgent priority for b2g v1. We should file this though/ > * We're creating the media cache on B2G on the system partition. If my > understanding of what Edwin and Chris Double are saying is correct, we only > have about 50MB free space left on the Otoro device system partition? /data partition. (/system is read-only.) We'll have O(100MB) on /data, but it's shared with installed applications/ > It seems to me that on B2G we should put the media cache files on the SD > card, if present. > And if not? And if the SD card is removed while the cache is being used? That's a good idea but it's tricky to implement. > Roc also suggested remoting the media cache to the main process so that we > can have a single (larger) media cache. I wonder how that would affect > performance. Since the cache side of things is mostly throughput oriented, not latency oriented, it should be fine. This is a good idea too. But it's very complex.

Hot-plugging of the SD card is an extremely rare event I am happy to not support for V1. A more problematic case is pluging the device into a USB master at which point we mount the SD card and revoke R/W access from the device. Overall I think the SD card is still the better solution here though. We should simply pretend the media cache was purged when we lose access.

We won't be able to play media at all in any of those cases, because we won't be able to recreate the cache. Unless we add code to fall back somewhere else, which is part of the complexity I was referring to above.

cjones, who should own this work?

Chris.

Chris Pearce wins the lottery of the Chrises :) If you won't have time for this, cpearce, just let us know.

This appends "-10000" to the media cache dir name for apps' media caches', which I assume is what we want.

Comment on attachment 658741 [details] [diff] [review] Patch: Append uid to media cache temp dir on B2G. This is what we want to do, but I would really prefer if this wasn't b2g specific. We'll have this same issue with content processes on any multi-user system. A little mxr archaeology turns up http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/public/nsIUserInfo.idl#18 . Can we use this here instead?

Hmmm, so for multi-user systems we create the media cache in the profile dir, but only if (XRE_GetProcessType() != GeckoProcessType_Content). I guess for the B2G main process that's going to evaluate to true? The last time I asked the profile directory didn't exist on B2G, so we'll fail in that case? I guess we should change to use $system_temp/mozilla-media-cache-$username as the media cache dir as you suggest to handle all these cases.

(In reply to Chris Pearce (:cpearce) from comment #12) > Hmmm, so for multi-user systems we create the media cache in the profile > dir, but only if (XRE_GetProcessType() != GeckoProcessType_Content). Note, I was careful to say "with content processes". It's not a problem when only one process is storing the cache in the profile. (Unless multiple uids are sharing profiles, but they deserve whatever brokenness they get.) > I guess > for the B2G main process that's going to evaluate to true? The last time I > asked the profile directory didn't exist on B2G, so we'll fail in that case? > It does. The existing code should work just fine in the main process.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13) > (In reply to Chris Pearce (:cpearce) from comment #12) > > I guess > > for the B2G main process that's going to evaluate to true? The last time I > > asked the profile directory didn't exist on B2G, so we'll fail in that case? > > > > It does. The existing code should work just fine in the main process. Just to clarify, do you mean that the B2G main process does have a profile dir (maybe the Gaia profile dir? or somewhere else?), or do you mean that the code does fail, i.e. B2G still does not have a profile dir? If B2G does have a profile dir (which I think is what you meant), and if we can access it in main and sun processes, then we can always create the media cache in $profile_dir/media-cache-$username. If not we may as well use $system_temp/mozilla-media-cache-$username so that we're always storing it in the same place.

(In reply to Chris Pearce (:cpearce) from comment #14) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13) > > (In reply to Chris Pearce (:cpearce) from comment #12) > > > I guess > > > for the B2G main process that's going to evaluate to true? The last time I > > > asked the profile directory didn't exist on B2G, so we'll fail in that case? > > > > > > > It does. The existing code should work just fine in the main process. > > Just to clarify, do you mean that the B2G main process does have a profile > dir (maybe the Gaia profile dir? or somewhere else?), or do you mean that > the code does fail, i.e. B2G still does not have a profile dir? > B2G has a normal Gecko profile. If for some reason (god forbid!) we ever used the media cache from the main b2g process with the current code, it would Just Work. (It previously Just Worked until we moved video and music OOP, before we bypass cache for Blobs.) > If B2G does have a profile dir (which I think is what you meant), and if we > can access it in main and sun processes, then we can always create the media > cache in $profile_dir/media-cache-$username. If not we may as well use > $system_temp/mozilla-media-cache-$username so that we're always storing it > in the same place. Low-rights child processes don't have write privileges in the profile directory, on purpose. Out of the entire file system, they can only write to $tmp. I don't have an opinion about moving media caches for all processes to $tmp.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > A little mxr archaeology turns up > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/ > public/nsIUserInfo.idl#18 . Can we use this here instead? That converts the username to a UTF8 C string. I'm unsure if using this would be a good idea on systems with non-Latin character sets or not, so I'll stick with the +uid approach.

The real problem here is that we don't want to store media cache files in a subdir on non-Windows systems, as if we create the subdir with the root process on B2G then non-root sub-processes can't write to it. So let's not do that. Then we don't need a B2G specific solution, as requested by cjones. We need to do store cache files in a subdir on Windows however, since DELETE_ON_CLOSE is apparently unreliable on Windows, and we run an idleservice observer to remove stray media cache files by recursively deleting the subdir.

Greenish on Try: https://tbpl.mozilla.org/?tree=Try&rev=57f5771e3416

Comment on attachment 660246 [details] [diff] [review] Patch: Don't store media cache files in subdir Review of attachment 660246 [details] [diff] [review]: ----------------------------------------------------------------- This approach is OK but we really should define a new special-directory tag (NS_PROCESS_TEMP_DIR?) that maps to NS_OS_TEMP_DIR on non-Windows and a special directory in the profile on Windows. nsMediaCacheRemover would be generalized to periodically wipe all the files in NS_PROCESS_TEMP_DIR that aren't open (Windows only). Then the media cache, and any other client, can just create a unique file in NS_PROCESS_TEMP_DIR with FILE_DELETE_ON_CLOSE and we don't need any platform-specific code there. ::: content/media/nsMediaCache.cpp @@ +539,5 @@ > + // * Content processes in multi-process Gecko store their media cache > + // files in the system temp dir. Note that we deliberately do not store > + // media cache files in a sub-directory of system temp dir, as if the > + // sub-directory was created by a priviledged process it may not be > + // accessible by unpriviledged content processes. Why not always put the media cache temp file in the temp directory, for non-Windows?

We discussed this here in the Auckland office and verified that yes we do still need to handle FILE_DELETE_ON_CLOSE failing (see bug 572579 comment 49). nsIFile::DELETE_ON_CLOSE is a yucky API; every user has to work around it failing on Windows, so we decided to fix the underlying API. Fortunately the only users of nsIFile::DELETE_ON_CLOSE are the media cache and a test, so it's easy to change the API. We can make this API much more user friendly if we change it so that you just ask for a temporary file that will be deleted on close, and you get back a nsIFile for you to use (you can't specify the path or name). This file will be in the profile dir if we have one, or in the system tempdir if we don't (e.g. b2g unprivileged processes). On Windows it will be in a folder in the profile dir which we attempt to recursively delete in the idle service observer (nsMediaCacheRemover can be re-purposed to do this).

Create a function NS_OpenAnonymousTemporaryFile(PRFileDesc**) that returns a file handle to an open DELETE_ON_CLOSE file. The file's name has a random component to help prevent name collisions. On Windows this file is stored in $profileDir/mozilla-anon-temp-files/, and we delete this directory recursively in an idle service observer. I re-purposed the old nsMediaCacheRemover to perform this. On non-Windows this file is in the system temp dir, but not in a sub dir of the temp dir. This means we won't have the problem that cjones was worried about on B2G where a root process creates the dir containing the temp files, preventing other non-root processes from accessing that dir. Looks like it's going greenish on Try: https://tbpl.mozilla.org/?tree=Try&rev=07ffde0de600

(In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > See bug 782460 comment 22. > > We *should not* have privileged content using the media cache, but if some > emergency arises and we do, or someone wants to test a bug with in-process > content, this footgun will go off. > We have privileged content using the media cache now.

Better get this reviewed and landed then. ;)

Comment on attachment 664351 [details] [diff] [review] Patch: Add NS_OpenAnonymousTemporaryFile(), use it in nsMediaCache::Init(). Review of attachment 664351 [details] [diff] [review]: ----------------------------------------------------------------- Use "hg move" to preserve history from nsMediaCacheRemover.cpp to nsAnonymousTemporaryFile.cpp ::: xpcom/io/nsAnonymousTemporaryFile.cpp @@ +13,5 @@ > +#include "nsITimer.h" > + > +#endif > + > +#include "nsAnonymousTemporaryFile.h" This should be the first include in this file. It's important to make sure it compiles without any other stuff defined before it. @@ +52,5 @@ > +#ifdef XP_WIN > + // On windows DELETE_ON_CLOSE is unreliable, so we store temporary files > + // in a subdir of the temp dir and delete that in an idle service observer > + // to ensure it's been cleared. > + rv = tmpFile->AppendNative(nsDependentCString("mozilla-anon-temp-files")); I think just "mozilla-temp-files" @@ +61,5 @@ > + > + // Give the temp file a name with a random element. CreateUnique will also > + // append a counter to the name if it encounters a name collision. > + nsAutoString name(NS_LITERAL_STRING("mozilla-temp-")); > + name.AppendInt(rand()); Comment that this is a performance optimization. ::: xpcom/io/nsAnonymousTemporaryFile.h @@ +21,5 @@ > + * > + * The file will be deleted when the file handle is closed. On Windows this > + * mechanism is unreliable if the application crashes or the computer power > + * cycles unexpectedly, so unopened temporary files are purged at some time > + * after application startup. Add a comment that on non-Windows platforms the file will be unlinked before this function even returns.

Thanks for the quick review! https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9d8bf66243

Backed out for mochitest-other leaks on Windows (just like on the Try run...): https://tbpl.mozilla.org/php/getParsedLog.php?id=15507648&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee0ff72fdf3

Also M2 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=15506741&tree=Mozilla-Inbound And M1: https://tbpl.mozilla.org/php/getParsedLog.php?id=15507212&tree=Mozilla-Inbound (Both present on the Try run)

Yay for 8 hour Windows Try turn arounds. The nsITimer was leaking. The nsITimer's callback function releases the nsITimer, so I guess the callback isn't always called. I'd tested this code path locally on Linux machine and I'd found that the nsITimer's callback wasn't being called on Linux unless I held a reference to the nsITimer. Maybe I was mistaken about the Linux behaviour, but I've re-tested on Windows without holding a reference to the nsITimer the nsITimer's callback was called. Maybe I was mistaken, but at least that is what the old behaviour of nsMediaCacheRemover was. This patch is the same as the previous, except in nsAnonymousTemporaryFile.cpp I don't hold a ref to the nsITimer, like we previously did in nsMediaCacheRemover. https://tbpl.mozilla.org/?tree=Try&rev=bf007ad4699b

We do need to hold a reference to the timer. If we're leaking on shutdown, I guess you need to make sure the timer is cancelled on shutdown, using a shutdown observer.

Changes from last patch/version: * Add a shutdown observer, and store a ref to the timer. We rely on the shutdown observer's reference and the idle service's reference to keep the anonymous temp file remover alive. * Don't create the temp file remover if we're not in the main gecko process. We had test failures on windows in test_ipc on Try because of this, as content processes don't have authorization to access the profile dir. It's taken ages to debug this remotely on Try... * Always store temp files in the OS temp dir (or a subdir of the OS temp dir on Windows), as NS_GetSpecialDirectory(NS_APP_USER_PROFILE_LOCAL_50_DIR,*) fails for content processes (at least on Windows and B2G). On Win NT the OS temp dir is still in the Users ~/AppData dir.