Default value of media.cache_size (0) causes some media to load extremely slowly or become unplayable

added TorBrowserTeam201903R component::applications/tor browser owner::tbb-team priority::high reporter::QZw2aBQoPyuEVXYVlBps resolution::user disappeared severity::normal status::closed tbb-disk-leak tbb-usability-website type::defect labels

I've narrowed down the criteria for when this bug happens. It will trigger for any video played using the built in media player (<video src='...'>) that's filesize is either greater than media.memory_cache_max_size (default 8192kb) or unknown (no Content-Length).

I've also established that setting media.cache_size to 0 does not eliminate writing the cache to disk, it simply limits the cache file on disk to a few hundred kilobytes and constantly cycles it. Due to the nature of how physical storage mediums work, this is likely to be functionally equivalent to writing the entire cache to disk without all the nonsense inbetween.

You can observe this yourself with the following command: inotifywait -me modify,create,delete,move -r /tmp/ | grep mozilla If you open the link in the first post in Tor Browser, you should start seeing mozilla temporary files being written to, this is the disk-backed MediaCache.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

Trac:
Username: QZw2aBQoPyuEVXYVlBps
Priority: Medium to High

https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l1159

Trac:
Keywords: N/A deleted, tbb-disk-leak, tbb-usability-website added

As a preliminary test, I replaced https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l758 with RefPtr<MediaBlockCacheBase> bc = new MemoryBlockCache(8192000);

This seems to work correctly from my brief testing and fixes both bugs in this ticket (the odd media loading behavior and the disk leaking).

I have only done a small amount of investigation into how the media caching code works, so it's likely this requires a closer look, but I believe it's a decent starting point.

I will attached a patch if anyone else would like to test for themselves or look into this further.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

memorymediacache.patch

FileBlockCache -> MemoryBlockCache

Trac:
Username: QZw2aBQoPyuEVXYVlBps

Thanks for the patch! I wonder what happens if the video is larger than the available memory cache (or the latter is already filled up with other cached stuff). Does the browser crash then? Or become unusable otherwise?

Trac:
Keywords: N/A deleted, TorBrowserTeam201901 added

FWIW there are media.memory_cache* preferences as well, which might be useful here.

Replying to gk:

Thanks for the patch! I wonder what happens if the video is larger than the available memory cache (or the latter is already filled up with other cached stuff). Does the browser crash then? Or become unusable otherwise? I tested using the video in the main ticket post, which is ~30mb (the MemoryCache was hardcoded to 8192000 bytes in the patch).

As far as I can tell, it seems to properly reuse old memory once it hits the cache limit, I could play through the whole video without anything unusual happening, if I seeked back to the beginning after playing through it, it would make a new HTTP request to fetch that data, so it seemed to have properly fallen out of the cache.

One thing that should be looked into is if the MemoryBlockCache can safely be used concurrently by multiple streams (as a "single global cache" like the FileBlockCache is), if you look at the code here: https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l737

You'll see that normally, a new MemoryBlockCache object is created for each resource below media.memory_cache_max_size, they don't seem to be reused across multiple resources like the FileBlockCache is.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

Replying to QZw2aBQoPyuEVXYVlBps:

RefPtr<MediaBlockCacheBase> bc = new MemoryBlockCache(8192000); RefPtr<MediaBlockCacheBase> bc = new MemoryBlockCache(0); https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MemoryBlockCache.cpp#l182

Replying to cypherpunks2:

Replying to QZw2aBQoPyuEVXYVlBps:

RefPtr<MediaBlockCacheBase> bc = new MemoryBlockCache(8192000); RefPtr<MediaBlockCacheBase> bc = new MemoryBlockCache(0); https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MemoryBlockCache.cpp#l182 I don't think the code you linked does what you think it does. If you look right before that, you'll see an early return on aContentLength == 0. initialLength is based on the size of the internal buffer, so that code in the if statement will simply be executed on the first write to the cache rather than on initialization, it doesn't change the situation very much.

Another option I thought of while reading the MediaCache source is that we can probably get by with just slightly modifying the current code that creates MemoryBlockCache objects. https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l737

If you would humor some code, this would be replacing the if statement from that linked line:

