Don't store favicons added after FaviconLoader::onPageShow
Categories
(Toolkit :: Places, task, P2)
Tracking
()
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file)
|
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68-
|
Details | Review |
We should store any favicons that are set initially on the page, but then we should stop storing them, icons set through javascript by modifying <link> tags are good for tabs, not for permanent storage.
There is one problem though, a few tests depend on this behavior.
Alternatively, we could decide to stop storing icons after a certain time threshold.
The scope is to avoid storing icons modified just to show a badge.
|
Assignee | |
Comment 1•6 years ago
|
||
Ok, I think we can use the onPageShow() handler of the FaviconLoader the threshold, from that point on any added icon should not be stored, while any icons added before were in the <head>
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 2•6 years ago
|
||
Icons added after the initial parsing are likely randomly generated to show badges,
thus they are not good for permanent storage, because they are transient and can
potentially flood the store.
|
||
Comment 4•6 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 5•6 years ago
|
||
Comment on attachment 9112607 [details]
Bug 1600244 - Don't store favicons added after the initial page load. r=Mossop
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: The result of storing many of these icons is that in certain cases (like the one in Bug 1598371) the database becomes so large that Firefox performance is affected (temporary freezes) due to I/O.
- User impact if declined: Browser may visibly freeze a moment on page loads for heavy users of websites using badged favicons
- Fix Landed on Version:
- Risk to taking this patch: Medium
- Why is the change risky/not risky? (and alternatives if risky): The biggest risk is that we may stop storing favicons for some sites, if they use js tricks to set their favicons.
- String or UUID changes made by this patch:
Beta/Release Uplift Approval Request
- User impact if declined: Browser may visibly freeze a moment on page loads for heavy users of websites using badged favicons.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce: The automated test should be sufficient.
- List of other uplifts needed: None
- Risk to taking this patch: Medium
- Why is the change risky/not risky? (and alternatives if risky): The biggest risk is that we may stop storing favicons for some sites, if they use js tricks to set their favicons.
- String changes made/needed:
|
||
Comment 6•6 years ago
|
||
Do we think or have evidence that this is common enough to warrant jumping the trains?
|
|
Assignee | |
Comment 7•6 years ago
|
||
We have a report from Yandex that this is surely a problem for everyone using yandex mail (see bug 1598371), other mail systems may have similar problems. I don't have statistics though, mostly guessing based on the kind of problem.
|
|
||
Comment 8•6 years ago
|
||
Comment on attachment 9112607 [details]
Bug 1600244 - Don't store favicons added after the initial page load. r=Mossop
favicons fix, approved for 72.0b5. I think this needs to go after bug 1600242.
|
|
||
Comment 9•6 years ago
|
||
| bugherder uplift | ||
|
||
Comment 10•6 years ago
|
||
Comment on attachment 9112607 [details]
Bug 1600244 - Don't store favicons added after the initial page load. r=Mossop
Per bug 1600242.
|
|
||
Updated•6 years ago
|
Description
•