Closed Bug 1504470 Opened Last year Closed Last year

Firefox hangs when loading favicon from this website

Categories

(Firefox :: Tabbed Browser, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- verified
firefox66 --- verified

People

(Reporter: rodrigo.mcunha, Assigned: mossop)

References

(Blocks 1 open bug)

Details

(Keywords: perf, regression)

Attachments

(5 files)

Attached file about:support
STR:
1. Open the website at Vizer.Gratis.
2. Search something in the search bar and move your cursor through the movie posters to provoke a noticeable hang, you'll notice that memory starts leaking.

Performance capture: https://perfht.ml/2JHWSFH

Info about my Firefox setup is at the aboutsupport.txt file.
Looking at the profile posted, in the first long hang in the main thread, the stack has image decoding and favicon code. The JS-only stack is:

loadFavicon
loadFavicon
setIconFromLink
receiveMessage


According to the reporter, the memory also starts leaking (not sure what is meant by that).

I wonder if this website is using a giant data URL or something like that. The website is down at the moment (I'm getting Cloudflare errors from it), so I can't test it at the moment
Component: General → Tabbed Browser
Summary: Firefox hangs and has memory leak in Vizer.Gratis website → Firefox hangs when loading favicon from this website
About the memory leaking part... I actually meant that memory usage started sky rocketing, it went up to 4GB on one of the times I've tested.
I'm not seeing any hangs from this site or any suggestion that mousing over the page is causing us to detect new favicons. Not sure how to reproduce this.
Yeah, the website is back again, but I couldn't reproduce the issue either.

Rodrigo, could you check the following:

- is the problem still happening? (maybe they fixed this on their end)
- do you need to be logged in for it to happen

If it still happens, can you go to about:memory and click "Measure and Save" to get a memory usage report?
Flags: needinfo?(rodrigo.mcunha)
Attached file memory-report.json.gz
The issue is definitely still present. Also, no I'm not logged in to the website.

I've gone ahead and save a memory measurement, which I've attached here.
Flags: needinfo?(rodrigo.mcunha)
Thanks for the report! It's showing a number of interesting things.. It shows that most of the memory is being taken by the webextensions process, but that *might* be just a symptom of the problem.

It looks like that website is somehow generating a 14513x14513 image stored as a 7MB data: URL, probably for the favicon (given the profile posted before). And that favicon gets copied and passed around several times (probably due to some webextensions API), so it ends up consuming a lot of memory.

I don't know why the website doesn't do the same for me. I wonder if it's triggered by one of your add-ons

Could you run Firefox in Safe Mode (about:support -> Restart with Add-ons Disabled) to see if the problem persists there? Please take another about:memory report when doing that so we can compare the two.
Attached file memory-report2.json.gz
Tried with Safe Mode, the memory usage seems to be way lower but it still reaches ~1.4GB when it was at around ~500MB when I first opened the page.

Here's the memory measurement for this try.
Still has the same 14513x14513 image in a data URL. Just to be sure could you look at the page source and look for 'link rel="icon"'. On my end I see a normal (short, https) URL for a 32x32 icon, and another for a 192x192 icon, but no massive "data:image" URL like we see in your about:memory report. Can you verify that the "data:image/png;base64,iVBORw0KGgoA" string shows up on the page for you?
Flags: needinfo?(rodrigo.mcunha)
These are the ones I see when I open the page source:
<link rel="icon" href="https://vizer.gratis/wp-content/uploads/2018/07/cropped-512-32x32.png" sizes="32x32" />
<link rel="icon" href="https://vizer.gratis/wp-content/uploads/2018/07/cropped-512-192x192.png" sizes="192x192" />
<link rel="apple-touch-icon-precomposed" href="https://vizer.gratis/wp-content/uploads/2018/07/cropped-512-180x180.png" />
<meta name="msapplication-TileImage" content="https://vizer.gratis/wp-content/uploads/2018/07/cropped-512-270x270.png" />

"data:image/png;base64,iVBORw0KGgoA" does not show up on the page though.
Flags: needinfo?(rodrigo.mcunha)


STR:
0. Start W/ new profile (not WebRender)
1. Install ublock origin (prevent window popup DoS)
2. Load https://vizer.gratis/
3. Close ads? with x button and then click FECHAR
4. Restart browser
5. Load https://vizer.gratis/ again
6. During page is loading, Try scroll up/down with mouse wheel or open browser menu etc

Actual Results:
Browser becomes unresponsive. After that, browser will respond.


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=52baefdfbeca055fccb7518db3fd51ceea0649dc&tochange=40ed437da7ae4407ece2ad47cd8fe3c7943f9eb9

Regressed by: 	40ed437da7ae	Dave Townsend — Bug 1453751: Load favicons in the content process
Flags: needinfo?(dtownsend)
Keywords: steps-wantedperf
Priority: -- → P2
I do see a reference to an icon which size is 14513 x 14513 when looking at the source[1]:

<link rel="shortcut icon" href="https://vizer.gratis/wp-content/uploads/2018/07/512.png" type="image/x-icon" />

***

[1] view-source:https://vizer.gratis/
I think I've figured this out. The page does indeed include a link to a very large png file as an icon (incorrectly marked in the source as an ico file), but there are some later icons that get selected in preference but only on retina displays. That is why me and probably others weren't seeing this.

The actual hang comes not from displaying the icon in the tab but from storing it in the favicon service. Previous to bug 1453751 the favicon service would be given the icon's url to load, start loading it asynchronously and then bail when it found it was too large: https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/toolkit/components/places/FaviconHelpers.cpp#682

Since bug 1453751 however we now load the data into a data uri then pass it to the favicon service directly and the favicon service does not in this case do any kind of size check. What it does do is synchronously decode the icon (bug 1346139) which is the source of the hang.

The straightforward fix for the hang is to make ReplaceFaviconDataFromDataURL do a better size check on the data uri contents (https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/toolkit/components/places/nsFaviconService.cpp#561) but that might have the effect of breaking some other cases that I'm not aware of. The harder fix is to do icon size optimization asynchronously.

As for using an extra 7MB of memory for the data uri I can't think of a way around that given how favicon loading now works. Unless we wanted to block all favicons above a certain size which would break tab icons for this site and probably some others.

What are your thoughts Marco?
Flags: needinfo?(dtownsend) → needinfo?(mak77)
(In reply to Dave Townsend [:mossop] (he/him) from comment #12)
> The straightforward fix for the hang is to make
> ReplaceFaviconDataFromDataURL do a better size check on the data uri
> contents

To save having to pass the large string through xpconnect, it would be better if we could not even try to store a large data uri, can we do that outside of the favcicon service, in the loading code? I'd prefer to keep the favicon service as a simple store, so that when things are stored they have been approved already by the code making the call.

> but that might have the effect of breaking some
> other cases that I'm not aware of. 

there shouldn't be side problems if we handle the exception properly (so if ReplaceFaviconDataFromDataURL throws we don't invoke setAndFetchFavicon...)

The harder fix is to do icon size
> optimization asynchronously.

That would help in general, but in this case it's clear there is a server side mistake, we should not cover it.

> As for using an extra 7MB of memory for the data uri I can't think of a way
> around that given how favicon loading now works. Unless we wanted to block
> all favicons above a certain size which would break tab icons for this site
> and probably some others.

And I suspect with a decent limit it wouldn't be that bad, we could still use other icons from the page, and it would be a tech evangelism problem. I'm sure nobody intended to set a 14k pixels icon, it was a mistake and by not doing anything we are covering a site bug.

My opinion is that we should indeed limit data uris for favicons to a maximum size, don't show/store them. This may help web owners to identify the problem. Long term we still need to properly move image optimization off the main thread.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #13)
> > As for using an extra 7MB of memory for the data uri I can't think of a way
> > around that given how favicon loading now works. Unless we wanted to block
> > all favicons above a certain size which would break tab icons for this site
> > and probably some others.
> 
> And I suspect with a decent limit it wouldn't be that bad, we could still
> use other icons from the page

That would be challenging to implement. We currently decide which icon to use based entirely on the contents of the link tag, we don't open a channel to the icon first. This does cause other problems too like with this site that claims the icon is an ico file when in fact it is a png.

Might be worth at some point in the future though if the attempt to load the selected icon fails in some way to backtrack and select the next best option and try again.
Assignee: nobody → dtownsend
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3b16bba67c
Switch to using a storage stream for caching favicon data. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8824f8d86c9
Drop favicons above 2048x2048. r=mak
https://hg.mozilla.org/mozilla-central/rev/4f3b16bba67c
https://hg.mozilla.org/mozilla-central/rev/a8824f8d86c9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify+

I successfully reproduced the issue on Firefox Nightly 65.0a1 (2018-11-03) under Windows 10 (x64) using the STR from Comment 10.

The issue is no longer reproducible on Firefox Beta 65.0b12 and latest Nightly 66.0a1 (2019-01-18). Tests were performed under Windows 10 (x64), macOS 10.12 and Ubuntu 18.04 (x64).

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.