Closed Bug 916588 Opened 7 years ago Closed 7 years ago

No favicons (or wrong favicons) appearing in lists on about:home


(Firefox for Android :: Awesomescreen, defect)

Not set



Firefox 27
Tracking Status
firefox25 --- unaffected
firefox26 + verified
firefox27 + verified
fennec 26+ ---


(Reporter: Margaret, Assigned: ckitching)



(Keywords: regression)


(2 files, 1 obsolete file)

No description provided.
Oops, submitted the form before I meant to...

Only some favicons are appearing, and some of them are wrong.

Is this a regression from bug 888326?
tracking-fennec: --- → ?
Keywords: regression
Summary: Favicons not appearing in lists on about:home → No favicons (or wrong favicons) appearing in lists on about:home
Attached image screenshot
It looks like there might be a problem recycling views, since the icon is appearing next to ( is the top entry in my most visited list).
But Bug 888326 was almost entirely cleanup - something quite stupid has happened if it caused this.

Had it landed, I'd have thought Bug 905685 a more likely culprit, but it doesn't appear to have done so yet. (but it's making the sort of functional change that might cause this sort of problem if botched)

Hmm. I shall investigate. This one looks like it may prove interesting.
Assignee: nobody → ckitching
Currently, I can't reproduce the icon-transposition thing on the current build. I am able to make it make icons not appear, although this is a known failure of the Favicon caching system addressed in Bug 914296.
I'll be better able to fiddle with it in the morning when I have less patchy wifi for my phone to use.

I am unable to reproduce this bug at all after applying the patch from 914296.

I suggest backing out 888326 for now and landing Bug 905685. 905685 is changing a bunch of things in places that are liable to cause this sort of problem - if this truly is an underlying bug it may fix it anyway - if not, 914296 will deal with it later.

My best working theory at the moment is some funkyness with old OnFaviconLoadListener objects firing when their result is no longer desired by that view. You scroll, the views gets recycled, and the callback lands on the wrong one. (After the desired callback for that one has hit it).
I think you just need enough sites in your history to scroll in order to be able to see this.

If I set up sync with a new profile, I see the icons from the default bookmarks appear throughout my list of synced sites.
tracking-fennec: ? → 26+
I'm getting conflicts trying to back out the patches from bug 888326, so I also backed out the patch from bug 895423.

Doing that, I found that the guilty changeset is this one:

Since this patch just touches FaviconView, there's probably just some small mistake in here that we can fix. Chris, let's see if we can get a patch to fix this problem, rather than going through the pain of trying to back out all of bug 888326.
Blocks: 888326
Looking back at this patch more, I did a bad job reviewing it

We shouldn't be holding onto bitmaps in this view, because the image will change as the view is recycled, so the stored bitmaps will be wrong. I think we should see if we can just back out this patch, but that would require also backing out bug 895423, or changing it to just use updateImage instead of updateAndScaleImage.
Chris, any progress looking into this? We should try to fix this this week, or else back out the patches that caused the problem.
Flags: needinfo?(ckitching)
This bug is not fixed by the patches from Bug 905685. My investigation continues.
Attached patch Demadnessify the Favicon system (obsolete) — Splinter Review
This seems to do the job.

It's sort of strange - the old Favicon system seems to dispatch more than one result when it loads a Favicon, one of which will be the desired result, the others, null.
This behaviour plus the fact that the FaviconView's code to clear itself when it was set with a null seem to have been the cause of this issue. I am no longer able to reproduce the bug with this patch applied.
Attachment #806397 - Flags: review?(margaret.leibovic)
Flags: needinfo?(ckitching)
Comment on attachment 806397 [details] [diff] [review]
Demadnessify the Favicon system

Testing with this patch, I don't see the wrong favicons, but I also don't see the default favicon appear. Looking at your original patch, I must have missed this when reviewing. I think you should replace these calls with setImageResource(R.drawable.favicon);

See here for what we did before:
Attachment #806397 - Flags: review?(margaret.leibovic) → review-
Yes - this was intentional - I thought this was requested in Bug 873243.
Never mind - replacing old behaviour.
Attachment #806397 - Attachment is obsolete: true
Attachment #806733 - Flags: review?(margaret.leibovic)
(In reply to Chris Kitching [:ckitching] from comment #12)
> Created attachment 806733 [details] [diff] [review]
> V2 - Demadnessify the Favicon system.
> Yes - this was intentional - I thought this was requested in Bug 873243.
> Never mind - replacing old behaviour.

Sorry if there was any confusion, that bug is about not showing the default favicon in the case where another favicon replaces it. But we still want the default favicon if there's no icon for the site.
Comment on attachment 806733 [details] [diff] [review]
V2 - Demadnessify the Favicon system.

Excellent, thanks. I'll land this for you!
Attachment #806733 - Flags: review?(margaret.leibovic) → review+

Please request approval to uplift to Aurora as well.
Comment on attachment 806733 [details] [diff] [review]
V2 - Demadnessify the Favicon system.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 888326
User impact if declined: Favicons in the new about home will be completely wrong. Icons for different sites will be displayed by sites, some sites won't have icons when they should.
Testing completed (on m-c, etc.): Landed on fx-team. No oranges yet.
Risk to taking this patch (and alternatives if risky): Low risk - its a two line change in some UI code.
String or IDL/UUID changes made by this patch: None.
Attachment #806733 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Steps to reproduce on Aurora 26.0a2 2013-09-18/ Nighlty 27.0a1 2013-09-18:
1) Load and bookmark the page
2) Bookmark a link from the context menu
3) Enter about:home and remove the bookmark from the context menu

Actual results:
The unvisited bookmarks take the favicons of existing bookmarks.
Attachment #806733 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I am not able to reproduce the bug using Google Nexus 7 (Android 4.4.2) and Samsung Galaxy S (Android 2.3.4).
Feel free to open it again or a new one if you will see the wrong behavior again.
You need to log in before you can comment on or make changes to this bug.