1724208 - menu History > Recently Closed Tabs/Windows: favicons not shown

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Latest Firefox 92.0a1 (20210805092724) on Windows 8.1

Bisection confirmed it's a regression from bug 1546847.

Menu "History" > "Recently Close Tabs"/Windows doesn't show the pages' favicons anymore. The frequently visited pages shown in the "History" have their favicons, even if they are the same pages.

Steps to reproduce:

1. Launch Firefox.
2. Open a new tab.
3. Open a page with a favicon, e.g. https://crash-stats.mozilla.org/report/index/e1bbee9b-743a-4353-bbf4-fe72b0210805#allthreads
4. Close the tab.
5. Either press the Alt key and open the menu "History" > "Recently Closed Tabs" or do it from the menu button.

Comment 2

[Shane Hughes [:aminomancer]]

Here's a quick patch if anyone wants to submit it: (I can't at the moment) https://paste.mozilla.org/BtcYn29t

I think there might be something else going on though. I can't tell because it seems random, but SessionWorkerCache.getById(aItem.image) is returning undefined for more tabs than I would expect. I think the cache process might be skipping some tabs/sites for some reason. However, I clear the startup cache frequently when I'm working on this kind of stuff so maybe that's the reason. I don't have any objective way to evaluate this, I'm just seeing more default favicons in the list than I'm used to. So that's just a suspicion.

Comment 3

[:Gijs (he/him)]

Attachment: WIP: Bug 1724208 - fix missing images in recently closed tab menu, r?dthayer

Comment 4

[:Gijs (he/him)]

! In D121876#3962477, @Gijs wrote:
This is the patch that aminomancer linked, but as noted in the bug it doesn't work for chrome: URLs, and I haven't yet looked at why that is.

From a brief look at the patch and at the regressing patch, I think the issue might be that we clear out the icon URL and tell the worker cache to release the reference when the tab is removed. I don't think this is safe. Doug/Kashav, should we just... not expire items in the cache? Or perhaps make session store responsible for it, if/when it evicts items from the session store "recently closed windows/tabs" collections, and/or when private browsing windows close?

Comment 5

[:Gijs (he/him)]

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

! In D121876#3962477, @Gijs wrote:
This is the patch that aminomancer linked, but as noted in the bug it doesn't work for chrome: URLs, and I haven't yet looked at why that is.

From a brief look at the patch and at the regressing patch, I think the issue might be that we clear out the icon URL and tell the worker cache to release the reference when the tab is removed. I don't think this is safe. Doug/Kashav, should we just... not expire items in the cache? Or perhaps make session store responsible for it, if/when it evicts items from the session store "recently closed windows/tabs" collections, and/or when private browsing windows close?

oops, +needinfo

Comment 6

[:Gijs (he/him)]

This should be fixed by backout of bug 1546847. Aryx, can you confirm?

Comment 7

[Hsin-Yi Tsai (she/her)[:hsinyi]]

The bug was closed 3 years old by backing out the regressor and doesn't seem to require any action, so clearing needinfo.