423126 - favicons aren't backed up to *.json

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[Simon Bünzli]

Steps to reproduce:
1. Bookmark some pages with favicons
2. Close Firefox and delete places.sqlite
3. Restore your bookmarks from the JSON backup

Actual result:
All pages show the default "blank page" icon.

Expected result:
Bookmarks are easy to distinguish from the beginning thanks to the favicons. This is especially important for when the favicon was manually assigned (e.g. for JavaScript bookmarklets).

Comment 1

[Simon Bünzli]

In case performance/size should be the issue, at least the favicons' URLs should be backed up (resp. the full data: URLs where there's no http(s) one).

NB: Otherwise there's currently the choice between losing tags (HTML) or favicons (JSON). Not really the ideal perspective for data conscious users...

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we currently store them in the SQLite? Is it a matter of just copying some data URI strings?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> Do we currently store them in the SQLite? Is it a matter of just copying some
> data URI strings?

Yes, they are stored in the table moz_favicons. Adding Dietrich who could answer your latter question.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, but a patch with tests would get my a+ ...

Comment 5

[Recall]

Bug 419731 and Bug 424669 have just been resolved. Since titles are now fully restored, this would be the final piece of the json backup. Is nobody able to get a quick working patch for FF3?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also happens with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre

Comment 8

[Dietrich Ayala (:dietrich)]

Would definitely like to see this fixed, but won't block 3.1 for it.

Comment 10

[Drew Willcoxon :adw]

Attachment: patch - setAndLoadFaviconForPage

Here's a stab at it.  It simply serializes favicon URIs on backup and setAndLoadFaviconForPage's on restore.

Works fine, but can someone who knows the code better tell me if it's wise to (asynchronously) load an entire database's worth of favicons after restore?  I suspect not... It would be easy to store the data URIs directly in the JSON, at the cost of course of a bigger backup file.

I will try and find a way to get some numbers to compare the tradeoff.

Comment 11

[Drew Willcoxon :adw]

I've done some preliminary testing of these two approaches:

1. Store only favicon remote URLs.  Asynchronously load data after restore.
2. Store both remote URLs and data URIs.  During restore, manually set each favicon's data.

The upshot is, not surprisingly, approach 1 is faster on both backup and restore, and its resulting JSON is smaller.

I tested the completion time of calls to PlacesUtils.backupBookmarksToFile and PlacesUtils.restoreBookmarksFromJSONFile.  There were 3 rounds of tests with 10000, 1000, and 100 bookmarks each.  Each round I tested both approaches, for each approach both backup and restore, and for each of those I ran the test 5 times and took the average time.  I did the same for a control, meaning I left out favicon info altogether (i.e., the code as it is now).

Here's how the two approaches compared to the control:

10000 bookmarks:
  Approach 1:
    Backup:  16.2405 % more time to complete than control
    Restore:  7.1314
  Approach 2:
    Backup:  34.1658
    Restore: 35.4294

1000 bookmarks:
  Approach 1:
    Backup:  20.7603
    Restore: 13.5342
  Approach 2:
    Backup:  34.1601
    Restore: 83.0130

100 bookmarks:
  Approach 1:
    Backup:  17.0163
    Restore: 14.1269
  Approach 2:
    Backup:  28.9044
    Restore: 80.4761

Even less surprising is the JSON backup file size:

10000 bookmarks (1000 and 100 are similar):
  Approach 1:  46.5646 % bigger than control
  Approach 2: 600.0019

My assumptions in the test bookmarks were:
- Each bookmark has a unique favicon
- Data URI average size is 798 characters

A third approach would be a combination of the first two, where we store full data URIs for some bookmarks and only remote URLs for others.  Obvious candidates for storing full data URIs are favicons behind walls that require user interaction to climb, like passwords.  A drawback of approach 1 is that such favicons are lost unless the user visits them before his history is cleaned up and they're expired -- which happens no later than when he closes the browser.  But there doesn't appear to be a way to tell whether a favicon URI returned by the favicon service is behind a password.  https yes, but while many passwords are over https, not all are.  Maybe it's the best we can do?

Other drawbacks of approach 1:
- If user quits browser before a favicon loads, it will be lost for good.  The more bookmarks or the slower the connection, the longer he has to wait.  If he's not online at all when he restores, not only will he not be able to see his favicons, they'll be gone the next time he starts the browser.
- Adds more network activity after restore (duh).

Comments?

Comment 12

[Dave Garrett]

I would think that many users wouldn't really want to have a flurry of favicon downloads on restore. Fully backing up the icons to the JSON itself sounds like the best route to me. Besides, HTML backups store favicons using data URIs so the most logical thing to do here is to do the same.

Comment 13

[Marco Bonardo [:mak]]

As we talked on IRC, i think a good approach could be to backup https for the reasons you say, but also backup favicons for bookmarks on the toolbar, since they appear immediately and some user has toolbar without labels (restore for them makes the toolbar unusable). And probably also bookmarks where the favicon is set by the user (see bookmarklets case above). That should reduce the volume of favicons data to save to about 1/3, for the remaining part we could save the favicon uri.
We could change expiration of the empty favicons to allow more time for favicon service to lazy/async load missing ones, and only expire unused ones.
This needs further considerations and Dietrich's approval clearly, before putting hands on code.

