866444 - No Site favicon in History submenus – Recently Closed Tabs, Windows

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/0e45f1b9521f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130427 Firefox/23.0 ID:20130427030919

Steps To Reproduce:
1. Open Firefox
2. Open any site(has site favicon) in new tab
3. Close the tab
4. Menu - History – Recently Closed Tabs

Actual Results:
No site favicon

Expected Results:
Favicon should be shown in the list

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/51ecec435845
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130326 Firefox/22.0 ID:20130326135237
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd8efcbaf42b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130326 Firefox/22.0 ID:20130326144706
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=51ecec435845&tochange=dd8efcbaf42b

Suspected:
dd8efcbaf42b	Jonathan Kew — bug 828508 - use higher-res favicons for tab titles in hidpi mode if available. r=dao

Comment 1

[Jonathan Kew [:jfkthame]]

Similarly, favicons don't appear in the Recently Closed Windows submenu.

This may well turn out to be the same as bug 860578, but I won't dupe it for now until we understand the cause properly.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: fix missing favicons in Recently Closed Tabs/Windows menu items

This feels rather wallpaper-ish to me, but it seems to be adequate to restore the missing icons. They're still only low-res, but I think fixing that is dependent on the more general issue of support for hi-dpi favicons in Places (see bugs 798223 and 854956), so I'm not going to worry about it for now.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 744636 [details] [diff] [review]
fix missing favicons in Recently Closed Tabs/Windows menu items

