1583384 - Recently closed tabs menu is slow due to giant (several mb) data URIs stored for closed tab favicons

Status: NEW

(Firefox :: Session Restore, defect, P3)

Description

[David Smith]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Open the History menu (hit Alt to open top menu, if necessary). Hover over the Recently Closed Tabs flyout menu. Wait for it to open.

Actual results:

It takes a full 3 seconds for the menu to appear.

If I try using a different menu because it seems like it's not working, the new menu choice will not activate until the RCT menu would have been finished. Basically, hovering the RCT menu blocks all menu usage for 3 seconds because it's taking that long to finish.

This is not an issue with initially populating the menu, as the delay happens on every usage.

In a brand new profile with only a single closed tab and no history, it takes about half a second for the menu to appear. That is still too slow.

This issue likely also affects addons which use this history information, such as Undo Close Tab, as it also has a slow response to requests for recently closed tabs. It was the slowness of UCT which led me to check the built-in menu to see if the problem was more likely to be in Firefox or the addon.

Expected results:

Menu should be immediate.

Comment 1

[Pablo]

Hi,

I tried to reproduce this on my Windows7 and windows10 64bit and i can´t replicate the issue.
I am using Firefox Nightly version 71.0a1

When i click on the history history menu by pressin Alt, and select Recently Closed Tabs , it appears normally
Could you please share a video with us of your delay when you open the recentry closed tabs?

Please also download the latest Firefox Nightly from here: https://nightly.mozilla.org/ and retest the problem with a brand new profile.

Thanks
Pablo

Comment 2

[:Gijs (he/him)]

This data is provided by session restore, so moving components for now.

It'd be helpful if you could use the firefox profiler ( https://profiler.firefox.com/ ) to create a profile of the slow behaviour. Basically, install the add-on, start it from the panel that pops up from its toolbar button, then open the recently closed tabs menu, then click the profiler again and click 'capture profile'. It may be helpful to have a profile from a clean Firefox profile with only 1 tab as well as with your "normal" profile that takes 3s, to see what difference is observable...

Comment 3

[David Smith]

OK, the fact that it relates to session restore may connect to another performance issue I was having, which was fixed after multiple wipes of the session restore data.

The browser has actually been performing well over the last few days, but I'm remembering the conditions that were causing issues.

1. Session restore backup files grew extremely large. With most normal browsing, the backup files range from 400KB to 500KB. At the time I was trying to wipe the session restore files, they were ranging from 10 MB to 32 MB.

2. And, I'd almost forgotten, all of this was heavily triggered by visiting some wiki pages on fandom.com (formerly wikia.com). (The specific wiki being tested: https://slave-harem.fandom.com/wiki/Slave_Harem_in_the_Labyrinth_of_the_Other_World_Wiki) Navigating through about a dozen pages on the wiki (jobs, etc), the browser is already getting slow again. Checking the session restore backup files (which were 400-500 KB just a few minutes ago), they've hit 11 MB already.

