Not just cause they take space slowing down any other db access, also cause that may help with future support of larger icons. We should definitely investigate different storage options for favicons, or even let the cache handle them, like thumbnails.

Following is a Web paage which can be found by Google Search for "sqlite size limit". > https://www.sqlite.org/limits.html > The current implementation will only support a string or BLOB length up to 231-1 or 2147483647 Because SQLite saves a row as a BLOB, size limit of a BLOB column is smaller than it. Is there any plan to use favicon larger than 2GB? From perspective of API, simplest/easiest one is localStorage. localStorage is constructed on mozStorage in Firefox, and Table column is "scope, key, value" only, and scope(==URI of site) is hidden by Firefox, and program can update key/value pair only using setItem(key,vlaue)/removeItem(key). (other API is getItem(key), clear() only). value is TEXT, so, if JavaScript object, JSON.stringify is needed, and escape, base64 ecoding etc. is needed if binary data(data: ,is bse64). If icon data: size is less than size limit of localStorage, localStorage is most easy-to-use one for "per site data". This is becuse localStorage is enhancement of Cookie which is "per site data". Second one is IndexedDB. This is also a well-designed wrapper of mozStorage. Bigest difference from localStorage is "API is asynchrounus". Third one is mozStorage, and 4-th one is "direct use of SQLite by code himself.. What is size of "future support of larger icons"? Even if it exceeds "vlaue .size limit of localStorage", following is pretty easy. var icondata = base64 data od data: URI. KeyRoot="favicon"; var remain=icondata lenth; var block=65534; var Strt=0; var CNT=0; while(0<remain){ CNT++; var Key=KeyRoot+CNT;locaalStorage.setItem(Key,icondata.substr(Start,Math.min(remain,block)); remain=remain-block; start=start+block; } As far as data size is less than value size limit of SQLite DB column, "writing data to a column of SQLite DB" is one of simplest way of "safest file writing". localStorage.setItem8Key,JSON.strigify(Obj)) is far better than "safe file writing of JSON file". Even if data size is larger than value size limit of SQLite DB column, "split it to multiple pieces" is not so difficult. Where are you planning to move favicon data? I think localStorge is best place to hold "per site data" which is irrelevant to history relevant data. If XUL JavaScript, following can be used. // Directly access from XUL with any URL var ios = Components.classes["@mozilla.org/network/io-service;1"] .getService(Components.interfaces.nsIIOService); var ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"] .getService(Components.interfaces.nsIScriptSecurityManager); var dsm = Components.classes["@mozilla.org/dom/storagemanager;1"] .getService(Components.interfaces.nsIDOMStorageManager); get_localStorage = function (Site) { var uri = ios.newURI(Site, "", null); var principal = ssm.getCodebasePrincipal(uri); var localStorage = dsm.getLocalStorageForPrincipal(principal, ""); return localStorage; } var URI = "favicon://www.example.com"; // traslate http://www.example.com for favicon saving. set as scope value var localStorage=get_localStorage(URI ) ; localStorage.setItem("favicon", data of data: uri) ; localStorage.setItem("favicon-save-time", (new Date()).toString() );

