Opened 9 months ago

Closed 4 months ago

Last modified 4 months ago

#29120 closed defect (user disappeared)

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

Reported by: QZw2aBQoPyuEVXYVlBps Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-disk-leak, tbb-usability-website, TorBrowserTeam201903R
Cc: mcs, brade, pospeselr Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

It seems this value was defaulted to 0 after this ticket: https://trac.torproject.org/projects/tor/ticket/10237

The issue this causes can be seen on this page:
https://livestreamfails.com/post/28515

If you open this link in a default configured Tor Browser (I'm using v8.0.4), and open the network inspector (CTRL+SHIFT+E), you will see the media file is constantly being re-requested, downloading the file a few hundred kilobytes at a time using range requests.

This causes the video to load extremely slowly. If the HTTP server does not support range requests, the video will seemingly become unplayable in the native player.

This kind of behavior could also be seen as abusive or mistaken as DoS related traffic by website operators, since it is not typically how browsers download media.

Setting media.cache_size to the default Firefox value of 512000 fixes this, it may also work with a lower cache size, but I've only tried the default Firefox value. I suppose it also may bring back the original issue of the media cache being stored on disk, that ticket is from 5 years ago though, so I'm sure if that situation has changed.

This bug only seems to affect certain media files, I'm not exactly sure what the other factors are in triggering the behavoir.

This bug also exists in the latest stable version of Firefox if you set the media.cache_size to 0, I suppose it's rarely encountered though since the default is much higher.

Child Tickets

Attachments (5)

memorymediacache.patch (714 bytes) - added by QZw2aBQoPyuEVXYVlBps 9 months ago.
FileBlockCache -> MemoryBlockCache
MemoryMediaCache2.patch (2.1 KB) - added by QZw2aBQoPyuEVXYVlBps 9 months ago.
MemoryMediaCache3.patch (3.2 KB) - added by QZw2aBQoPyuEVXYVlBps 9 months ago.
MemoryMediaCache4.patch (3.9 KB) - added by QZw2aBQoPyuEVXYVlBps 8 months ago.
MemoryMediaCache5.patch (3.9 KB) - added by QZw2aBQoPyuEVXYVlBps 8 months ago.

Download all attachments as: .zip

Change History (48)

comment:1 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

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.

comment:2 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Priority: MediumHigh

comment:4 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

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.

Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Attachment: memorymediacache.patch added

FileBlockCache -> MemoryBlockCache

comment:5 Changed 9 months ago by gk

Keywords: TorBrowserTeam201901 added

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?

comment:6 Changed 9 months ago by gk

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

comment:7 in reply to:  5 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

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.

comment:8 in reply to:  4 ; Changed 9 months ago by 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

comment:9 in reply to:  8 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

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.

Last edited 9 months ago by QZw2aBQoPyuEVXYVlBps (previous) (diff)

comment:10 in reply to:  description Changed 9 months ago by 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.

Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Attachment: MemoryMediaCache2.patch added

comment:11 Changed 9 months ago by 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?

comment:12 in reply to:  11 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

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.

comment:13 Changed 9 months ago by 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.

Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Attachment: MemoryMediaCache3.patch added

comment:14 Changed 9 months ago by gk

Cc: mcs brade pospeselr added
Keywords: TorBrowserTeam201901R added; TorBrowserTeam201901 removed

Thanks, setting this up for review.

comment:15 Changed 9 months ago by gk

Status: newneeds_review

comment:16 Changed 9 months ago by pospeselr

This patch looks good to me. You confirmed this issue also works in vanilla Firefox right? We should probably open up a issue on bugzilla and see about getting this uplifted.

comment:17 in reply to:  16 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Replying to pospeselr:

This patch looks good to me. You confirmed this issue also works in vanilla Firefox right? We should probably open up a issue on bugzilla and see about getting this uplifted.

The behavior is present in vanilla Firefox, though it might be considered "working as intended" since in vanilla Firefox you probably wouldn't be expected to set the file cache size to 0 like we do.

There is indeed a comment in the source that points towards this behavior being known: "The cache can also be overflowing because the media.cache_size preference was reduced." (https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l1162)

This patch doesn't change the underlying behavior of the file backed cache, so the odd loading behavior would still be encountered if you forced the Tor Browser out of private browsing.

It might still be worth pushing this patch upstream though, since in my eyes it makes sense to never write to disk in private browsing mode, even in vanilla Firefox.

comment:18 Changed 9 months ago by gk

Keywords: TorBrowserTeam201902R added; TorBrowserTeam201901R removed

Moving our review tickets to February.

comment:19 Changed 9 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201902R removed
Status: needs_reviewneeds_revision

You have

 if (aContentLength < 0) {

in your patch but looking at the original code I think we should take care of the case here as well that aContentLength is 0.

Additionally, I think we should go back to the default size for media.cache_size so that users outside of private browsing mode get the usual behavior (right now they would still be affected by this bug without resetting the pref). (https://gitweb.torproject.org/tor-browser.git/tree/browser/app/profile/000-tor-browser.js?h=tor-browser-60.5.0esr-8.5-1 is the file to look at)

QZw2aBQoPyuEVXYVlBps: Thanks for this find and a patch, really appreciated. We are close here. Do you want to fix the remaining things up or should we? How should we credit you? (I am fine crediting QZw2aBQoPyuEVXYVlBps in the commit message :) )

pospeselr: Could you open a ticket at Mozilla's bug tracker with the attached patch and find out whether Mozilla would take it (even somewhat modified)?

comment:20 in reply to:  19 ; Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Replying to gk:

You have

 if (aContentLength < 0) {

in your patch but looking at the original code I think we should take care of the case here as well that aContentLength is 0.

If you look here: https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l153
You'll see that aContentLength is -1 if it is unknown, so an aContentLength of 0 means that's what the server actually sent.
I can test whether this plays nicely with the current code or not, but if not, what do you suggest we do here?

Additionally, I think we should go back to the default size for media.cache_size so that users outside of private browsing mode get the usual behavior (right now they would still be affected by this bug without resetting the pref). (https://gitweb.torproject.org/tor-browser.git/tree/browser/app/profile/000-tor-browser.js?h=tor-browser-60.5.0esr-8.5-1 is the file to look at)

I agree with this, I was going to mention it but it slipped my mind. I also suggested above that the default of media.memory_cache_max_size should be increased to handle buffering longer media content in memory, what do you think about this?

QZw2aBQoPyuEVXYVlBps: Thanks for this find and a patch, really appreciated. We are close here. Do you want to fix the remaining things up or should we? How should we credit you? (I am fine crediting QZw2aBQoPyuEVXYVlBps in the commit message :) )

It's a little time consuming for me to test my code since compiling takes awhile, but since there's only a few changes, I think I should be fine completing it.
You can credit either "QZw2aBQoPyuEVXYVlBps" or "anonymous", this is simply a throwaway account because I wasn't able to get the cypherpunks account to work.

comment:21 Changed 9 months ago by QZw2aBQoPyuEVXYVlBps

Here is the result of loading a media file where the server gives a "0" as the content length:

[45086:Main Thread]: D/MemoryBlockCache 0x7f6f9075b340 MemoryBlockCache() MEMORYBLOCKCACHE_ERRORS='InitUnderuse'
[45086:Main Thread]: D/MemoryBlockCache 0x7f6f9075b340 Init()
[45086:Main Thread]: D/MediaCache GetMediaCache(0) -> Memory MediaCache 0x7f6f99e24870
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 opened
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 feeding reader
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 DataStarted: 0 aLoadID=1 aLength=0
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 at end of stream
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 at end of stream
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 at end of stream
[Child 45086, MediaPlayback #1] WARNING: Decoder=7f6f92aa1e00 state=DECODING_METADATA Decode metadata failed, shutting down decoder: file /var/tmp/build/firefox-4d0f9fa5fdd5/dom/media/MediaDecoderStateMachine.cpp, line 340
[Child 45086, MediaPlayback #1] WARNING: Decoder=7f6f92aa1e00 Decode error: NS_ERROR_DOM_MEDIA_METADATA_ERR (0x806e0006) - static mozilla::MP4Metadata::ResultAndByteBuffer mozilla::MP4Metadata::Metadata(mozilla::ByteStream*): Cannot parse metadata: file /var/tmp/build/firefox-4d0f9fa5fdd5/dom/media/MediaDecoderStateMachine.cpp, line 3118
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 at end of stream
[45086:MediaCache]: D/MediaCache Stream 0x7f6fa8857040 closed
[45086:Main Thread]: D/MediaCache MediaCacheStream::~MediaCacheStream(this=0x7f6fa8857040) MEDIACACHESTREAM_LENGTH_KB=0
[45086:Main Thread]: D/MediaCache ~MediaCache(Memory-backed MediaCache 0x7f6f99e24870)
[45086:Main Thread]: D/MemoryBlockCache 0x7f6f9075b340 ~MemoryBlockCache() - destroying buffer of size 0; combined sizes now 0

The end result for the user is a "No video with supported format and MIME type found." error message on the video. This result seems sane to me.

comment:22 in reply to:  20 Changed 8 months ago by gk

Replying to QZw2aBQoPyuEVXYVlBps:

Replying to gk:

You have

 if (aContentLength < 0) {

in your patch but looking at the original code I think we should take care of the case here as well that aContentLength is 0.

If you look here: https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l153
You'll see that aContentLength is -1 if it is unknown, so an aContentLength of 0 means that's what the server actually sent.

Yes. I am a bit wary that we are subtly changing the logic here: right now in Firefox esr60 a memory-backed media cache is only used if aContentLength is larger than 0 and less or equal than the max size as determined by a pref. Your patch keeps the latter requirement. However, it changes the former exposing the memory cache creation to a content length of 0. While this seems okay for now I am a little bit worried that Mozilla changes something under the hood which is not affecting them as they have the > check while we don't exclude the case where aContentLength is 0. So, I guess without making your patch even more complicated, one way forward would be to take it in the alpha and try to upstream it at the same time, so we are sure we won't get surprised by our logic change. That's fine with me.

Additionally, I think we should go back to the default size for media.cache_size so that users outside of private browsing mode get the usual behavior (right now they would still be affected by this bug without resetting the pref). (https://gitweb.torproject.org/tor-browser.git/tree/browser/app/profile/000-tor-browser.js?h=tor-browser-60.5.0esr-8.5-1 is the file to look at)

I agree with this, I was going to mention it but it slipped my mind. I also suggested above that the default of media.memory_cache_max_size should be increased to handle buffering longer media content in memory, what do you think about this?

We could do that. But I'd be cautious here. Maybe let's pick twice as the default for now? That should not be worse than the status quo and we essentially have not heard about the bug you reported before. So, the problem might not be that widespread.

QZw2aBQoPyuEVXYVlBps: Thanks for this find and a patch, really appreciated. We are close here. Do you want to fix the remaining things up or should we? How should we credit you? (I am fine crediting QZw2aBQoPyuEVXYVlBps in the commit message :) )

It's a little time consuming for me to test my code since compiling takes awhile, but since there's only a few changes, I think I should be fine completing it.
You can credit either "QZw2aBQoPyuEVXYVlBps" or "anonymous", this is simply a throwaway account because I wasn't able to get the cypherpunks account to work.

Okay, let's use "cypherpunk" then. :)

Thanks for you work here and testing, much appreciated. If you could change the pref I am happy to merge the patch and test it wider in an upcoming alpha.

Last edited 8 months ago by gk (previous) (diff)

comment:23 in reply to:  16 ; Changed 8 months ago by cypherpunks

If you want a more sane solution and not "sane" - do not touch that Mozilla's if code block, it's not broken. Add your own if (aIsPrivateBrowsing) after it. Never use int64_t for something with size in its name. Never use int64_t() in C++. Use spellchecker. What happens when you return nullptr;?

comment:24 in reply to:  23 ; Changed 8 months ago by QZw2aBQoPyuEVXYVlBps

Replying to cypherpunks:

If you want a more sane solution and not "sane" - do not touch that Mozilla's if code block, it's not broken. Add your own if (aIsPrivateBrowsing) after it. Never use int64_t for something with size in its name. Never use int64_t() in C++. Use spellchecker. What happens when you return nullptr;?

We either use their if or copy their code to our own if, I think reusing their if is better than code duplication. We want the same exact logic to handle either case, so it makes sense to use that block. int64_t is what Mozilla uses, I'm just following their code. I tested what happened on nullptr return above, see comment 10.

The next patch includes the preference changes, I removed the media.cache_size preference override and added one for media.memory_cache_max_size, doubling the default value.

Last edited 8 months ago by QZw2aBQoPyuEVXYVlBps (previous) (diff)

Changed 8 months ago by QZw2aBQoPyuEVXYVlBps

Attachment: MemoryMediaCache4.patch added

comment:25 in reply to:  24 Changed 8 months ago by cypherpunks

Replying to QZw2aBQoPyuEVXYVlBps:

Replying to cypherpunks:

If you want a more sane solution and not "sane" - do not touch that Mozilla's if code block, it's not broken. Add your own if (aIsPrivateBrowsing) after it. Never use int64_t for something with size in its name. Never use int64_t() in C++. Use spellchecker. What happens when you return nullptr;?

We either use their if or copy their code to our own if, I think reusing their if is better than code duplication.

That's because you don't see the difference.

We want the same exact logic to handle either case, so it makes sense to use that block.

And we don't.

int64_t is what Mozilla uses, I'm just following their code.

Heh.

I tested what happened on nullptr return above, see comment 10.

Exactly. Now think what's wrong.

The next patch includes the preference changes, I removed the media.cache_size preference override

Good.

and added one for media.memory_cache_max_size, doubling the default value.

Not good.

If you want to write a more sane code, you can try to pass Mozilla's review on Bugzilla.

comment:26 Changed 8 months ago by gk

Keywords: TorBrowserTeam201902R added; TorBrowserTeam201902 removed
Status: needs_revisionneeds_review

comment:27 Changed 8 months ago by gk

mcs/brade can you have a (second) look on the latest patch? Thanks!

comment:28 in reply to:  27 ; Changed 8 months ago by mcs

Replying to gk:

mcs/brade can you have a (second) look on the latest patch? Thanks!

The patch looks good to us. We are a little concerned about portions of comment:25, especially the part about return nullptr; (because we are do not understand cyperpunks' point).

It might be worthwhile to add parens around the second part of the if expression; with the patched code, clang emits a -Wlogical-op-parentheses warning due to potential confusion when reading the code. The following would be better:

  if (aIsPrivateBrowsing ||
      (aContentLength > 0 &&
       aContentLength <= int64_t(MediaPrefs::MediaMemoryCacheMaxSize()) * 1024)) {

comment:29 in reply to:  28 Changed 8 months ago by QZw2aBQoPyuEVXYVlBps

Replying to mcs:

The patch looks good to us. We are a little concerned about portions of comment:25, especially the part about return nullptr; (because we are do not understand cyperpunks' point).

I also have no idea what cypherpunks meant by that comment. The original Mozilla code also returns nullptr on failure, so we're not changing any logic in that regard. It's even documented in the function comment: https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l152

comment:30 Changed 8 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201902R removed
Status: needs_reviewneeds_revision

Setting to needs_revision to address mcs's concern. We are close! :)

comment:31 Changed 8 months ago by QZw2aBQoPyuEVXYVlBps

Here's the patch with the parentheses fixed.

Changed 8 months ago by QZw2aBQoPyuEVXYVlBps

Attachment: MemoryMediaCache5.patch added

comment:32 Changed 8 months ago by gk

Keywords: TorBrowserTeam201903R added; TorBrowserTeam201902 removed
Status: needs_revisionneeds_review

comment:33 Changed 8 months ago by cypherpunks

Don't waste time. Open a ticket on Bugzilla.

comment:34 Changed 8 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Commit b7c1e2820087e60d019b57d25042a27dc83adc4a on tor-browser-60.5.1esr-8.5-1 has the patch.

pospeselr: Can you file an upstream ticket and get the patch upstreamed?

QZw2aBQoPyuEVXYVlBps: thanks again for analyzing the bugs and providing a patch, really appreciated!

comment:35 Changed 8 months ago by pospeselr

Opened https://bugzilla.mozilla.org/show_bug.cgi?id=1532486 and provided edited patch without the tor-browser specific pref.

comment:36 Changed 7 months ago by cypherpunks

Resolution: fixed
Status: closedreopened

This sucks as expected. But the case that you can't explain to mozilla what the bug is, and they wontfixed you, is more hilarious. :)
When the cache dies, if you smart enough, you can get:

All candidate resources failed to load. Media load paused.
TypeError: this.video.error is null videocontrols.xml:1277:1
NS_ERROR_NOT_AVAILABLE:   videocontrols.xml:391

comment:37 in reply to:  36 Changed 7 months ago by gk

Status: reopenedneeds_information

Replying to cypherpunks:

This sucks as expected. But the case that you can't explain to mozilla what the bug is, and they wontfixed you, is more hilarious. :)
When the cache dies, if you smart enough, you can get:

All candidate resources failed to load. Media load paused.
TypeError: this.video.error is null videocontrols.xml:1277:1
NS_ERROR_NOT_AVAILABLE:   videocontrols.xml:391

How can we reproduce that? And what are the cases that suck as expected?

Last edited 7 months ago by gk (previous) (diff)

comment:38 Changed 4 months ago by gk

Resolution: user disappeared
Status: needs_informationclosed

comment:39 Changed 4 months ago by cypherpunks

Resolution: user disappeared
Status: closedreopened

user disappeared, but the bug didn't.

comment:40 in reply to:  39 ; Changed 4 months ago by gk

Resolution: user disappeared
Status: reopenedclosed

Replying to cypherpunks:

user disappeared, but the bug didn't.

Great! Please answer the questions in comment:37 when reopening the bug. So gar we still don't know how we can investigate it.

comment:41 in reply to:  40 Changed 4 months ago by cypherpunks

Resolution: user disappeared
Status: closedreopened

Replying to gk:

Replying to cypherpunks:

user disappeared, but the bug didn't.

Great! Please answer the questions in comment:37 when reopening the bug.

Think about media.memory_caches_combined_limit_pc_sysmem. Sucks that you can't explain mozillians that they should fix their bug.

So gar we still don't know how we can investigate it.

Who are we?

comment:42 Changed 4 months ago by gk

Resolution: user disappeared
Status: reopenedclosed

Okay, I am closing this again. Please leave it that way. No need to clutter this bug with further. If you happen to find _steps to reproduce_ we can try out, open a new ticket with all the necessary information (like operating system, Tor Browser version and the actual steps to reproduce you bug).

comment:43 Changed 4 months ago by cypherpunks

Great that you closed it as user disappeared, meaning that you couldn't fix it. But it's a weird reason to close confirmed bugs anyway.
What the steps do you need when you have the source code? Memory limit means that you get the error with your patch, opening no more than 4 media instances on a 1 GB VM. The error instead of no cache, because

I also have no idea what cypherpunks meant by that comment. The original Mozilla code also returns nullptr on failure, so we're not changing any logic in that regard. It's even documented in the function comment: ​https://hg.mozilla.org/releases/mozilla-esr60/file/256453759958ed9c2eb17a0764d2fcfd7f8e3323/dom/media/MediaCache.cpp#l152

nullptr if initialization failed

Note: See TracTickets for help on using tickets.