1051615 - Update pageURL-to-faviconURL mapping when provided by page visit

Status: RESOLVED DUPLICATE of bug 1051544

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

Description

[Mark Capella [:capella]]

Attachment: updatePageURLMappings.diff

This is a very edge-case situation that nonetheless we can enhance at almost no cost or risk.

I found this while looking at bug 1044940. While it's a very small improvement, it points to follow-ups / new patches.

In cases where the user visits a page, the faviconURL is available when Favicons.getSizedFavicon() is called, but not tucked into the pageURL->faviconURL mappings (memory cache).

This causes TwoLinePageRow (consumed by BookmarksList and HistoryList) and Topsites, to wind up displaying the default globe as they're unable to locate the favicon locally via getSizedFaviconForPageFromLocal().

Comment 1

[Chris Kitching [:ckitching]]

Comment on attachment 8470548 [details] [diff] [review]
updatePageURLMappings.diff

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

(In reply to Mark Capella [:capella] from comment #0)
> This causes TwoLinePageRow (consumed by BookmarksList and HistoryList) and
> Topsites, to wind up displaying the default globe as they're unable to
> locate the favicon locally via getSizedFaviconForPageFromLocal().

That shouldn't be the case: the favicon url is supposed to end up in the history table after you visit a page. Subsequent calls to load a favicon using the database should be using the history table to find the favicon URL. The result of such resolutions should then be pushed to the cache. (this certainly *used* to work...)

So, from a correctness point of view, your patch should do nothing. Since it has apparently failed to do nothing, we have a different bug to fix: why doesn't a call to getSizedFaviconForPageFromLocal use the database properly to resolve favicon URLs? That might be the real answer to Bug 1044940. I advise you to work towards identifying and solving that underlying problem (read nothing into my assignment to Bug 1044940: theft encouraged).

From a performance point of view, I'm not convinced this patch is a good idea.

When scrolling through a list in about:home, if we change direction we're going to want to re-load some favicons that we recently loaded. These will probably be in the memory cache, but that's keyed by favicon url. This means we'll have to query the database to resolve page urls to favicon URLs before we can hit the cache: unacceptable. So we have a small cache of these mappings, allowing cached favicons to be used to satisfy requests in terms of a page URL without needing to hit the database anyway (otherwise the cache isn't really helping that much...).

Keying by favicon URL not page URL is sensible: different pages very often map to the same favicon (different pages on the same site usually have the same favicon, for example). If you key the favicon cache by page URL, you end up with a lot of duplication. Admittedly I did a poor job of hiding this detail. Refactors welcome.

