1051544 - Favicon cache doesn't update history entries with favicon ids

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

STR:
1. Start Fennec with a fresh profile (I used aurora but it should happen on nightly as well)
2. Go to http://people.mozilla.org/~kgupta/bug/favicon/index.html
3. Click on the link on that page
4. Click on the URL bar and observe that on the "top sites" screen the two entries for the two pages you just visited both have a favicon-thumbnail. Swiping over to the history tab also shows both entries having favicons.
5. Quit fennec
6. Open fennec again
7. Look at the top sites screen - now only one of the entries has a favicon. OMG!

Expected results:
In step 7 both entries should still have a favicon.

I poked around in the code and it looks like if you have two pages pointing to the same favicon, the favicon cache will return the favicon directly when you go the second page, but the browser.db file is never updated to reflect this. So the history table entry for the second page has an empty faviconid. When you restart fennec the favicon cache is empty and so it doesn't know how to find the favicon for the second page. That's my theory, anyway.

This results in many history entries in the browser_db not having proper favicon_id values, and in my case, when I created a new search engine from one of those pages, it didn't get a favicon either. So now I have a bunch of search engines all showing the default favicon when they should all have perfectly good favicons.

Comment 1

[Mark Capella [:capella]]

fyi, We're all converging in this area ... see:

   Bug 1051615 - Update pageURL-to-faviconURL mapping when provided by page visit
   Bug 1044940 - Favicons in the bookmarks table are neither read correctly nor written

Comment 3

[Mark Capella [:capella]]

Attachment: bug1051544.diff

The description above nails the problem.

We write History table entries without favicon_id information as we visit pages. We later update the History favicon_id after onTabChanged(LINK_FAVICON) when we call LoadFaviconTask to get the favicon from the network and store it in the favicon table.

This satisfies most cases, but in the above case, where we have two pages referencing the same faviconURL, LoadFaviconTask is never called to save the favicon table entry as it's already been stored, and so the second page's history table entry isn't updated.

A quick solution I've found is the attached, where we update the History table entry at final onTabChanged(PAGE_SHOW) when we know we have the correct pageURL <-> faviconURL mapping.

This is basically an idea I'd previously worked into bug 1044940 but removed as it wasn't tightly coupled to that particular issue. ckitching had some questions around comment #7 ...

re: |You're already updating the URLs in the database over in LoadFaviconTask after you've downloaded the favicon. This code will mean you update the URLs, then download the favicon, store it, then update the URLs again. |

While we do update the favicon_id in the history table entry in LoadFaviconTask after we've downloaded the favicon, the bit about updating the URLSs again is more accurately about updating -new- History table entries created during user interactions. (pageURL's that may differ from the original pageURL that caused the favicon to be written to the DB).

This also corrects a second user interaction where we can currently fail differently, but for the same root cause. The STR for this is tricky, but after clearing History from the History panel, swiping close and reopening FF again and visiting the page by tapping a Bookmark, then swiping close and reopening again, you'll observe that both the Bookmark and History panel view for the item now shows default.

In any case, I'm asking ckitching for feedback here as I'm pretty sure this isn't ready for r+, and perhaps on looking at it, he might provide a better solution than this quick hack.

Comment 4

[Mark Capella [:capella]]

Ping on this guy?

Comment 5

[Chris Kitching [:ckitching]]

(In reply to Mark Capella [:capella] from comment #4)
> Ping on this guy?

Er, sorry, things have been a bit hectic.

Reading the first post I think Kats is almost certainly right. Pretty embarrassing that this has gone unnoticed for so long. :P

What I dislike about your patch is that it makes every favicon load (including ones that hit the cache!) incur a database write.

Can't we instead fix this when we create the new history entry? We don't want to write to the database every time everyone loads the favicon. If we do that, there's almost no point having a cache at all.

Comment 6

[Mark Capella [:capella]]

I'm not sure I follow. In Kat's example, we'll never incur an update/write to the History Table to update the favicon_id ... as we normally would.

During step 3) of the original description above, due to the favicon itself already having been written to the Favicon Table by step 2), we find the pageURL -> faviconURL map and assume we don't need to write to the Favicon Table, and therefore don't update the History Table.

