Site's favicon is replaced if browser error page is shown

VERIFIED FIXED in Firefox 3

Status

()

defect
P3
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: stevee, Assigned: Ehsan)

Tracking

({dev-doc-complete, polish, regression})

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments)

Reporter

Description

11 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042502 Minefield/3.0pre ID:2008042502

1. Bookmark a page that has a favicon (eg: www.mozilla.org ). Observe that your bookmark has the dinosaur favicon next to it.
2. Unplug your network cable
3. Click on the bookmark

Expected:
- You get taken to a Firefox error page, and the bookmark's favicon stays as the dinosaur

Actual:
- You get taken to a Firefox error page, but the bookmark's favicon changes to the yellow /!\ icon from the error page.

This might be a dupe of bug 350440.

Comment 1

11 years ago
This could even be a regression from bug 429721 - IIRC data favicons aren't stored in database but chrome ones are.

http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsFaviconService.cpp#459

Updated

11 years ago
Depends on: 429721
Keywords: regression
OS: Windows XP → All
Hardware: PC → All
Assignee

Updated

11 years ago
Assignee: nobody → ehsan.akhgari
Assignee

Updated

11 years ago
Status: NEW → ASSIGNED
Assignee

Comment 2

11 years ago
Nominating as blocking, because this causes the favicon on the users' bookmarks to change inadvertently, especially for users on slow dial-up connections who may get network related errors often.  The fix is straightforward, and a patch is coming up.  This should at least pick up wanted-firefox3.
Flags: blocking-firefox3?
Assignee

Comment 3

11 years ago
Posted patch Patch (v1)Splinter Review
Simply check for the URI of the error page favicons instead of blacklisting all data: URI favicons.  A comment is added to netError.xhtml to keep the favicon used there in sync with the one used in Places.
Attachment #317840 - Flags: review?
Assignee

Updated

11 years ago
Attachment #317840 - Flags: review? → review?(dietrich)
Assignee

Updated

11 years ago
Whiteboard: [has patch][needs review dietrich]
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Assignee

Comment 4

11 years ago
Dietrich: will you be able to review this in time for RC1?

Comment 5

11 years ago
This is really annoying for pages which don't have favicons, because you are forever stuck with them being the error favicon.
Duplicate of this bug: 431593
Assignee

Updated

11 years ago
Blocks: 429721
No longer depends on: 429721
Assignee

Comment 7

11 years ago
Comment on attachment 317840 [details] [diff] [review]
Patch (v1)

Trying to get a review from Seth...
Attachment #317840 - Flags: review?(dietrich) → review?(seth)

Comment 8

11 years ago
Comment on attachment 317840 [details] [diff] [review]
Patch (v1)

forwarding your review request on to dietrich.
Attachment #317840 - Flags: review?(seth) → review?(dietrich)
Ehsan, apologies for not responding sooner to your request for review. However, this was marked blocking- by the release drivers, so is not at the top of my priority list. I'll get to it as soon as I can.
Assignee

Comment 10

11 years ago
Thanks.  The haste is because the problem it would cause for a lot of users if RC1 ships without having this fixed.  Anyway I understand that there are more serious stuff for you to work on, so I appreciate your review on this when you can.  :-)
Duplicate of this bug: 431998
After getting set to the error page icon, if the network connection is restored and the user browses back, the favicon is properly updated right?

If that's the case then I still think that this doesn't block. While I understand that it's bad for pages without favicons, that's a smaller set of sites and will affect fewer users IMO.

If that's not the case, then this should be renominated for blocking.
Assignee

Comment 13