// The size we will initialize the cache to
int64_t cacheBytes = 0;

if (aContentLength <= 0) {
  // Unknown content length, give it a max sized buffer just to be sure.
  cacheBytes = int64_t(MediaPrefs::MediaMemoryCacheMaxSize()) * 1024;
} else {
  // If the content length is known, we'll try to initialize a cache that will hold it, but only up to the max cache size
  cacheBytes = std::min(aContentLength, int64_t(MediaPrefs::MediaMemoryCacheMaxSize()) * 1024);
}

RefPtr<MediaBlockCacheBase> bc = new MemoryBlockCache(cacheBytes);
nsresult rv = bc->Init();
if (NS_SUCCEEDED(rv)) {
  RefPtr<MediaCache> mc = new MediaCache(bc);
  LOG("GetMediaCache(%" PRIi64 ") -> Memory MediaCache %p", aContentLength,
    mc.get());
  return mc;
}

// Init() failed...
// In the MediaCache source, there is a fallback to a file-backed cache beyond this point.
// We don't want that, so something else will have to be done here.
// In the origial code, the function will return a nullptr (gMediaCache) if both caches fail to initialize, but I'm not sure what the consequences of that are.

This code would create a new MemoryBlockCache for every request, doing away with the content size limit, and instead replacing it with a maximum cache size (based on the media.memory_cache_max_size preference). This should work since my previous test showed that an undersized cache will properly reuse memory as needed. I think it should also play nicely with whatever built in mechanism is in place to manage the total in-memory cache size, since it's pretty much using the original code with just minor modification.

Let me know what you think.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

I've gone ahead and made another patch with the above changes.

In testing, it seems to work well, I believe this is a more "sane" solution than changing the global cache to a MemoryBlockCache.

I monitored the debug logs for FileBlockCache and MemoryBlockCache, I never observed any FileBlockCache code being called, which is good.

I also observed all the allocated MemoryBlockCache objects being properly destroyed after navigating away from pages with media content, so it seems this method doesn't leak any memory.

I also tested what happens when the cache fails to initialize, it simply results it a media error ("No video with supported format and MIME type found."). I think this is probably acceptable since the only case where this would happen is if the user is extremely low on memory or has hit the media.memory_caches_combined_limit_kb, which shouldn't happen unless they have a large amount of tabs open containing media content. There's also really no alternative to it if we're going to be avoiding the disk cache at all costs.

If this patch is used, I would also suggest bumping the default value of media.memory_cache_max_size to something slightly higher, as using this method also limits the amount of read-ahead buffer that can be stored for larger content, which can be slightly detrimental to playback compared to the file-backed cache. I would suggest at least doubling it to 16384, 32768 is what I tested with though and it worked well.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

MemoryMediaCache2.patch

Trac:
Username: QZw2aBQoPyuEVXYVlBps

I have not looked close at the patch but what we should do in my opinion is checking whether we are in private browsing mode and use only the memory cache in that case and otherwise we leave the logic as is. Does that sound reasonable?

Replying to gk:

I have not looked close at the patch but what we should do in my opinion is checking whether we are in private browsing mode and use only the memory cache in that case and otherwise we leave the logic as is. Does that sound reasonable? That seems reasonable, I'm not sure what the best way to check if we're in private browsing mode from that section of code is though.

I can see from the source each media stream in the cache exposes mIsPrivateBrowsing, but I'm pretty sure there will be no streams in the cache during initialization, so that doesn't do us much good.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

Alright, I poked around the code a little more and I think I found a decent solution.

I changed GetMediaCache to add a aIsPrivateBrowsing parameter. As far as I can tell, GetMediaCache is only ever called from MediaCacheStream::Init, so I just changed the call there to pass in the stream's mIsPrivateBrowing member.

The rest of the code is pretty similar to the previous patch, but the new logic only applies in private browsing mode now. I added and updated a few comments in the code to reflect the new behavior as well.

Trac:
Username: QZw2aBQoPyuEVXYVlBps

MemoryMediaCache3.patch

Trac:
Username: QZw2aBQoPyuEVXYVlBps

Default value of media.cache_size (0) causes some media to load extremely slowly or become unplayable

Child items ...

Activity