(In reply to WADA from comment #1) > Because SQLite saves a row as a BLOB, size limit of a BLOB column is smaller > than it. > Is there any plan to use favicon larger than 2GB? Nope. > From perspective of API, simplest/easiest one is localStorage. localStorage is main-thread only, we want to do any favicon work off the main thread. > Second one is IndexedDB. This is also a well-designed wrapper of mozStorage. > Bigest difference from localStorage is "API is asynchrounus". Yes that is also a possibility, but far more complex than we need. What we need is still to have all the blobs in a single place, because we have UI views that show many favicons at once, so having blobs separated (like in a file system circle buffer) would be too expensive (opening and managing file handles is expensive). So the idea we somehow coalesced to is to move favicons to a separate sqlite database, and ATTACH DATABASE it to places.sqlite. IT will look like part of places.sqlite, but won't cause fragmentation and size issues to it.

FYI. moz_favicons in places.sqlite. > CREATE TABLE moz_favicons ( id INTEGER PRIMARY KEY, url LONGVARCHAR UNIQUE, data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT) guid is shared with moz_bookmarks? > CREATE TABLE moz_bookmarks ( id INTEGER PRIMARY KEY, type INTEGER, fk INTEGER DEFAULT NULL, parent INTEGER, position INTEGER, > title LONGVARCHAR, keyword_id INTEGER, folder_type TEXT, dateAdded INTEGER, lastModified INTEGER, guid TEXT) Mapping to localStorage.is pretty simple. moz_favicons + id + url => scope in localStorage, data => set of { key="favicon#"+1 to 2001,value=part#NN of base64 icon data }, or key="favicon",value=base64 icon data if Segments=1 + key="Segments",value=2001 + key="Size",value=2GB+800KB, key="BlockSize",value=1MB + key="LastSize",value=800KB + key="guid",value=guid for me mime-type => key=mime-type,value=mime-type of this icon If guid is shared with moz_bookmarks. scope = "favicon://" + "moz_bookmarks" + ".guid" + ":9090" key=guid value,value=moz_favicons + id + url (pointer to scope for favicon)

(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #2) > > From perspective of API, simplest/easiest one is localStorage. > localStorage is main-thread only, we want to do any favicon work off the main thread. What do you call "main-thread"? Although localStorage is defined for JavaScript in HTML of Web site, there is no limitation of localStoraage(==webappsstore.sqlite) use in Fx. "localStorage" is one of best Wrapper of mozStorage(==SQLite DB) with pretty simple/easy-to-use *synchrounous* API. (IndexedDB is asynchrounous like mozStorage) It's SQLite DB representatiion of var Obj={ A: 1, B:2, C:3, ...}. JavaScript Object : var dat = Obj["B"] ; Obj["B"] = 1000; localStorage : var dat = localStorage.getItem("B"); localStorage.setItem("B",1000); It's a simplest one, and it's suitable to "per site data". I think XUL code in Firefox is also a valid "Web Application". An XUL element can be considered "an Web site which is located in Firefox". Please note that following is merely emulation of "window.loclStorage object creation in a window context for an URI". > // Directly access from XUL with any URL > var ios = Components.classes["@mozilla.org/network/io-service;1"] > .getService(Components.interfaces.nsIIOService); > var ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"] > .getService(Components.interfaces.nsIScriptSecurityManager); > var dsm = Components.classes["@mozilla.org/dom/storagemanager;1"] > .getService(Components.interfaces.nsIDOMStorageManager); > get_localStorage = function (Site) > { > var uri = ios.newURI(Site, "", null); > var principal = ssm.getCodebasePrincipal(uri); > var localStorage = dsm.getLocalStorageForPrincipal(principal, ""); > return localStorage; > } > var URI = "favicon://www.example.com"; // traslate http://www.example.com for favicon saving. set as scope value > var localStorage=get_localStorage(URI ) ;

(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #2) > So the idea we somehow coalesced to is to move favicons to a separate sqlite database, and ATTACH DATABASE it to places.sqlite. > IT will look like part of places.sqlite, but won't cause fragmentation and size issues to it. Do you want to split many many BLOB data for big icons to different sqlite file dur to fragmentation in places.sqlire? If so, I think it's not so effective because SQLite holds a entire table row as a BLOB data. If collection of many big data, repeated inset/delete produces big unused spaces in sqlite fiile, but "used spce/unused space ratio" is usually doesn't depend on block size. It merely affect on "size of unused space. "places.sqlite + favicons.sqlite + ATTACH" usually doesn't reduce "total unused disk space size in sqlite.file(s)" and "used space/unused space ration".. Is performance issue found in places.sqlite? Or merely many complaints of "size of places.sqlite file is too big for me!" is annoying? Biggest volume of BLOB data in places.sqlite "data for favicon"? A known issue in SQLite is VACUUM. "Split to multiple sqlite files(DBs)" may be a good idea for "VACUUM a sqlite file at once". This kind of work can be called "user side DB split by key range or data type for performance". Exaample. Split table for bookmarks to multiple sqlite file(DB) by key range(for examle, range of top level domain or hashed value of URL". Split data for the key range to multiple sqlite fille(DB) : sqlite file(DB) for bookmark, and sqlite file(DB) for favicon(Your idea).

(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #0) > future support of larger icons. "Page Thumnail as *icon* for a page" like one? Pointer to row of central moz_places. Example of moz_bookmarks > CREATE TABLE moz_bookmarks ( > id INTEGER PRIMARY KEY, type INTEGER, fk INTEGER DEFAULT NULL, parent INTEGER, position INTEGER, > title LONGVARCHAR, keyword_id INTEGER, folder_type TEXT, dateAdded INTEGER, lastModified INTEGER, guid TEXT) > > id = "27" , fk = "24" , title = "Forum MozillaZine.jp Firefox" > colum named "flk" points id of row in moz_places. Row in moz_favicons looks pointed only by " favicon_id" of moz_places. A kkind of "Normalization" is done properly. > CREATE TABLE moz_places ( > id INTEGER PRIMARY KEY, > url LONGVARCHAR, title LONGVARCHAR, (snip) > favicon_id INTEGER, (snip) > > id = "24" , uri = "http://forums.mozillazine.jp/viewforum.php?f=2" > title = "MozillaZine.jp Forum • Forum - Mozilla Firefox" > favicon_id = "19" > > CREATE TABLE moz_favicons ( > id INTEGER PRIMARY KEY, > url LONGVARCHAR UNIQUE, data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT) > > id = "19" , uri = "http://forums.mozillazine.jp/favicon.ico" , data = "BLOB (Size: 608)" If so, I think following is possible and not so hard, and it won't require big change. Seprated favicons.sqlite file(SQLite DB) for Table moz_favicons. Change definition of favicon_id = "19" of moz_places : id of row in Table=moz_favicons of same SQLite DB => id of row in Table=moz_favicons of different SQLite DB Because ATTACH is supported by SQLite as you say, this is not so hard. Consistency of "favicon_id of moz_places" can be kept by simple code in Places and/or FavIcon Manager. column of "users" of moz_favicons : Array of id of moz_places who referrs to this favicon. When moz_place uses this favico, add himself to "users". When moz_place stops using this favico, remove himself from "users". If no user, remove this favicon. Full page data, small icon for Tb, mid-size Thumnail for navigation, can be held at out side of important places.sqlite. If so, this DB can be used for data like Back&Forward Cache. If written to SQLite DB, JavaScrpt Object can be destroyed. It'll reduce memory consumption. If SQLite DB is split, VACUUM of favicons.sqlite can be executed independently from VACUUM of places.sqlite. If SQLite DB is split, unbundling of "site meta data" and "site data" will be achived. places.sqlite : for site meta data such as visitt history favicons.sqlite : for site data such as favicon, Thumnail, ...

> or even let the cache handle them Early versions of Mozilla just relied on the cache for favicons, but this meant that your bookmarks would randomly lose and regain their favicons as they expired from the cache / were recached, so that's not a solution I would recommend.

I started posting 4 changesets that are the MVP. This is not all the work, but it's complex enough to review, that I prefer starting sooner with that. I plan still at least 2 changesets for this bug: 1. fallback to root icons if direct icon for a page is not found 2. more tests. I marked down parts where there's lack of tests and will write tests while the review proceeds. In addition I need some additional testing: 3. Talos regressions 4. Migration of my default profile and verification of the results Depending on the status of bug 1343499 I may decide to add proper ico files support here, or do that in the follow-up bug 1337402. The reason to do that here is mostly a better migration.

Comment on attachment 8816669 [details] Bug 977177 - Move favicons to a separate store. https://reviewboard.mozilla.org/r/97320/#review125516 ::: toolkit/components/places/Database.cpp:850 (Diff revision 3) > - rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > - "PRAGMA synchronous = FULL")); > + MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA foreign_keys = ON") > + ); > - NS_ENSURE_SUCCESS(rv, rv); > + NS_ENSURE_SUCCESS(rv, rv); > +#ifdef DEBUG > + { I think this could use a comment: you just set the pragma, so briefly explain why you're immediately checking whether it was set. ::: toolkit/components/places/FaviconHelpers.cpp:964 (Diff revision 3) > + nsCOMPtr<mozIStorageStatement> stmt; > + nsresult rv = mDB->CreateStatement(NS_LITERAL_CSTRING( > + "SELECT id, width, data FROM moz_icons WHERE typeof(width) = 'text'" > + ), getter_AddRefs(stmt)); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + if (NS_FAILED(rv)) return rv; Nit: braces + new line as usual for if's ::: toolkit/components/places/FaviconHelpers.cpp:1017 (Diff revision 3) > + AcceptedMimeTypes::IMAGES)) { > + return NS_ERROR_FAILURE; > + } > + > + // If it's an SVG, there's nothing to optimize or convert. > + if (aMimeType.EqualsLiteral(PNG_MIME_TYPE)) { ... this should be SVG_MIME_TYPE, right?

Comment on attachment 8816670 [details] Bug 977177 - Update favicons API consumers. https://reviewboard.mozilla.org/r/97322/#review126032 This is probably one of the biggest patches I've ever reviewed, but I looked it over a couple of times, and I don't see any obvious problems.

Comment on attachment 8848212 [details] Bug 977177 - Add size ref fragment to icon protocols. https://reviewboard.mozilla.org/r/121148/#review126034

Comment on attachment 8848213 [details] Bug 977177 - Expire old page to icon relations to avoid serving deprecated icons. https://reviewboard.mozilla.org/r/121150/#review126036

Nice work Marco (as always)! I know you said there's more to do, but congratulations on these parts!

Thanks, I'll shortly start working on the next parts. Imagelib also got support for ico frames extraction, so I may add that on top before landing the migration.

There's currently a blocking showstopper in bug 1352408, where I'm currently unable to update icons when they change, that basically means after a visit that loads a new icon, the UI won't update. I'm waiting for feedback from imagelib experts to evaluate what to do. Apart from that, I'm code-complete and just refining the last unit tests and measuring Talos.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2fd5b4af803426d9d116d34f50e3f08ea6aa1e Perfherder (still fetching results): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=80325998821e&newProject=try&newRevision=3a2fd5b4af80&framework=1&showOnlyImportant=0

Hi Brindusa, we are "close" to landing this (provided we are missing reviews and a dependency yet, before we can land, but close enough). It would be nice to start figuring out resources and a test plan for favicons functionality in the ui. The scope of the testing is to check that all the ui views showing favicons keep doing that.

Comment on attachment 8853436 [details] Bug 977177 - Add favicons.sqlite to profile related lists. https://reviewboard.mozilla.org/r/125528/#review128102 thanks for making sure talos was accounted for :)

Talos doesn't show anything interesting apart from a possible improvement to tp5n nonmain_normal_fileio opt (e10s and not)

Remaining dependencies: Bug 1351163 is about to change a bunch of code in the moz-anno protocol handler, I'll have to rebase on top of it Bug 1352408 is confirmed as the reason why the UI icons don't update on visit, the image cache just returns the same payload if not invalidated.

Adding myself to QA Contact to verify this after it will get fixed.

Latest version of the patches contains unbitrot for bug 1351163 and image cache fix using the patch in bug 1352408.

Comment on attachment 8816670 [details] Bug 977177 - Update favicons API consumers. https://reviewboard.mozilla.org/r/97322/#review130946 ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:132 (Diff revision 5) > > + if (!mData.IsEmpty()) { > + nsCOMPtr<nsIInputStream> stream; > + rv = NS_NewByteInputStream(getter_AddRefs(stream), > + mData.get(), mData.Length(), > + NS_ASSIGNMENT_DEPEND); I'm not sure using NS_ASSIGNMENT_DEPEND here is safe, since this class instance isn't guaranteed to remain alive until the stream pump is finished. But since you're using a nsCString now rather than a raw byte buffer, you can just use `NS_NewCStringInputStream` instead, which is simpler and guarantees safe refcounting. ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:137 (Diff revision 5) > + MOZ_DIAGNOSTIC_ASSERT(mListener); > + NS_ENSURE_TRUE(mListener, NS_ERROR_UNEXPECTED); We should probably move this toward the top of the HandleCompletion method, since also use it further on, and we no longer have the `!mListener` check at the start of the method.

(In reply to Kris Maglione [:kmag] from comment #50) > Comment on attachment 8816670 [details] > Bug 977177 - Update favicons API consumers. > > https://reviewboard.mozilla.org/r/97322/#review130946 > > ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:132 > (Diff revision 5) > > > > + if (!mData.IsEmpty()) { > > + nsCOMPtr<nsIInputStream> stream; > > + rv = NS_NewByteInputStream(getter_AddRefs(stream), > > + mData.get(), mData.Length(), > > + NS_ASSIGNMENT_DEPEND); > > I'm not sure using NS_ASSIGNMENT_DEPEND here is safe, since this class > instance isn't guaranteed to remain alive until the stream pump is finished. Ah ops, the next changeset changes it to COPY! I just merged the change into the wrong changeset... > But since you're using a nsCString now rather than a raw byte buffer, you > can just use `NS_NewCStringInputStream` instead, which is simpler and > guarantees safe refcounting. Sounds good. > ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:137 > (Diff revision 5) > > + MOZ_DIAGNOSTIC_ASSERT(mListener); > > + NS_ENSURE_TRUE(mListener, NS_ERROR_UNEXPECTED); > > We should probably move this toward the top of the HandleCompletion method, > since also use it further on, and we no longer have the `!mListener` check > at the start of the method. Yep, thanks.

Comment on attachment 8853434 [details] bug 977177 - Fallback to the root domain icon. https://reviewboard.mozilla.org/r/125524/#review131146

Comment on attachment 8853435 [details] Bug 977177 - Split ico files into native frames. https://reviewboard.mozilla.org/r/125526/#review131148

Comment on attachment 8853436 [details] Bug 977177 - Add favicons.sqlite to profile related lists. https://reviewboard.mozilla.org/r/125528/#review131150

Comment on attachment 8853437 [details] Bug 977177 - Don't expire root domain icons with history, and don't associate pages to them. https://reviewboard.mozilla.org/r/125530/#review131152

Comment on attachment 8853438 [details] Bug 977177 - Invalidate the page-icon image cache when necessary. https://reviewboard.mozilla.org/r/125532/#review131154

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/942aa1533e08 Move favicons to a separate store. r=adw https://hg.mozilla.org/integration/autoland/rev/90d755bcbb92 Update favicons API consumers. r=adw https://hg.mozilla.org/integration/autoland/rev/4a0770d810dc Add size ref fragment to icon protocols. r=adw https://hg.mozilla.org/integration/autoland/rev/60002894a42b Expire old page to icon relations to avoid serving deprecated icons. r=adw https://hg.mozilla.org/integration/autoland/rev/62f3fc3cb351 Fallback to the root domain icon. r=adw https://hg.mozilla.org/integration/autoland/rev/864e72c60156 Split ico files into native frames. r=adw https://hg.mozilla.org/integration/autoland/rev/5311e5ac3b4b Add favicons.sqlite to profile related lists. r=adw,jmaher https://hg.mozilla.org/integration/autoland/rev/42df4b3da073 Don't expire root domain icons with history, and don't associate pages to them. r=adw https://hg.mozilla.org/integration/autoland/rev/d73a295b3652 Invalidate the page-icon image cache when necessary. r=adw

https://hg.mozilla.org/mozilla-central/rev/942aa1533e08 https://hg.mozilla.org/mozilla-central/rev/90d755bcbb92 https://hg.mozilla.org/mozilla-central/rev/4a0770d810dc https://hg.mozilla.org/mozilla-central/rev/60002894a42b https://hg.mozilla.org/mozilla-central/rev/62f3fc3cb351 https://hg.mozilla.org/mozilla-central/rev/864e72c60156 https://hg.mozilla.org/mozilla-central/rev/5311e5ac3b4b https://hg.mozilla.org/mozilla-central/rev/42df4b3da073 https://hg.mozilla.org/mozilla-central/rev/d73a295b3652

As expected, based on the previous Try measurement, this shown a positive improvement to tp5n nonmain_normal_fileio of about 12-13% most likely due to not associating anymore root domain icons to pages. It's likely we may re-use some of this improvement in the future to store multiple icons for each page in bug 1352459.

Great work! Here are the improvements stats: == Change summary for alert #5970 (as of April 12 2017 14:59 UTC) == Improvements: 14% tp5n nonmain_normal_fileio windows7-32 pgo e10s 474983808.04 -> 409410164.67 13% tp5n nonmain_normal_fileio windows7-32 opt e10s 477240408.25 -> 414585120.08 13% tp5n nonmain_normal_fileio windows7-32 opt 470541926.33 -> 409490063.17 13% tp5n nonmain_normal_fileio windows7-32 pgo 470547722.25 -> 410897176.83 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5970

Release Note Request (optional, but appreciated) [Why is this notable]: non backwards compatible change [Affects Firefox for Android]: no [Suggested wording]: Favicons have been moved to a new data store to allow using higher resolution icons. It's not possible to downgrade a newly created profile to a previous Firefox version. [Links (documentation, blog post, etc)]: not yet, I may probably write a blog post in the next week.

This is in the release notes as "Breaking profile changes - do not downgrade Firefox and use a profile that has been opened with Firefox 55+"

It seems like there's a minor issue. When I use the new bookmark feature, while I'm editing it a favicon from one of the websites is shown, it only changes to an empty one when I actually add the bookmark.

that sounds like old bug 946820.

(In reply to Marco Bonardo [::mak] from comment #81) > that sounds like old bug 946820. Similar, but to me it only happens while the new bookmark dialog is open. (And I was using the sidebar)

Firefox ESR 52.x (at least versions released after FF55) should be able to open the profile and vice versa until it goes out of support.

(In reply to alex from comment #83) > Firefox ESR 52.x (at least versions released after FF55) should be able to > open the profile and vice versa until it goes out of support. The profile incompatibility problem is only for profiles *CREATED* in Firefox 55 (or later). if a profile was updated to 55, it can be moved to 52, the only issue will be missing favicons.