11 years ago
(In reply to comment #12)
> After getting set to the error page icon, if the network connection is restored
> and the user browses back, the favicon is properly updated right?

Only if the page *has* a favicon, otherwise the new incorrect warning favicon is going to stay there forever.  See comment 5.

> If that's the case then I still think that this doesn't block. While I
> understand that it's bad for pages without favicons, that's a smaller set of
> sites and will affect fewer users IMO.
> 
> If that's not the case, then this should be renominated for blocking.

Renominating for blocking as per comment 5 and 12.
Flags: blocking-firefox3- → blocking-firefox3?
I read comment 5, did you read comment 12? I said that if this only affects pages without favicons, then I don't think it blocks. Wanted, not blocking.
Flags: blocking-firefox3? → blocking-firefox3-
Assignee

Comment 15

11 years ago
(In reply to comment #14)
> I read comment 5, did you read comment 12? I said that if this only affects
> pages without favicons, then I don't think it blocks. Wanted, not blocking.

Oops, sorry; I misread comment 12.  Gotta get some sleep...  Sorry for the bugspam.
Comment on attachment 317840 [details] [diff] [review]
Patch (v1)

r=me, thanks
Attachment #317840 - Flags: review?(dietrich) → review+
A problem I've come across, is if a site's favicon gets input via javascript then it isn't updated in the bookmarks and the error favicon stays there forever.

Given this bug will make that invalid, should I make a new bug or just leave it as another reason to get this fixed?
Assignee

Comment 18

11 years ago
Comment on attachment 317840 [details] [diff] [review]
Patch (v1)

This is an annoying bug, with a relatively simple fix.  Requesting approval.  I hope we can get this landed for RC1.
Attachment #317840 - Flags: approval1.9?
Assignee

Comment 19

11 years ago
(In reply to comment #17)
> A problem I've come across, is if a site's favicon gets input via javascript
> then it isn't updated in the bookmarks and the error favicon stays there
> forever.
> 
> Given this bug will make that invalid, should I make a new bug or just leave it
> as another reason to get this fixed?

I think that is a separate problem.  It would be good if you can file a new bug with a test case (or at least, a demo site which exhibits the problem).
Whiteboard: [has patch][needs review dietrich] → [has patch][has review][needs approval]
Comment on attachment 317840 [details] [diff] [review]
Patch (v1)

a1.9=beltzner
Attachment #317840 - Flags: approval1.9? → approval1.9+
Assignee

Updated

11 years ago
Keywords: checkin-needed
Whiteboard: [has patch][has review][needs approval] → [has patch][has review][has approval]
Checking in docshell/resources/content/netError.xhtml;
/cvsroot/mozilla/docshell/resources/content/netError.xhtml,v  <--  netError.xhtml
new revision: 1.32; previous revision: 1.31
done
Checking in toolkit/components/places/src/nsFaviconService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsFaviconService.cpp,v  <--  nsFaviconService.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/places/src/nsFaviconService.h;
/cvsroot/mozilla/toolkit/components/places/src/nsFaviconService.h,v  <--  nsFaviconService.h
new revision: 1.11; previous revision: 1.10
done
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Priority: -- → P3
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Target Milestone: --- → Firefox 3

Comment 22

11 years ago
Comment on attachment 317840 [details] [diff] [review]
Patch (v1)

If I read this correctly, you have overlooked the idl file; The sentence at

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/places/public/nsIFaviconService.idl&rev=1.6&mark=84#80

needs to be removed. Of course then the on-line documentation will need updating so need to add 'dev-doc-needed' to the keywords.
Assignee

Comment 23

11 years ago
Posted patch Comment fixSplinter Review
(In reply to comment #22)
> (From update of attachment 317840 [details] [diff] [review] [details])
> If I read this correctly, you have overlooked the idl file; The sentence at
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/places/public/nsIFaviconService.idl&rev=1.6&mark=84#80
> 
> needs to be removed.

You're right.  Here is a trivial patch to fix the comments.  Requesting review (shouldn't take more than a couple of seconds!) :-)
Attachment #319941 - Flags: review?
Assignee

Updated

11 years ago
Attachment #319941 - Flags: review? → review?(dietrich)
Assignee

Comment 24

11 years ago
(In reply to comment #22)
> Of course then the on-line documentation will need
> updating so need to add 'dev-doc-needed' to the keywords.

I did the documentation update: <http://developer.mozilla.org/en/docs/index.php?title=nsIFaviconService&diff=71183&oldid=60771>
Attachment #319941 - Flags: review?(dietrich) → review+
comment change checked in

Checking in toolkit/components/places/public/nsIFaviconService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsIFaviconService.idl,v  <--  nsIFaviconService.idl
new revision: 1.8; previous revision: 1.7
done

Comment 28

11 years ago
I haven't been following this too closely, but though this may be fixed in places, I'm seeing the exact same bug in RSS feeds.
Assignee

Comment 29

11 years ago
Can you provide a set of steps to reproduce?
Posted image screenshot
{Build ID: 2008051003} new profile

1. go and bookmark this. ftp://ftp.mozilla.org/pub/
2. favicon is firefox's ftp icon. This is not Site/default's favicon.
Assignee

Comment 31

11 years ago
(In reply to comment #30)
> Created an attachment (id=320373) [details]
> screenshot
> 
> {Build ID: 2008051003} new profile
> 
> 1. go and bookmark this. ftp://ftp.mozilla.org/pub/
> 2. favicon is firefox's ftp icon. This is not Site/default's favicon.

FTP sites do not have favicons, as we know them from HTTP/HTTPS sites.  Therefore, this behavior is expected.
(In reply to comment #31)
> FTP sites do not have favicons, as we know them from HTTP/HTTPS sites. 
> Therefore, this behavior is expected.

Well... Do you say this?

"On Bookmarks, FTP and local directory use Firefox's special favicon.
These not use Firefox default bookmark favicon.
This is not a bug. This is Firefox 3 new feature."
Assignee

Comment 33

11 years ago
(In reply to comment #32)
> (In reply to comment #31)
> > FTP sites do not have favicons, as we know them from HTTP/HTTPS sites. 
> > Therefore, this behavior is expected.
> 
> Well... Do you say this?
> 
> "On Bookmarks, FTP and local directory use Firefox's special favicon.
> These not use Firefox default bookmark favicon.
> This is not a bug. This is Firefox 3 new feature."

Can you file a follow up bug for this?
(In reply to comment #33)
> Can you file a follow up bug for this?

No I cannot. I leave it to you. 

Comment 35

11 years ago
(In reply to comment #29)
> Can you provide a set of steps to reproduce?
> 

Nevermind on mine - I cannot recreate the issue, although I have a few bookmarks who's favicons were previously changed to the error icon and will not revert. I cannot, however, seem to get any to change now.
Duplicate of this bug: 433244
verified with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0
Status: RESOLVED → VERIFIED
Duplicate of this bug: 430982
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.