About the performances, the backup happens often on shutdown, but luckily only on the first shutdown in the day (personally i would reduce the idle timeout, i'll ask dietrich about that). the restore is an op where the user is expecting to have some waiting... so till the loss is some second probably won't be an issue.

Comment 14

[Marco Bonardo [:mak]]

actual more important use cases we found:
- bookmarklets with a user defined icon
- toolbar bookmarks (users without labels on toolbar), only direct children
- recently accessed (or most visited) bookmarks (with an hard limit)

Comment 15

[Dietrich Ayala (:dietrich)]

This could be a high-impact change, as we run backup daily. Too late in the 3.1 cycle for this. Re-targeting to 3.2.

Comment 16

[Tomas]

Any chance to get this in 3.6 final?

Comment 17

[Brian Carpenter [:geeknik]]

Tomas, unless one of the devs knows something I don't, probably not. I don't even think anyone is working on this right now.

Comment 18

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 20

[Danny]

Don't want to be annoying, but is it going to be in Firefox 3.7 (or whatever is the next major version)? What are the plans?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think this is waiting for a decision between the methods proposed in Comment 11.

Comment 22

[Sid]

(In reply to comment #11)
> I've done some preliminary testing of these two approaches:
> 
> 1. Store only favicon remote URLs.  Asynchronously load data after restore.
> 2. Store both remote URLs and data URIs.  During restore, manually set each
> favicon's data.
> 
> ...
> 
> Comments?

IMHO we need the second approach. Back-up is a very important feature, and in no case any data should be lost (or don't call it back-up then). With the first approach, restoring a favicon depends on so many external factors: unstable internet connection, resources that need authorisation, unexpected browser closing, etc. It's not an option. The second way seems to be perfectly reliable, though at the cost of speed and file size. To me it's worth losing some extra time and a couple of megabytes just to be sure that I can always restore 100% of my data.

Comment 23

[Shawn Wilsher :sdwilsh]

This doesn't block for the same reasons it didn't block past releases.

Comment 24

[Peter van der Woude [:Peter6]]

should this bug have keyword dataloss ?

Comment 25

[Shawn Wilsher :sdwilsh]

Don't think so - the icons can be redownloaded (and would expire eventually too I think).

Comment 26

[Mike]

(In reply to comment #22)
> no case any data should be lost (or don't call it back-up then)
No. Backup/restore is not for normal operation, it is for emergency. Therefore, only important and/or user defined data should be stored. Favicon is nothing out of this. For anyone who disagree: page content is far more important than favicon, but isn't backup.

I strongly suggest to WONTFIX this bug.
Firefox should be quick.

PS. If someone do care about his bookmarks, then he should use OS-provided backup, or Weave, or something like Scrapbook addon.

Comment 27

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I don't agree...
If exporting is slow devs must simply speed up this like in other browsers or even programs and all exported data should be preserved

Comment 28

[WildcatRay]

(In reply to comment #26)
> 
> PS. If someone do care about his bookmarks, then he should use OS-provided
> backup, or Weave, or something like Scrapbook addon.

Sync (nee Weave) will only include the bookmark because favicons are stored separately (which is also the reason for this bug), therefore, it is not an alternative.

Another competitor, Xmarks (nee Foxmarks) must also be considered. AFAICT-Xmarks does sync favicons with bookmarks. For the devs to concede this to Xmarks can be seen as Firefox not being able to "compete". A minor point, perhaps, but still a shortcoming against the competition.

Comment 29

[Scott A.]

It would be nice if this could be worked on for 3.6.x prior to 4.0 release, this would ease the transition for people who like to make new profiles(/install fresh) with new major versions. This function shouldn't be something that relies on extensions.

Comment 30

[LycianSage]

(In reply to comment #28)

> Another competitor, Xmarks (nee Foxmarks) must also be considered.
> AFAICT-Xmarks does sync favicons with bookmarks. For the devs to concede this
> to Xmarks can be seen as Firefox not being able to "compete". A minor point,
> perhaps, but still a shortcoming against the competition.

Xmarks doesn't support favicons anymore. i don't think it's a wise move to rely on third party software for bookmark management.

i strongly agree with Sid, Virtual_ManPL, and Scott A.; and strongly disagree with Shawn Wilsher, and Mike.

favicos are neglected by some portion of web users. but for information seeking professionals, favicons are vital. if you are an academic, journalist, investigator, archeologist, or software developer bookmarks accumulate in your browser and become increasingly essential. information seeking is what Internet is about. and favicons are great usability enhancers for Internet users.

IMHO, for a modern browser project, neglecting favicons is [epic fail].

loosing favicon is loosing important data. bookmark users seek for favicons in the toolbars. they don't deal with reading the title. a bookmark is more a favicon than a title.

relying on a web-sourced favicon management (only storing the URI of the .ico file) is not the robust way of browser development. firefox should "store locally". and "backup locally". and "restore locally".

finally, Firefox 4.0 users are faced with the problem too.

Comment 31

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Exactly, I have over 6MB of exported HTML file of bookmarks.
If I want to restore backup from .json I need to open manually over 10.000 links to get a favicon...

What's the status in fixing this bug ?
Will it make to Firefox 5 ?
If not what's the plan.

Comment 33

[Marco Bonardo [:mak]]

not a regression, since we never added icons to json backups from the beginning

Comment 34

[heinzili11]

Not only the favicons are missing, the whole import function is broken for many years yet.
https://bugzilla.mozilla.org/show_bug.cgi?id=950533

Comment 35

[Bogdan Maris, Desktop QA]

This is still an issue on latest builds (54.0a1, 53.0a2, 52.0 RC and 52.0 ESR) across platforms.

Comment 36

[Marco Bonardo [:mak]]

I think we need a different solution for this, now that favicons will change, each page can have more than one.
It's unlikely we'll ever add favicon payloads to the json, it would become huge.
The best solution would be to fetch favicons in background for any page lacking one, we should probably spend time figuring out the possible privacy and perf implications there, rather than spending time on this.

Comment 37

[Julien Cristau [:jcristau] (back April 22)]

Too late for firefox 52, mass-wontfix.

Comment 38

[Marco Bonardo [:mak]]

wontfixing per comment 36, we need a different solution, surely we won't store favicons in the json file.