3. There is a corresponding jump in main process memory usage. about:memory shows the main process using 725 MB in explicit allocations, whereas prior to this navigation, Resource Monitor indicated was using maybe 350 MB (didn't get an exact figure). This extremely excessive memory growth happened after any usage of the fandom.com site, and more extended browsing on the site takes the main process memory usage (just the main process, mind you, not all processes together) well over 1 GB, at which point the entire browser starts slowing to a crawl.

When I started writing this reply, prior to giong back to fandom.com, everything was working smoothly. Testing on a fresh profile with no history also showed no delay. After the wiki navigation, I got this performance profile capture, which reflects a 1-2 second delay on opening the recently closed tabs menu: https://perfht.ml/2oKtwQh (Note: I cannot load this URL myself due to cross-site security restrictions in the browser, but it's possible you may have things set up properly to view it. If not, the file version can be downloaded from: https://drive.google.com/file/d/1rQXinwJaBvse7lqRBn5CmvflOi_lZ9U-/view?usp=sharing )

Comment 4

[:Gijs (he/him)]

(In reply to David Smith from comment #3)

When I started writing this reply, prior to giong back to fandom.com, everything was working smoothly. Testing on a fresh profile with no history also showed no delay. After the wiki navigation, I got this performance profile capture, which reflects a 1-2 second delay on opening the recently closed tabs menu: https://perfht.ml/2oKtwQh (Note: I cannot load this URL myself due to cross-site security restrictions in the browser, but it's possible you may have things set up properly to view it. If not, the file version can be downloaded from: https://drive.google.com/file/d/1rQXinwJaBvse7lqRBn5CmvflOi_lZ9U-/view?usp=sharing )

This is super helpful, thank you.

Something stupid is happening here with data: URIs. This means we spend hundreds of milliseconds just cloning object data containing these URIs, and then later some more hundreds of milliseconds just parsing the URIs back from strings.

Testing this (note: site is NSFW, but the favicon image, https://vignette.wikia.nocookie.net/slave-harem/images/6/64/Favicon.ico/revision/latest?cb=20150227205520, is safe), it seems like somehow this particular webpage sets a 2mb 512x512 pixel image as the favicon for the site, which serialized into base64 takes up close to 3mb. This is represented twice - once on each item in the return value of getClosedTabData and once on the state property of each of those items. So if you have, say, 2 closed tabs from this site, they take up 2 * 2 * 3 = 12mb of data, and that stuff is being serialized and deserialized repeatedly - plus because it's represented as data URIs, it has to actually get parsed, and parsing a 3 million character string into a URI takes a while, too.

This is all pretty awful.

I don't know sessionstore well enough to know what the best way forward is and/or if there are already improvements on file that would help here. Some ideas:

• we shouldn't store the images twice. This would help a bit but probably not too much on its own
• we shouldn't send arbitrarily sized favicon data over IPC or keep them - a size limit of sorts would help (see also bug 1586083)
• we could potentially think about deduping these. If we know the favicon URL is the same we could keep the data in a better way - then at least if you have 10 of these tabs we only store the image once
• we could avoid storing these images for closed tabs, but this wouldn't fix the session store backup size explosion
• we could avoid storing the images altogether and "just" use the places DB for favicons. This would potentially lose favicons after a restart, especially if you clear history on shutdown - though I dunno if that currently works at all with session restore anyway.

Marco or Mike, do you have opinions on how to proceed to improve the situation here and/or how reliable places favicons would be for this usecase?

In the very short term, if you're involved in this site or know the mods or whatever, you could always ask them to fix their favicon to not be crazy. I mean, sure, this is a dumb bug on the Firefox side but it's probably not helping the performance of their website to send everyone a giant favicon, irrespective of what browser/device they use to look at the site...

Comment 5

[David Smith]

That's helpful to know. It also explains the other performance issue I was having — the massive inflation of RAM usage by the main process — as that seemed to be tied to the main process keeping dozens of copies (at least 50, at one point) of a data: image string that was about 3 MB long (as seen in about:memory). I had no way of determining where that entry in about:memory was coming from, but it definitely seems to match the favicon image.

Comment 6

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #4)

• we shouldn't send arbitrarily sized favicon data over IPC or keep them - a size limit of sorts would help (see also bug 1586083)

Potentially, we could dump the image to a properly sized canvas, where by properly sized I mean we should calculate which size we need to print a 16px image on the current dpi. For example on 2x dpi we'd dump the image to a 32px canvas. Then we'd store that, instead of the original image.
The favicon service does something similar, it only stores images of certain sizes and resizes too large ones.

• we could potentially think about deduping these. If we know the favicon URL is the same we could keep the data in a better way - then at least if you have 10 of these tabs we only store the image once

This is a potential improvement to the sessionstore format, I like it. It may indeed have a specific area for icons and refer to those through a uuid.

• we could avoid storing the images altogether and "just" use the places DB for favicons. This would potentially lose favicons after a restart, especially if you clear history on shutdown - though I dunno if that currently works at all with session restore anyway.

Does permanent PB store session? That is another case where icons may not be in the db, but if we don't restore in permanent pb then it's not a problem.
Anyway, we could maybe store icons only if history is disabled, otherwise just use a "page-icon:url" on restore. It would save resources for most users.

In the very short term, if you're involved in this site or know the mods or whatever, you could always ask them to fix their favicon to not be crazy.

512px is not completely crazy, imo, we don't use it but other applications may use it, for example to create an icon when storing the page as a web app. Anyway it's impossible to control the source in this case, we must be smart.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #6)

Anyway, we could maybe store icons only if history is disabled, otherwise just use a "page-icon:url" on restore. It would save resources for most users.

As a side note, there may be edge cases here for which the favicons service doesn't store an icon, chrome or about pages probably.