498722 - nsIFaviconService.getFaviconLinkForIcon error on null argument

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Eddie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b99) Gecko/20090605 Firefox/3.5b99 GTB5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090612 Firefox/3.5

Calling nsIFaviconService.getFaviconLinkForIcon(null) returns 

NS_ERROR_ILLEGAL_VALUE on line XX: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFaviconService.getFaviconLinkForIcon]

instead of default favicon URI.

(cf https://developer.mozilla.org/en/nsifaviconservice)

Reproducible: Always

Steps to Reproduce:
1. Open Javascript Shell (extension developer tools) and enter "faviconService.getFaviconLinkForIcon(null).spec"
Actual Results:  
NS_ERROR_ILLEGAL_VALUE on line XX: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFaviconService.getFaviconLinkForIcon]

Expected Results:  
chrome://mozapps/skin/places/defaultFavicon.png

This problem appeared only in 3.5RC1 (did not exist in 3.5b99).

Possibly related to https://bugzilla.mozilla.org/show_bug.cgi?id=497563

It breaks the AutoDial extension.

Comment 1

[John P Baker]

(In reply to comment #0)
> Possibly related to https://bugzilla.mozilla.org/show_bug.cgi?id=497563

Indeed, seems to be one NS_ENSURE_ARG too many.

> It breaks the AutoDial extension.

and more importantly the idl.

Comment 2

[Shawn Wilsher :sdwilsh]

Yes, this is bad.  We should block on this :(

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Found a few other methods that we broke too.  sadface.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses comments on irc.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 383563 [details] [diff] [review]
v1.1

r=me

Comment 6

[Dietrich Ayala (:dietrich)]

you should also add a test for making sure that it returns the expected value.

Comment 7

[Shawn Wilsher :sdwilsh]

I think we should break this behavior on mozilla-central, but keep parity on 1.9.1 personally.  I won't be around the rest of the evening to check this in (or write that additional test) sadly.

Comment 8

[Marco Bonardo [:mak]]

i second that, this patch is good for 1.9.1, on trunk we should not allow null arguments to iconFailed methods.

Comment 9

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

AutoDial has about 10k ADUs, fwiw.

Do we know other add-ons we're likely to have broken with this change? Can we do a mxr search for that?

Comment 10

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a3e70614ee52

Comment 11

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

We've landed this on 1.9.1 in case we decide to respin the rc for it; right now it might hit the following addons:

https://addons.mozilla.org/en-US/firefox/addon/7849 - 40k ADU
https://addons.mozilla.org/en-US/firefox/addon/7765 - 1k ADU
https://addons.mozilla.org/en-US/firefox/addon/4999 - 200k ADU
https://addons.mozilla.org/en-US/firefox/addon/4096 - 1k ADU
https://addons.mozilla.org/en-US/firefox/addon/8615 - 10k ADU

One of those has 200k ADUs, but it's unclear if it would actually be affected by the problem.

Still mulling.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> https://addons.mozilla.org/en-US/firefox/addon/4999 - 200k ADU
> One of those has 200k ADUs, but it's unclear if it would actually be affected
> by the problem.
I checked the source code and can't find the method used anywhere in that add-on.  Could someone point me at the file it's supposed to be in and I'll let you know if it's going to break them?

Comment 13

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

Still needs to land on trunk.

Comment 14

[John P Baker]

(In reply to comment #3)
> Created an attachment (id=383560) [details]
> v1.0
> 
> Found a few other methods that we broke too.  sadface.

These others are Documentation errors. AddFailedFavicon and RemoveFailedFavicon would, AFAICT, crash with null argument before bug 497563 and isFailedFavicon has been throwing on null from way back when [bug 425064]
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsFaviconService.cpp#781

Comment 15

[Marco Bonardo [:mak]]

trunk patch will be different indeed, it should fix docs.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Still needs to land on trunk.
I'll have a trunk patch today, but it will not involve any code changes.  We'll be updating the documentation to not say we accept null values.

(In reply to comment #14)
> These others are Documentation errors.
While this is true, the documentation said one thing, and I'd like to follow that for 1.9.1 (not crashing isn't going to break add-ons either).

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #11)
> it might hit the following addons:
> 
> https://addons.mozilla.org/en-US/firefox/addon/7849 - 40k ADU

Favicon Picker isn't affected by this bug.

> https://addons.mozilla.org/en-US/firefox/addon/7765 - 1k ADU

IdentFavIcon is fine too.

> https://addons.mozilla.org/en-US/firefox/addon/4999 - 200k ADU

Same for Interclue.

> https://addons.mozilla.org/en-US/firefox/addon/4096 - 1k ADU

Fine too.

> https://addons.mozilla.org/en-US/firefox/addon/8615 - 10k ADU

And Autodial works again now. Looks good so far on all platforms with RC2. Marking as verified fixed on 1.9.1.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> it might hit the following addons:
> 
> https://addons.mozilla.org/en-US/firefox/addon/7849 - 40k ADU
> https://addons.mozilla.org/en-US/firefox/addon/7765 - 1k ADU
> https://addons.mozilla.org/en-US/firefox/addon/4999 - 200k ADU
> https://addons.mozilla.org/en-US/firefox/addon/4096 - 1k ADU
> https://addons.mozilla.org/en-US/firefox/addon/8615 - 10k ADU

This list was generated by searching for calls to any of the functions touched by attachment 383563 [details] [diff] [review]. Given comment 14, only 7849 and 8615 are actually potentially affected.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(That search only covered an old copy of the AMO database, too, so it's far from perfect.)

Comment 20

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

Can we get this fix landed on trunk, too, please?

Comment 21

[Marco Bonardo [:mak]]

trunk needs a different patch.

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: trunk v1.0

This fixes our documentation on trunk.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 384540 [details] [diff] [review]
trunk v1.0

please also fix

-nsresult
+NS_IMETHODIMP
nsFaviconService::GetFaviconLinkForIcon(nsIURI* aFaviconURI,

and 

nsFaviconService::IsFailedFavicon(nsIURI* aFaviconURI, PRBool* _retval)
{
   NS_ENSURE_ARG(aFaviconURI);
+  NS_ENSURE_ARG_POINTER(_retval);

r=me with the above.

Comment 24

[Shawn Wilsher :sdwilsh]

The documentation for those methods never said anything about NULL, but we did allow null to be passed in before.  That's why the branch patch maintained that behavior (to not break add-ons).

Comment 25

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/c25385c417b9

Comment 26

[Marco Bonardo [:mak]]

hm, you did not fix my comments though.

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> hm, you did not fix my comments though.
see comment 24?

Comment 28

[Marco Bonardo [:mak]]

i saw that, still i don't get how is that related to fixing a wrong usage of nsresult and a check on a returned pointer param...

Comment 29

[Shawn Wilsher :sdwilsh]

er, what?  Can you elaborate on your review comments?  I thought you were asking for me to update the documentation of those two methods.

Comment 30

[Marco Bonardo [:mak]]

yeah probably my bad, sorry for miscomprehension, expanding comments:

-nsresult
+NS_IMETHODIMP
nsFaviconService::GetFaviconLinkForIcon(nsIURI* aFaviconURI,

you fixed this wrong nsresult on branch, should be fixed on trunk too

nsFaviconService::IsFailedFavicon(nsIURI* aFaviconURI, PRBool* _retval)
{
   NS_ENSURE_ARG(aFaviconURI);
+  NS_ENSURE_ARG_POINTER(_retval);

this check on the return pointer validity should be added on trunk too

Comment 31

[John P Baker]

Do we need a dev-doc-needed keyword for the document from comment #0:
https://developer.mozilla.org/en/nsifaviconservice ?

Comment 32

[Shawn Wilsher :sdwilsh]

Yeah, docs need to be updated.

Comment 33

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

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 34

[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