922116 - Favicons are blank after changing the orientation of the device

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[u421692]

Environment:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox Nightly 27.0a1 (2013-09-30)

Steps to reproduce:
1. Start Firefox
2. Go to History and Bookmarks
3. Change the device orientation
4. Observe the favicons

Expected result:
Favicons are loaded correctly 

Actual result:
Favicons are blank until restarting Firefox

Comment 1

[Aaron Train [:aaronmt]]

Probably a regression from bug 905685, but needs a proper window here.

Comment 2

[u421692]

Regression window:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=6b28fcac0f06&tochange=061c17bbab44

Bug 918917 caused this regression.

Comment 3

[u421692]

fx-team branch:
good build: 1380278973
bad build: 1380280413

Comment 4

[Aaron Train [:aaronmt]]

I can reproduce this as well on Nightly (10/01).

Comment 5

[:Margaret Leibovic]

I'm seeing blank favicons on a local build of m-c without even changing the device orientation :(

I'll try to debug this, but feel free to jump in if you have suggestions!

Comment 6

[Chris Kitching [:ckitching]]

For what it's worth, this problem (Still) seems not to exist with the patch from Bug 914296.

I'll let you know if I turn up any information useful in solving this problem in isolation. Apologies for creating more work - I wouldn't suggest putting too much time into it - this problem exists entirely because of race conditions between Bug 914296 and other attempted changes to the favicon system. Since that seems to solve this problem and seems near to being Newman-proof this is all about to get trampled anyway.

Comment 7

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #6)
> For what it's worth, this problem (Still) seems not to exist with the patch
> from Bug 914296.
> 
> I'll let you know if I turn up any information useful in solving this
> problem in isolation. Apologies for creating more work - I wouldn't suggest
> putting too much time into it - this problem exists entirely because of race
> conditions between Bug 914296 and other attempted changes to the favicon
> system. Since that seems to solve this problem and seems near to being
> Newman-proof this is all about to get trampled anyway.

This isn't really an acceptable solution - this is a problem on aurora now, and I highly doubt we're going to uplift bug 914296 to aurora.

I'm actually considering just trying to revert the FaviconView changes from bug 888326, even if that means removing the updateAndScaleImage method. Since that method's only used for search engine icons, I would rather us just have smaller search engine icons with colored backgrounds, than have missing and/or wrong favicons all over the place.

Comment 8

[Chris Kitching [:ckitching]]

(In reply to :Margaret Leibovic from comment #7)
> I'm actually considering just trying to revert the FaviconView changes from
> bug 888326, even if that means removing the updateAndScaleImage method.
> Since that method's only used for search engine icons, I would rather us
> just have smaller search engine icons with colored backgrounds, than have
> missing and/or wrong favicons all over the place.

This seems sensible, but laborious.
I shall attempt to find a solution for this problem on its own now.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Clear favicons without deleting useful data.

This appears to make Favicons no longer vanish upon rotation.

Comment 10

[:Margaret Leibovic]

Comment on attachment 812875 [details] [diff] [review]
Clear favicons without deleting useful data.

I want lucasr to look at this, too.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: V2 - Clear favicons without deleting useful data.

... Culling that debugging code...

Comment 12

[Sriram Ramasubramanian [:sriram]]

fwiw, http://stackoverflow.com/questions/2859212/how-to-clear-an-imageview-in-android 
setImageResource(0) might not clear the image sometimes. I am facing that while using smoothie.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> fwiw,
> http://stackoverflow.com/questions/2859212/how-to-clear-an-imageview-in-
> android 
> setImageResource(0) might not clear the image sometimes. I am facing that
> while using smoothie.

This might be because setImageResource will only update if the given resource id is different. setImageBitmap, on the other hand, will unconditionally set the internal drawable for the ImageView.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 812883 [details] [diff] [review]
V2 - Clear favicons without deleting useful data.

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

Tested this patch locally. Seems to fix the issue.

Looks good (with the suggested change).

::: mobile/android/base/widget/FaviconView.java
@@ +158,5 @@
>          hideBackground();
>      }
>  
> +    private void showNoImage() {
> +        setImageResource(0);

Replace this with setImageBitmap(null) to ensure this will actually clear the ImageView's content.

Comment 15

[:Margaret Leibovic]

Comment on attachment 812883 [details] [diff] [review]
V2 - Clear favicons without deleting useful data.

This seems reasonable, and I wasn't able to find any problems when testing.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6d76e2fac7a7

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/6d76e2fac7a7

Comment 18

[:Margaret Leibovic]

QA folks, can you help us verify that this fixes the blank favicon issue on Nightly? If so, I'll request approval to uplift this to aurora along with bug 918917.

Comment 19

[u421692]

Verified as fixed on Nightly 27.0a1 (2013-10-04) on Samsung Galaxy R (Android 2.3.5) and Samsung Galaxy Tab (Android 4.0.4).

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this is needed on FF26 along with bug 918917, please nominate for uplift approval with risk evaluation.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 812883 [details] [diff] [review]
V2 - Clear favicons without deleting useful data.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 918917
User impact if declined: Blank favicons in about:home after device rotation
Testing completed (on m-c, etc.): Landed in m-c, verified to fix the bug.
Risk to taking this patch (and alternatives if risky): Fair low, small change in FaviconView to properly handle the async tasks in about:home lists.
String or IDL/UUID changes made by this patch: n/a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/290d6d1241d5

Comment 23

[u421692]

Verified as fixed on Aurora 26.0a2 (2013-10-14) on HTC Desire HD (Android 2.3.5)