favicon doesn't refresh with connectivity after no connectivity

RESOLVED FIXED in Firefox 26

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: kar, Assigned: wesj)

Tracking

20 Branch
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(fennec+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Created attachment 713670 [details]
Nightly (02/13) - Screenshot

I can reproduce a Kar bug.
Very similar to bug 837365.

Updated

5 years ago
tracking-fennec: ? → +
(Assignee)

Comment 3

5 years ago
Created attachment 777937 [details] [diff] [review]
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.
Attachment #777937 - Flags: review?(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 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.
Attachment #777937 - Flags: review?(bnicholson) → review-
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 808636
(Assignee)

Comment 8

4 years ago
Created attachment 792470 [details] [diff] [review]
Patch v2
Attachment #777937 - Attachment is obsolete: true
Attachment #792470 - Flags: review?(bnicholson)
Attachment #792470 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/fbe6ff42af1a
https://hg.mozilla.org/mozilla-central/rev/fbe6ff42af1a
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26

Comment 11

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.