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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Comment 1

[:Margaret Leibovic]

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?

Comment 2

[:Margaret Leibovic]

Attachment: screenshot

It looks like there might be a problem recycling views, since the nextbus.com icon is appearing next to facebook.com (nextbus.com is the top entry in my most visited list).

Comment 3

[Chris Kitching [:ckitching]]

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.

Comment 4

[Chris Kitching [: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).

Comment 5

[:Margaret Leibovic]

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.

Comment 6

[:Margaret Leibovic]

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:
https://hg.mozilla.org/mozilla-central/rev/1fe12c9597f4

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.

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[:Margaret Leibovic]

Chris, any progress looking into this? We should try to fix this this week, or else back out the patches that caused the problem.

Comment 9

[Chris Kitching [:ckitching]]

This bug is not fixed by the patches from Bug 905685. My investigation continues.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Demadnessify the Favicon system

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.

Comment 11

[:Margaret Leibovic]

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:
https://hg.mozilla.org/mozilla-central/rev/1fe12c9597f4#l1.164

Comment 12

[Chris Kitching [:ckitching]]

Attachment: V2 - Demadnessify the Favicon system.

Yes - this was intentional - I thought this was requested in Bug 873243.
Never mind - replacing old behaviour.

Comment 13

[: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 14

[:Margaret Leibovic]

Comment on attachment 806733 [details] [diff] [review]
V2 - Demadnessify the Favicon system.

Excellent, thanks. I'll land this for you!

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/1df3efcf41b0

Please request approval to uplift to Aurora as well.

Comment 16

[Chris Kitching [:ckitching]]

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.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1df3efcf41b0

Comment 18

[Adrian Tamas (:AdrianT)]

Steps to reproduce on Aurora 26.0a2 2013-09-18/ Nighlty 27.0a1 2013-09-18:
1) Load news.google.com and bookmark the page
2) Bookmark a link from the context menu
3) Enter about:home and remove the news.google.com bookmark from the context menu

Actual results:
The unvisited bookmarks take the favicons of existing bookmarks.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f3e5fbcc0bf

Comment 20

[Flaviu Cos]

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.