1119455 - Favicon gets discarded, re-decoded and repainted when moving the mouse pointer over a tab

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

If I (or the web server I'm running) return a large image as a favicon, it seems to get thrown out and re-downloaded whenever we mouse over the tab.

Comment 1

[:Gijs (he/him)]

I wonder if this is the same as bug 1121802 (per its comment #0). Seth?

Comment 2

[Seth Fowler [:seth] [:s2h]]

It sounds related, but I haven't investigated this case - I've mostly been looking at image documents. If bug 1121802 does not fix the issue, I'll take another look.

Comment 3

[Seth Fowler [:seth] [:s2h]]

I investigated, and it looks like bug 1121802 doesn't fix this. The favicon code seems to be bypassing the cache for some reason.

We need to fix this.

Comment 4

[Timothy Nikkel (:tnikkel)]

What's happening here is there is a restyle in the browser chrome that requires frame reconstruct, the frame needing a reconstruct is a parent of the XUL <image> element holding the favicon. When we destroy the nsImageBoxFrame for the favicon we drop the reference to it's imgRequestProxy, which is the last reference to that specific proxy, in it's destructor it unconditionally Unlocks the image, which means the image can be discarded. And I guess things get discarded synchronously and immediately these days because when the frame is reconstructed we have to re-decode the image again.

HTML <img> elements and css images like background images don't suffer from this problem because the images are held on the content nodes and the style system respectively, so when the frame goes away the image is still held onto.

We could write code to make sure the image stays around in this kind of case, but it would be better to migrate this away from XUL and use standard HTML instead of spending resources on XUL. Is it feasible to make favicons not use xul <image>?

Comment 5

[:Gijs (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> We could write code to make sure the image stays around in this kind of
> case, but it would be better to migrate this away from XUL and use standard
> HTML instead of spending resources on XUL. Is it feasible to make favicons
> not use xul <image>?

In principle, "yes", because we just need it to display an image, so we can use HTML <img> elements instead.

... but we'd need to make them and the tabs behave "properly", which seems to not be trivial. XUL images have interesting properties in terms of how they size depending on what their src is - if I just try the obvious trivial patch to replace them with html:img elements instead, I end up with outsized icons on e.g. https://newsblur.com/ , and even trying to fix the icons' height and width to 16px in CSS or using attributes in the XBL binding only succeeds in curtailing the height - the width remains stretched, no matter what incantation of max-width: 16px !important; I throw at it. :-\

(and this is still ignoring the complication of needing to hide the image completely when there is no favicon etc. ... right now, while the tab is loading, the tab's contents jumble about seemingly randomly when I try the aforementioned "obvious" approach)

Is there something obvious I'm missing that would make this simpler?

Comment 6

[Dão Gottwald [:dao]]

Attachment: use background-image

(In reply to :Gijs Kruitbosch from comment #5)
> height and width to 16px in CSS or using attributes in the XBL binding only
> succeeds in curtailing the height - the width remains stretched, no matter
> what incantation of max-width: 16px !important; I throw at it. :-\

Pretty sure this is because of the icon's parent -moz-box frame. I'm guessing it asks its children for their preferred width and somehow this ends up being the img's natural width rather than what you specified.

The cleanest way to avoid these kind of issues would be to make the whole tab strip HTML, which is of course no easy task.

We could also set style.backgroundImage instead of the src attribute (see patch). With bug 435426 we could also just use background-image: url(attr(image url));.

Comment 7

[Timothy Nikkel (:tnikkel)]

Sounds like the issues have more to do with the specific markup than platform features, and/or fixing the platform features would require more time be put into XUL on the platform side, so marking this needinfo answered.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Timothy Nikkel (:tnikkel) from comment #7)
> Sounds like the issues have more to do with the specific markup than
> platform features,

Not really. XUL images just aren't behaving sane here.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to Timothy Nikkel (:tnikkel) from comment #7)
> > Sounds like the issues have more to do with the specific markup than
> > platform features,
> 
> Not really. XUL images just aren't behaving sane here.

You cut off the rest of that sentence where I blame the platform XUL code. I was specifically replying to Gijs needinfo request, and my comment should be read in that context.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Timothy Nikkel (:tnikkel) from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > (In reply to Timothy Nikkel (:tnikkel) from comment #7)
> > > Sounds like the issues have more to do with the specific markup than
> > > platform features,
> > 
> > Not really. XUL images just aren't behaving sane here.
> 
> You cut off the rest of that sentence where I blame the platform XUL code.

Right, I only disagree with the part that I quoted.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Timothy Nikkel (:tnikkel) from comment #9)
> > (In reply to Dão Gottwald [:dao] from comment #8)
> > > (In reply to Timothy Nikkel (:tnikkel) from comment #7)
> > > > Sounds like the issues have more to do with the specific markup than
> > > > platform features,
> > > 
> > > Not really. XUL images just aren't behaving sane here.
> > 
> > You cut off the rest of that sentence where I blame the platform XUL code.
> 
> Right, I only disagree with the part that I quoted.

Well then I disagree with your disagreement. If the tab bar weren't implemented in xul then Gijs wouldn't have seen those problems, hence my original statement is correct.

Comment 12

[Dão Gottwald [:dao]]

Oh, you mean you replied to Gij's question about HTML image sizing in a -moz-box parent. I think that's also a legitimate platform bug. Sure, that bug disappears if nobody ever uses -moz-box, but that's not the reality we live in.

Comment 13

[Timothy Nikkel (:tnikkel)]

Right. The question is: should we invest more into making XUL work, or invest into moving away from XUL?

Comment 14

[Dão Gottwald [:dao]]

For this particular manifestation it's not worth investing, since we can work around it easily enough. But platform bugs can manifest in many ways, so I think it's worth taking a closer look. Haven't we just started supporting -webkit-box-* properties as aliases to -moz-box-*?

Comment 15

[:Gijs (he/him)]

Dão, is that patch ready for review or does it need more work, or would you prefer not to take it (in which case, why)? Any edgecases we need to worry about?

We could file a followup bug to investigate the Core issue here (I tried briefly to reproduce with an HTML div with display: -moz-box in a JSBin thing and didn't manage, but I didn't try very hard).

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Dão Gottwald [:dao] from comment #14)
> For this particular manifestation it's not worth investing, since we can
> work around it easily enough. But platform bugs can manifest in many ways,
> so I think it's worth taking a closer look. Haven't we just started
> supporting -webkit-box-* properties as aliases to -moz-box-*?

From bug 1208635 it sounds like -webkit-box is an alias to display: flex, which is completely separate code.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Timothy Nikkel (:tnikkel) from comment #16)
> (In reply to Dão Gottwald [:dao] from comment #14)
> > For this particular manifestation it's not worth investing, since we can
> > work around it easily enough. But platform bugs can manifest in many ways,
> > so I think it's worth taking a closer look. Haven't we just started
> > supporting -webkit-box-* properties as aliases to -moz-box-*?
> 
> From bug 1208635 it sounds like -webkit-box is an alias to display: flex,
> which is completely separate code.

I meant -webkit-box-orient, -webkit-box-flex and friends. Whether those have anything to do with Gijs' bug I don't know. I'm just speculating here because I haven't looked into what Gecko is actually doing here and I'm not the right person to do so either.

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Dão Gottwald [:dao] from comment #17)
> I meant -webkit-box-orient, -webkit-box-flex and friends. Whether those have
> anything to do with Gijs' bug I don't know. I'm just speculating here
> because I haven't looked into what Gecko is actually doing here and I'm not
> the right person to do so either.

I think this is bug 1262049. A quick reading of that bug tells me that Daniel wrote code so that the modern flex box implementation can handle legacy -moz-box properties (instead pretending the -webkit prefixed properties wanted the modern semantics) and then mapped -webkit-box-orient -webkit-box-flex to these -moz properties so that -webkit-box prefixed things were handled more like webkit for web compat reasons. He didn't touch the old XUL code for -moz-box (we don't ever want to expose that code to remote content ever again). Putting time into xul:image elements has no upside as far as web compat or web authors because XUL elements aren't allowed to be created on remote content.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> the old XUL code for -moz-box (we don't ever want to expose that code to
> remote content ever again).

But display:-moz-box is exposed to web content as far as I can tell.

> Putting time into xul:image elements has no
> upside as far as web compat or web authors because XUL elements aren't
> allowed to be created on remote content.

The original bug is about xul:image, but Gijs' issue was about html:img in a -moz-box container.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8745295 [details] [diff] [review]
use background-image

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

Generally this looks like it works fine (tested on OS X), but I did notice one small bug:

0. set Firefox up to automatically (but lazily) restore sessions if you haven't already;
1. open new tab;
2. load http://www.gijsk.com/ (could be any website with no favicon - I noticed this because it happened to be open)
3. select another tab (ie make sure that the favicon-less-tab is not the selected tab)
4. close browser
5. reopen browser
6. switch to the tab with no favicon you opened earlier

Now the throbber appears on the tab as the page loads, but when the page finishes loading, the 'space' for the throbber does not disappear, leaving only blank space behind, and causing the title of the tab to be misaligned.

::: browser/base/content/tabbrowser.xml
@@ +886,5 @@
>                  browser.currentURI, aURI, false, loadType, null, loadingPrincipal);
>              }
>  
> +            let iconURL = browser.mIconURL || "";
> +            aTab._setIconURL(iconURL);

Shouldn't this be inside the if statement ?

::: browser/themes/osx/browser.css
@@ +2532,5 @@
>      background-image: url(chrome://browser/skin/tabbrowser/[e-mail]);
>    }
>  
>    .tab-icon-image {
> +    background-image: url("chrome://mozapps/skin/places/[e-mail]");

Do we actually use these anymore? I never see them - we hide the favicon for pages with no favicon, right? Should we just get rid of this rule and its lodpi equivalent in tabs.inc.css, or am I missing something?

Comment 21

[:Gijs (he/him)]

Attachment: Patch to show wonky <html:img> issues

(In reply to Dão Gottwald [:dao] from comment #19)
> > Putting time into xul:image elements has no
> > upside as far as web compat or web authors because XUL elements aren't
> > allowed to be created on remote content.
> 
> The original bug is about xul:image, but Gijs' issue was about html:img in a
> -moz-box container.

Yes. Here's a trivial test patch you can use to see the problem. It'll show up on any hidpi osx machine where we use hidpi favicons for e.g. about:preferences, or by going to sites that use > 16x16 favicons like jsbin.

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #20)
> > +            let iconURL = browser.mIconURL || "";
> > +            aTab._setIconURL(iconURL);
> 
> Shouldn't this be inside the if statement ?

It could be but it's not a very useful optimization. The if-statement is mostly good for the _tabAttrModified call.

> >    .tab-icon-image {
> > +    background-image: url("chrome://mozapps/skin/places/[e-mail]");
> 
> Do we actually use these anymore?

For pinned tabs, yes.

Comment 23

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #20)
> Comment on attachment 8745295 [details] [diff] [review]
> use background-image
> 
> Review of attachment 8745295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally this looks like it works fine (tested on OS X), but I did notice
> one small bug:
> 
> 0. set Firefox up to automatically (but lazily) restore sessions if you
> haven't already;
> 1. open new tab;
> 2. load http://www.gijsk.com/ (could be any website with no favicon - I
> noticed this because it happened to be open)
> 3. select another tab (ie make sure that the favicon-less-tab is not the
> selected tab)
> 4. close browser
> 5. reopen browser
> 6. switch to the tab with no favicon you opened earlier
> 
> Now the throbber appears on the tab as the page loads, but when the page
> finishes loading, the 'space' for the throbber does not disappear, leaving
> only blank space behind, and causing the title of the tab to be misaligned.

The onerror attribute on the tab is supposed to handle this case, but my patch breaks this. So I don't think we can use background-image after all...

Comment 24

[:Gijs (he/him)]

:tn, what can we do about the HTML:img case from the other patch?

Comment 25

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #24)
> :tn, what can we do about the HTML:img case from the other patch?

FWIW, I'd actually prefer if xul:image was fixed. XUL alone is fragile enough, but the behavior of HTML in XUL is even worse, neither specified nor understood by anyone, so there might be more weirdness we don't know about yet. We'll eventually want to make the whole tab strip HTML, but starting with a tiny piece and mixing that with the legacy layout doesn't seem like a great idea to me.

Comment 26

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #20)
> > >    .tab-icon-image {
> > > +    background-image: url("chrome://mozapps/skin/places/[e-mail]");
> > 
> > Do we actually use these anymore?
> 
> For pinned tabs, yes.

Possibly also for the tab overflow menu

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to :Gijs Kruitbosch (back May 3rd) from comment #24)
> :tn, what can we do about the HTML:img case from the other patch?

Reduce the testcase so the problem can be pinned on XUL or non-xul (much more likely to get attention)?

Comment 28

[Justin Dolske [:Dolske]]

Random thought: would it be worth considering using <html:canvas> instead of <html:img> (and draw whatever image we have into it)? That would seem sidestep issues of getting the wrong size / scaling, because the canvas size would be static. It would obviously mean that we'd need some JS code to handle watching image loads and doing the drawImage() calls...

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to Justin Dolske [:Dolske] from comment #28)
> Random thought: would it be worth considering using <html:canvas> instead of
> <html:img> (and draw whatever image we have into it)? That would seem
> sidestep issues of getting the wrong size / scaling, because the canvas size
> would be static. It would obviously mean that we'd need some JS code to
> handle watching image loads and doing the drawImage() calls...

As the code is currently, DrawImage would always draw the largest resource in an ico file. As opposed to picking resource that is larger, but closest in size as we would do if an img element was used.