Loading a page in the standard way (the main use of the method you're changing) doesn't correlate with a likely need to perform the same resolution again in the near future, so I don't think adding the mapping to the cache at this point helps us more than it hurts us (by causing a cache write).

Comment 2

[Mark Capella [:capella]]

re: From a performance point of view ...
We're adding a nanosecond update of the cache in the real-world situation where a user opens a page that isn't going to impact anything in noticeable fashion.

re: Since it has apparently failed to do nothing, we have a different bug to fix...
In this bugs description, I agree that it points to follow-ups. I don't want to morph this into anything bigger than what is here.

Comment 3

[Chris Kitching [:ckitching]]

(In reply to Mark Capella [:capella] from comment #2)
> re: From a performance point of view ...
> We're adding a nanosecond update of the cache in the real-world situation
> where a user opens a page that isn't going to impact anything in noticeable
> fashion.
> 
> In this bugs description, I agree that it points to follow-ups. I don't want
> to morph this into anything bigger than what is here.

This is not an instance of scope-creep. Your patch is trying to solve a different problem (though has merit as a debugging tool, since it uncovered this behaviour).

The fact your patch has any affect on the correct behaviour of the favicon system is a (useful) *symptom* of a different problem which we should solve.

Your patch isn't fully effective at hiding the problem, either: when your new mapping is dropped from the cache before a load happens, you'll still see no favicon. The fix is to make loads correctly perform resolutions (using the cache as appropriate). Once we've got that, this patch has merit as a potential performance improvement: but one that, as I explained in some detail in Comment 1, I don't think has a positive payoff (nor a particularly significant negative one). 

On the bright side, the fact you've discovered this behaviour is likely to prove valuable in figuring out why some loads aren't polling the database correctly, which is most excellent.

Comment 4

[Chris Kitching [:ckitching]]

s/any affect/any effect/

Comment 5

[Chris Kitching [:ckitching]]

Bug 1051544 is also most likely related (probably all part of the same problem).

Comment 6

[Mark Capella [:capella]]

re: That shouldn't be the case: the favicon url is supposed to end up in the history table after you visit a page. Subsequent calls to load a favicon using the database should be using the history table to find the favicon URL.

From a fresh profile, I've synched Bookmarks but not History. Or a simpler case, I've cleared my History, closed FF and openned it again.

For my bookmark with pageURL "http://arstechnica.com/", and where the actual faviconURL winds up being located at "http://static.arstechnica.net/favicon.ico", the Bookmark Panel displays a default globe, and of course the History panel is empty.

If I tap the arstechnica bookmark, the site is dsplayed and the favicon shows in the awesomebar. The Bookmark panel still has a default favicon for it's listItem, and the History panel now has a listItem also with a default globe.

The |history Table| has an entry for the pageURL of "http://arstechnica.com/" with a favicon_id of *nothing*.
The |favicons Table| has an entry for faviconURL "http://static.arstechnica.net/favicon.ico" and an _id of "1".

So, what's mapping the pageURL->faviconURL, or what's expected to, is where I get lost.


re: Bug 1051544 is also most likely related (probably all part of the same problem).

I believe you're right.


btw: Why can't I find a reference to this method?
http://mxr.mozilla.org/mozilla-central/search?string=updateHistoryEntry&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 7

[Chris Kitching [:ckitching]]

(In reply to Mark Capella [:capella] from comment #6)
> From a fresh profile, I've synched Bookmarks but not History. Or a simpler
> case, I've cleared my History, closed FF and opened it again.
> 
> For my bookmark with pageURL "http://arstechnica.com/", and where the actual
> faviconURL winds up being located at
> "http://static.arstechnica.net/favicon.ico", the Bookmark Panel displays a
> default globe, and of course the History panel is empty.
> 
> If I tap the arstechnica bookmark, the site is displayed and the favicon
> shows in the awesomebar. The Bookmark panel still has a default favicon for
> its listItem, and the History panel now has a listItem also with a default
> globe.
> 
> The |history Table| has an entry for the pageURL of
> "http://arstechnica.com/" with a favicon_id of *nothing*.
> The |favicons Table| has an entry for faviconURL
> "http://static.arstechnica.net/favicon.ico" and an _id of "1".
> 
> So, what's mapping the pageURL->faviconURL, or what's expected to, is where
> I get lost.

That's a description of Bug 1051544. It looks like the favicon id is not being written to the history table when you visit the page. That's very scary. That needs to be fixed.

While I'm not comfortable landing your patch, it has proved useful by uncovering the underlying problem (thanks!). That it changes the behaviour pointed us to the underlying database problem.

A quick glance suggests the way to do this might be to add a history table write to updateFaviconForUrl in LocalBrowserDB. Ensure this write isn't attempted elsewhere to avoid duplicating logic before you take that approach.
You might like to consider what could be done in terms of mass-fixing existing broken rows. I suspect the answer is "nothing, but it'll get fixed magically next time they visit the page".

Also playing a role (possibly) is Bug 1044940. LoadFaviconTask only reads history table to find favicon ids. You cleared history, and since LoadFaviconTask isn't looking at the bookmarks table to find an id, it fails. (it might fail anyway: depends what's in your bookmarks table (which also needs to be kept consistent).

It's annoying that we have two tables. It'd be nice if we just had a "pages" table that held title/url/favicon and then referenced it from the history/bookmarks table so we don't have to do these duplicate writes everywhere.
Anyone in the mood for restructuring the entire database? In the meantime, looks like you're going to have to perform three writes in updateFaviconForUrl (or find the existing solution to this problem (which must be *somewhere*!) and figure out why it suddenly died).


From here, armed with the new information about the nature of the problems, you should be able to go and attack Bug 1051544 and Bug 1044940 at their source.


In general, I oppose landing patches that introduce additional technical debt unless there is a compelling reason to do so (such as the need to ship a feature on time, or some deadline that precludes a proper solution).
In this case, The cost/benefit analysis looks something like the following:

Pros:
• Usually hides the effect of the database consistency errors when you've visited a page earlier in your browsing session and then look at it in one of the list views.

Cons:
• This hack must be repeated everywhere code does similar work: people will forget, we'll get regressions.
• The cache is no longer consistent with the database contents.
• We're contradicting the assumptions made by the function you're modifying: a reader may think "But isn't that redundant?", causing additional confusion.
• Tiny performance hit (reduced cache hit rate, cost of extra writes)
• Temptation to neglect underlying problem now it's less obvious to reproduce.

Additionally, we've got no pressure to urgently ship this.

Fixes to the database consistency problems that this patch has uncovered are *very* welcome. (if you do not wish to do that work, please let me know so I can schedule it).

> btw: Why can't I find a reference to this method?
> http://mxr.mozilla.org/mozilla-central/
> search?string=updateHistoryEntry&find=&findi=&filter=^[^\0]*%24&hitlimit=&tre
> e=mozilla-central

Because it's both undocumented and unused.

It's not clear that it's useful to keep. Perhaps investigate and see what it was replaced with? Make use of hg log and determine if we need to take any action.

Comment 8

[Mark Capella [:capella]]

re: you should be able to go and attack Bug 1051544 and Bug 1044940 at their source.

I'm going to point Kat's bug 1051544 here so he's at least aware of the similar ongoing research.

I'll take this bug so it shows up on my Dashboard, and for now confine my comments / developments here to maintain my context, and avoid jumping the discussion around.

I'd said earlier I didn't want to expand scope on the original tiny patch and perhaps land it with followup work that we both pretty much knew was still outstanding. But ... I think it's too late for that ;-)

re: I suspect the answer is "nothing, but it'll get fixed magically next time they visit the page".

That was and still is my current thought. Instead of DB scrubs/whatever, we start out smaller and fix-up logical associations during normal User interactions.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #7)
> (In reply to Mark Capella [:capella] from comment #6)
> > From a fresh profile, I've synched Bookmarks but not History. Or a simpler
> > case, I've cleared my History, closed FF and opened it again.

I believe in fixing bugs from the inside out, not outside in.

So let's fix Bug 1044940, which should fix the initial observation.

Then let's fix Bug 1051544, which should ensure that the database has correct data.

Then let's come back to this bug and see if it's worth addressing. Certainly nothing about the page URL mappings will address issues that appear across restarts.


> It's annoying that we have two tables. It'd be nice if we just had a "pages"
> table that held title/url/favicon and then referenced it from the
> history/bookmarks table so we don't have to do these duplicate writes
> everywhere.

Our history table kinda performs that role, and we could easily make it so: use visitCount = 0 instead of clearing history items that match bookmarks. Close enough that it causes incorrect assumptions, evidently. But no point discussing this further.

Comment 10

[Richard Newman [:rnewman]]

Incidentally, you might be interested in Bug 1040806 Comment 5. That bug outlines some of the relationships here.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8470548 [details] [diff] [review]
updatePageURLMappings.diff

Clearing request for now. Let's tackle the data persistence stuff first.

Comment 12

[Richard Newman [:rnewman]]

If we ever do this, ckitching is the right reviewer (owns this code, understands the issues well), so marking him as mentor.

Comment 13

[Mark Capella [:capella]]

Cool. I'll be happy to explain my understanding of the issue if he needs.

Comment 14

[Mark Capella [:capella]]

Actually, kat's bug 1051544 will be enough to drive this.