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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Steve England [:stevee]]

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

[John P Baker]

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

Comment 2

[(no longer active)]

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.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 4

[(no longer active)]

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

Comment 5

[Damian Shaw [Quan]]

This is really annoying for pages which don't have favicons, because you are forever stuck with them being the error favicon.

Comment 7

[(no longer active)]

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

Trying to get a review from Seth...

Comment 8

[Seth Spitzer]

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

forwarding your review request on to dietrich.

Comment 9

[Dietrich Ayala (: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.

Comment 10

[(no longer active)]

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.  :-)

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 13

[(no longer active)]

(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.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 15

[(no longer active)]

(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 16

[Dietrich Ayala (:dietrich)]

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

r=me, thanks

Comment 17

[Damian Shaw [Quan]]

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?

Comment 18

[(no longer active)]

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.

Comment 19

[(no longer active)]

(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).

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a1.9=beltzner

Comment 21

[Dietrich Ayala (:dietrich)]

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

Comment 22

[John P Baker]

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.

Comment 23

[(no longer active)]

Attachment: Comment fix

(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!) :-)

Comment 24

[(no longer active)]

(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>

Comment 25

[John P Baker]

(In reply to comment #24)
> I did the documentation update:
> <http://developer.mozilla.org/en/docs/index.php?title=nsIFaviconService&diff=71183&oldid=60771>

Appears to be two documents: http://developer.mozilla.org/en/docs/Places:Favicon_Service

Comment 26

[(no longer active)]

(In reply to comment #25)
> Appears to be two documents:
> http://developer.mozilla.org/en/docs/Places:Favicon_Service

Done: <http://developer.mozilla.org/en/docs/index.php?title=Places%3AFavicon_Service&diff=71184&oldid=70794>

Comment 27

[Dietrich Ayala (:dietrich)]

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

[Kevin]

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.

Comment 29

[(no longer active)]

Can you provide a set of steps to reproduce?

Comment 30

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: 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.

Comment 31

[(no longer active)]

(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.

Comment 32

[AKIHIRO Misaki (a.k.a Kuden)]

(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."

Comment 33

[(no longer active)]

(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?

Comment 34

[AKIHIRO Misaki (a.k.a Kuden)]

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

No I cannot. I leave it to you. 

Comment 35

[Kevin]

(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.

Comment 37

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0

Comment 39

[Gervase Markham [:gerv]]

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