Closed Bug 498722 Opened 11 years ago Closed 11 years ago

nsIFaviconService.getFaviconLinkForIcon error on null argument

Categories

(Firefox :: Bookmarks & History, defect)

3.5 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.6a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: iddo_s, Assigned: sdwilsh)

References

Details

(4 keywords)

Attachments

(2 files, 1 obsolete file)

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.
Version: unspecified → 3.5 Branch
(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.
Flags: blocking-firefox3.5?
Yes, this is bad.  We should block on this :(
Assignee: nobody → sdwilsh
Blocks: 497563
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch v1.0 (obsolete) — Splinter Review
Found a few other methods that we broke too.  sadface.
Attachment #383560 - Flags: review?(dietrich)
Attached patch v1.1Splinter Review
Addresses comments on irc.
Attachment #383560 - Attachment is obsolete: true
Attachment #383560 - Flags: review?(dietrich)
Attachment #383563 - Flags: review?(dietrich)
Comment on attachment 383563 [details] [diff] [review]
v1.1

r=me
Attachment #383563 - Flags: review?(dietrich) → review+
Flags: in-testsuite?
you should also add a test for making sure that it returns the expected value.
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.
i second that, this patch is good for 1.9.1, on trunk we should not allow null arguments to iconFailed methods.
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?
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.
(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?
Still needs to land on trunk.
Flags: blocking-firefox3.6+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5+
(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
trunk patch will be different indeed, it should fix docs.
(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).
(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.
(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.
(That search only covered an old copy of the AMO database, too, so it's far from perfect.)
Can we get this fix landed on trunk, too, please?
trunk needs a different patch.
Attached patch trunk v1.0Splinter Review
This fixes our documentation on trunk.
Attachment #384540 - Flags: review?(dietrich)
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.
Attachment #384540 - Flags: review?(dietrich) → review+
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).
Whiteboard: [can land]
http://hg.mozilla.org/mozilla-central/rev/c25385c417b9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → Firefox 3.6a1
hm, you did not fix my comments though.
(In reply to comment #26)
> hm, you did not fix my comments though.
see comment 24?
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...
er, what?  Can you elaborate on your review comments?  I thought you were asking for me to update the documentation of those two methods.
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
Do we need a dev-doc-needed keyword for the document from comment #0:
https://developer.mozilla.org/en/nsifaviconservice ?
Yeah, docs need to be updated.
Keywords: dev-doc-needed
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)
Keywords: fixed1.9.2
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.