840989 - favicon doesn't refresh with connectivity after no connectivity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Karen Rudnitski [:kar]]

Not sure if the summary makes sense. Here's what happens on aurora on my samsung galaxy SIII.

STR:
- in an area of network connectivity (ie wifi), go to the bbc.co.uk/news and the bbc favicon appears. For fun, you can bookmark it. 
- you will see the correct favicon in the top sites or bookmarks list as a result.
- if you attempt to go to the bbc.co.uk/news but connectivity drops (ie wifi drops out), obviously the page doesn't load (as expected!).
- when going back to the bookmarks or top sites list, a yellow triangle with exclamation mark becomes the favicon. Which is fine, I guess.
- however, when I go back into an area with connectivity (wifi turns back on) and I go into the bookmarks (or top sites) list and tap to go to that site, the site loads but the favicon still doesn't refresh : the yellow triangle is still there. And has been for 2 days since continuously going back to that page within range.

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Nightly (02/13) - Screenshot

I can reproduce a Kar bug.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Very similar to bug 837365.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

This prevents us from storing favicons for error pages. I added a boolean for Tab.isErrorPage. I didn't add getters and setters for it, because I really hate them when they're just wrapping a private member, but I'd be happy to change that if I'm in the minority of that feeling.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 777937 [details] [diff] [review]
Patch

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

This sounds like a dupe of bug 751650. This patch mostly looks OK, but we should probably also make sure to prevent saving error titles while we're at it (see https://bugzilla.mozilla.org/show_bug.cgi?id=751650#c25). Do you think that would be simple enough to do here?

Something else to note is that this patch will prevent bad favicons from being stored in the future, but they won't fix bad favicons that have already been stored, like Karen's. For those, we'll need to have some kind of expiration logic for cached favicons (bug 699785).

::: mobile/android/base/Tab.java
@@ +65,5 @@
>      private Bitmap mThumbnailBitmap;
>      private boolean mDesktopMode;
>      private boolean mEnteringReaderMode;
>      private Context mAppContext;
> +    public boolean isErrorPage;

For consistency, this should be a private mIsErrorPage field, with a public isErrorPage() getter.

::: mobile/android/chrome/content/browser.js
@@ +1535,5 @@
>      });
>    },
>  };
>  
> +function NativeUI(aId) {

What is this NativeUI? I assume this is from another patch you have applied locally?

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 777937 [details] [diff] [review]
Patch

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

r-'ing for now just to get this out of my queue until the questions/comments in comment 4 are addressed.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> This sounds like a dupe of bug 751650. This patch mostly looks OK, but we
> should probably also make sure to prevent saving error titles while we're at
> it (see https://bugzilla.mozilla.org/show_bug.cgi?id=751650#c25). Do you
> think that would be simple enough to do here?

No. I think they're completely unrelated bugs.

> won't fix bad favicons that have
> already been stored, like Karen's. For those, we'll need to have some kind
> of expiration logic for cached favicons (bug 699785).

Good point. I think we have the same problem in the other bug, and maybe I can fix it there (although I'm not exactly sure how...)

> For consistency, this should be a private mIsErrorPage field, with a public
> isErrorPage() getter.

I'll update this patch, but I don't think we should tie these together.

Comment 7

[Wesley Johnston (:wesj)]

Started to update this and remembered I would up doing a more advanced error page enum in bug 808636. I'm going to wait and see what decision we make there before running on with this.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Comment 9

[Wesley Johnston (:wesj)]

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

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fbe6ff42af1a

Comment 11

[Jan Henning [:JanH]]

I've found this happening again in FF 34. After loading an error page, the adressbar favicon changes to a warning triangle and remains so until Firefox has been restarted.

I've also managed to replicate the OP's problem regarding bookmark favicons. While I think it didn't work immediately after bookmarking and then forcing an error page for m.bbc.com/news, after restarting Firefox (with connectivity still disabled) the BBC bookmark is now displaying the warning triangle as its favicon.

Unlike the OP's description, the problem fixes itself after yet another restart cycle - this time with connectivity enabled - however as long as you don't restart Firefox, the warning triangle persists both in the addressbar and on the bookmark page even after connectivity has been restored.