>+      // Wallpaper for bug 866444:
>+      // strip -moz-resolution fragment from the favicon URL
>+      let tabImage = aTab.getAttribute("image").replace(/[#&]-moz-resolution=\d+,\d+/, "");
>+
>       this._windows[aWindow.__SSi]._closedTabs.unshift({
>         state: tabState,
>         title: tabTitle,
>-        image: aTab.getAttribute("image"),
>+        image: tabImage,
>         pos: aTab._tPos
>       });

image: tabbrowser.getIcon(aTab), ?

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: fix missing favicons in Recently Closed Tabs/Windows menu items

OK, that works too, thanks! Care to steal the review? :)

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 744646 [details] [diff] [review]
fix missing favicons in Recently Closed Tabs/Windows menu items

>@@ -2000,16 +2000,21 @@ let SessionStoreInternal = {
>       delete tabData.extData;
> 
>     if (history && browser.docShell instanceof Ci.nsIDocShell) {
>       let storageData = SessionStorage.serialize(browser.docShell, aFullData)
>       if (Object.keys(storageData).length)
>         tabData.storage = storageData;
>     }
> 
>+    // Wallpaper for bug 866444:
>+    // strip -moz-resolution fragment from the favicon URL
>+    if ("image" in tabData.attributes)
>+      tabData.attributes["image"] = tabData.attributes["image"].replace(/[#&]-moz-resolution=\d+,\d+/, "");
>+
>     return tabData;
>   },

I don't really understand this bug. Why is #-moz-resolution a problem here?

Comment 6

[Jonathan Kew [:jfkthame]]

I don't really know, TBH, but I assume it is confusing Places somehow. None of this code is actually familiar to me. :\

Given that this regressed in bug 828508, and the only thing that did was to add the -moz-resolution media fragment, stripping it back off when retrieving the URI seemed a likely fix. Basically, it's not supposed to be exposed to the outside world, it's there only for display purposes.

Comment 7

[Jonathan Kew [:jfkthame]]

Oops, submitted prematurely - I was going to add that the first chunk was sufficient to fix the favicons in Recently Closed Tabs, but they were still missing from Recently Closed Windows; this second chunk of the patch fixes that.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 744636 [details] [diff] [review]
> fix missing favicons in Recently Closed Tabs/Windows menu items
> 
> >+      // Wallpaper for bug 866444:
> >+      // strip -moz-resolution fragment from the favicon URL
> >+      let tabImage = aTab.getAttribute("image").replace(/[#&]-moz-resolution=\d+,\d+/, "");
> >+
> >       this._windows[aWindow.__SSi]._closedTabs.unshift({
> >         state: tabState,
> >         title: tabTitle,
> >-        image: aTab.getAttribute("image"),
> >+        image: tabImage,
> >         pos: aTab._tPos
> >       });
> 
> image: tabbrowser.getIcon(aTab), ?

Actually, this doesn't work as well, because if the tab has not been loaded yet (e.g. it's a restored-session tab that wasn't actually loaded), it won't get the icon, whereas retrieving it from the tab's image attribute will provide the proper icon even in this case.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: fix missing favicons in Recently Closed Tabs/Windows menu items

So, it really does work better like this.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Comment on attachment 744636 [details] [diff] [review]
> > fix missing favicons in Recently Closed Tabs/Windows menu items
> > 
> > >+      // Wallpaper for bug 866444:
> > >+      // strip -moz-resolution fragment from the favicon URL
> > >+      let tabImage = aTab.getAttribute("image").replace(/[#&]-moz-resolution=\d+,\d+/, "");
> > >+
> > >       this._windows[aWindow.__SSi]._closedTabs.unshift({
> > >         state: tabState,
> > >         title: tabTitle,
> > >-        image: aTab.getAttribute("image"),
> > >+        image: tabImage,
> > >         pos: aTab._tPos
> > >       });
> > 
> > image: tabbrowser.getIcon(aTab), ?
> 
> Actually, this doesn't work as well, because if the tab has not been loaded
> yet (e.g. it's a restored-session tab that wasn't actually loaded), it won't
> get the icon, whereas retrieving it from the tab's image attribute will
> provide the proper icon even in this case.

This sounds like a bug, e.g. sessionstore code setting the image attribute directly rather than calling tabbrowser.setIcon.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Dão Gottwald [:dao] from comment #10)

> This sounds like a bug, e.g. sessionstore code setting the image attribute
> directly rather than calling tabbrowser.setIcon.

I don't have much understanding of the sessionstore code, but it looks to me like the image attribute gets saved and restored directly due to being listed in SessionStoreInternal.xulAttributes at http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#253, so that it gets handled by attribute-copying code.

If that's a bug, maybe someone who actually understands the sessionstore code needs to take this and fix it properly. Or if no-one's available to do that at the moment, I'd suggest that for now we use the "wallpaper" here so as to fix the current regression.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 744673 [details] [diff] [review]
fix missing favicons in Recently Closed Tabs/Windows menu items

Review of attachment 744673 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should fix this the right way, as Dão suggested. The xulAttributes code is quite old and from a time where it probably was okay to handle the 'image' attribute directly.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use get/setIcon to restore tab icons and remove 'image' from xulAttributes

How does this patch work for you? I unfortunately don't have any HiDPI testing devices.

Comment 14

[Jonathan Kew [:jfkthame]]

(Doesn't apply cleanly on current m-c, fwiw. But it seemed trivial to merge, so I did a build with it to test...)

This doesn't handle the case of "restored" tabs that are closed without having been loaded (i.e. they were "pending", but then closed) -- they go into the Recently Closed Tabs submenu, but without the expected favicon (even though the favicon *was* shown).

I'd actually expect this problem to show up on all systems, not only hidpi hardware. If you open a new tab and load a site that has a custom favicon, then close the tab and look at the Recently Closed Tabs submenu, does the favicon show up?

Comment 15

[Jonathan Kew [:jfkthame]]

And to reproduce the problem even with your patch applied: restore a session with deferred tab loading, but don't actually visit all the tabs (so that they remain "pending"). Then right-click one of the (unloaded) tab titles, and close it. Check the Recently Closed Tabs submenu: the tab you closed will be at the top of the list, but its favicon is missing.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> I'd actually expect this problem to show up on all systems, not only hidpi
> hardware. If you open a new tab and load a site that has a custom favicon,
> then close the tab and look at the Recently Closed Tabs submenu, does the
> favicon show up?

It doesn't with trunk. With the patch this is fixed.

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> This doesn't handle the case of "restored" tabs that are closed without
> having been loaded (i.e. they were "pending", but then closed) -- they go
> into the Recently Closed Tabs submenu, but without the expected favicon
> (even though the favicon *was* shown).

I just tried it and it works for me with the patch applied. BTW, the patch should now cleanly apply to m-c, want to give it another try?

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Tim Taubert [:ttaubert] (gone from May 8th-10th) from comment #16)
> (In reply to Jonathan Kew (:jfkthame) from comment #14)
> > I'd actually expect this problem to show up on all systems, not only hidpi
> > hardware. If you open a new tab and load a site that has a custom favicon,
> > then close the tab and look at the Recently Closed Tabs submenu, does the
> > favicon show up?
> 
> It doesn't with trunk. With the patch this is fixed.

Agreed.

> (In reply to Jonathan Kew (:jfkthame) from comment #14)
> > This doesn't handle the case of "restored" tabs that are closed without
> > having been loaded (i.e. they were "pending", but then closed) -- they go
> > into the Recently Closed Tabs submenu, but without the expected favicon
> > (even though the favicon *was* shown).
> 
> I just tried it and it works for me with the patch applied. BTW, the patch
> should now cleanly apply to m-c, want to give it another try?

Yes, it applies cleanly now. However, I'm still seeing two issues:

1) Open a new window (not tab). Load a page with a favicon. Close the window. Result: the window is listed in History/Recently Closed Windows, but its favicon is missing.

2) Still not getting the favicon in Recently Closed Tabs if I close a "pending" tab from a newly-restored session. (This is dependent on having the "Don't load tabs until selected" option checked in Preferences - though I believe that's the default.)

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use get/setIcon to restore tab icons and remove 'image' from xulAttributes

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> 1) Open a new window (not tab). Load a page with a favicon. Close the
> window. Result: the window is listed in History/Recently Closed Windows, but
> its favicon is missing.

This patch contains a simple fix for it.

> 2) Still not getting the favicon in Recently Closed Tabs if I close a
> "pending" tab from a newly-restored session. (This is dependent on having
> the "Don't load tabs until selected" option checked in Preferences - though
> I believe that's the default.)

Strange, it works for me. Did you try to open new tabs and restart the browser after the patch has been applied? I think this might not work with the broken image URLs saved from previous sessions.

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 746375 [details] [diff] [review]
use get/setIcon to restore tab icons and remove 'image' from xulAttributes

Ah, looks better now - thanks. The Recently Closed Windows menu gets the expected icons, and the Tabs menu looks fine too.

You're right - the issue there was with tabs that had been saved by a "broken" version. So I can still reproduce the issue if I go back to current Nightly, load some tabs, save the session, quit; then launch the patched version, restore the session, and close a "pending" tab. But that's an edge case not worth worrying about; it's a cosmetic issue, most people will probably never see it, and it'll resolve itself naturally as sessions get updated.

Comment 20

[Jonathan Kew [:jfkthame]]

And as a bonus, this will let us fix bug 860578 more neatly, as Dao requested in comment 3 there.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 746375 [details] [diff] [review]
use get/setIcon to restore tab icons and remove 'image' from xulAttributes

Review of attachment 746375 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4656,5 @@
> +
> +    return data;
> +  },
> +
> +  set: function (tab, data) {

Maybe default argument |data = {}|?

::: browser/components/sessionstore/test/browser_attributes.js
@@ +22,5 @@
> +  yield whenBrowserLoaded(tab.linkedBrowser);
> +
> +  // Check that the tab has an 'image' attribute.
> +  ok(tab.hasAttribute("image"), "tab.image exists");
> +  ss.persistTabAttribute("image");

Nit: Shouldn't this line go below the following comment?

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> > +  set: function (tab, data) {
> 
> Maybe default argument |data = {}|?

Good idea, will use that.

> > +  // Check that the tab has an 'image' attribute.
> > +  ok(tab.hasAttribute("image"), "tab.image exists");
> > +  ss.persistTabAttribute("image");
> 
> Nit: Shouldn't this line go below the following comment?

Yes, thanks.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2b9336be5aa8

Comment 24

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/2b9336be5aa8

Comment 25

[Alex Keybl [:akeybl]]

(In reply to Tim Taubert [:ttaubert] from comment #23)
> https://hg.mozilla.org/integration/fx-team/rev/2b9336be5aa8

Ready for uplift Jonathan/Tim?

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to Alex Keybl [:akeybl] from comment #25)
> Ready for uplift Jonathan/Tim?

As far as I'm concerned, yes, but I'd prefer Tim to nominate his patch, as I'm not comfortable trying to comment on the level of risk here in unfamiliar code.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 746375 [details] [diff] [review]
use get/setIcon to restore tab icons and remove 'image' from xulAttributes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 828508
User impact if declined: Missing favicons.
Testing completed (on m-c, etc.): Landed a couple of days ago, no known regressions.
Risk to taking this patch (and alternatives if risky): The fix itself is very self-contained. It is low-risk although it contains a little cleanup other than just the pure fix. Also it contains a test for previously untested behavior so I think this safe to take.
String or IDL/UUID changes made by this patch: None.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0309cdbeeb00
https://hg.mozilla.org/releases/mozilla-beta/rev/2e0069a5ee61

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta for mochitest b-c failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/f7a70a0efeec

https://tbpl.mozilla.org/php/getParsedLog.php?id=23284186&tree=Mozilla-Beta

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Re-landed with a test suite fix. Try run was green before pushing.

https://hg.mozilla.org/releases/mozilla-beta/rev/27864d643e24

Comment 31

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0

Verfied as fixed in Firefox 22 beta 3 (buildID: 20130528181031) and latest Nightly (buildID: 20130529031131).

Comment 32

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed in Firefox 23 beta 2 (build ID: 20130701144430)

User Agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0

Comment 33

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

Verified as fixed on Firefox 24 beta 1 (buildID: 20130806170643).