This can also be true in the second user interaction in describe in comment #3 where the user has cleared History, yet the Favicon Table entry still exists. We can't predict or prevent that, but we can correct it as we encounter it.

In these cases we're not incurring an additional write, just performing it in a different code-path.

In normal usenot involving these (duplicate/cleared history) edge cases, we still avoid incurring an extra write, by checking first to see if it's required and bypassing if not, all the while on the background thread without affecting user response time.

Comment 7

[Mark Capella [:capella]]

Comment on attachment 8496673 [details] [diff] [review]
bug1051544.diff

In case I didn't exactly answer your last question: |Can't we instead fix this when we create the new history entry?|

We can't, because when we create the new history entry at initial page load |onTabChanged(PAGE_SHOW)|, we don't yet have the faviconURL string. So we need to update it at subsequent |onTabChanged(LINK_FAVICON)|.

This seems correct, and in absence of another answer, I'm bumping from feeback? to review? and asking rnewman also since he usually finds obvious stuff I've not considered.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8496673 [details] [diff] [review]
bug1051544.diff

Review of attachment 8496673 [details] [diff] [review]:
-----------------------------------------------------------------

This is going to get bitrotted all to hell by Wes's work in Bug 1077590, and that bug is definitely the oil tanker that shouldn't change course. So this'll have to wait a few days to land.

But that bug also shines a light on one of the problems in this patch, and provides a way to fix it.

BrowserDB is going away because it makes it easy to write code that's wrong.

In this patch, we're assuming that a favicon load refers to (and should write into) the LocalBrowserDB that's currently held by BrowserDB, but that's not a valid assumption.

(Thought experiment: open Fennec in guest mode. Trigger a favicon load from, say, the search activity or the share overlay, or some future service. Observe that the favicon load just wrote into the wrong DB.)

BrowserDB makes it convenient to write code like this: having side effects on some global database from deep within the bowels of an otherwise isolated component. Wes's patch gives you direct access to the correct DB as we go.

That aside, I agree with the high-level approach, which I'd phrase like this: when we finish loading a page, we know the icon URL, and we can fetch it; if we fetch it from the cache, and the page URL is new, update the DB accordingly; if we fetch it 'live', make sure the DB is correct.

That raises three perf concerns.

* Don't write into the DB for the icon if the page already has a mapping. We *know* when we record the history visit whether the page already existed in the DB. If it already existed in the DB, can we assume it already had a mapping? If not, can we do better than what we're doing here?

* Definitely don't issue two queries when one will do -- here you call getIDForFaviconURL then updateHistoryFaviconID, and in another case you call  updateHistoryFaviconID and updateBookmarksFaviconID, each of which makes two queries. I'd rather see these exposed as separate endpoints in BrowserProvider to ensure that common operations like this result in *one* hit to the CP (and inside that, *one* hit to the DB).

Yes, this involves you doing more than just plugging together BrowserDB calls. I think the reduction in query overhead is worthwhile.

* GlobalHistory batches requests for 100msec to avoid hammering the DB. We have the cache, and this is non-critical data. Is there a good reason not to batch these writes in a similar way?

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 10

[Kartikaya Gupta (email:[e-mail])]

So how's this going?

Comment 11

[Mark Capella [:capella]]

My take-away from comment #8 was try not to interfere with Wes's work in Bug 1077590, so I pushed off.

Additionally, the preferred approach seems more complex than what I posted, and I haven't looked into it. Not sure of the tradeoff between code complexity and realizable perf gain, I just haven't looked.

I can't realistically get to this before January, so I'm freeing it ... perhaps Richard / Chris can morph the solution faster than I'll be able to?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

The oil tanker that is bug 1077590 appears to have ground to a halt. As this bug quite a noticeable user issue, any objection to moving forward with a fix on this one? (assuming we can find somebody with spare cycles to take it on)

Comment 13

[Richard Newman [:rnewman]]

I think Wes would be happy with me (or someone else) stealing Bug 1077590. If this gets there first, I'm fine with that.

Lots of open issues in Comment 8, though!

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.