1600242 - Stop storing favicons having Cache-Control: no-store

Status: RESOLVED FIXED

(Toolkit :: Places, task, P1)

Description

[Marco Bonardo [:mak]]

We should skip those icons, there's clearly the intent to make them transient.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1600242 - Stop storing favicons having Cache-Control: no-store. r=Mossop

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c37d1f725bb2
Stop storing favicons having Cache-Control: no-store. r=mossop

Comment 3

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c37d1f725bb2

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 9112558 [details]
Bug 1600242 - Stop storing favicons having Cache-Control: no-store. r=Mossop

Beta/Release Uplift Approval Request

• User impact if declined: 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.
• 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, but in that case it's a server side issue, since those should not be served with this header.
• String changes made/needed:

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:

Comment 5

[Julien Cristau [:jcristau]]

Comment on attachment 9112558 [details]
Bug 1600242 - Stop storing favicons having Cache-Control: no-store. r=Mossop

favicons fix, approved for 72.0b5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b452a50fccba

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9112558 [details]
Bug 1600242 - Stop storing favicons having Cache-Control: no-store. r=Mossop

I'm having a hard time justifying taking this change for ESR given what we know here. Seems like a pretty risky change to take for that release. Let's let this ride the normal trains. If you feel strongly otherwise, LMK. Note that it'll need a rebased patch too (as will bug 1600244 I suspect).