471018 - Warning: nsFaviconService.cpp: outparam 'aHasData' not written on NS_SUCCEEDED(return value)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[(no longer active)]

Attachment: Patch

toolkit/components/places/src/nsFaviconService.cpp:365: outparam 'aHasData' not written on NS_SUCCEEDED(return value)
   Blamed on Ehsan Akhgari <[e-mail]>: toolkit/components/places/src/nsFaviconService.cpp:302, revision e9b859357547ec900e25f3897b4787c055038302
toolkit/components/places/src/nsFaviconService.cpp:336: outparam declared here
   Blamed on Shawn Wilsher <[e-mail]>: toolkit/components/places/src/nsFaviconService.cpp:336, revision 005a32ae9d7a6d1161084b70518d71976abc6761

Comment 1

[Marco Bonardo [:mak]]

the privateBrowsing check is done after looking if the icon already exists, but there's no reason to do that, so can i suggest to move up the check before looking for the icon since we're not setting an icon and this change is hwv going to overwrite the result of the check.

Comment 2

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

Comment on attachment 354377 [details] [diff] [review]
Patch

Perhaps it would be best to set the outparam to false at the beginning of the function to avoid problems like this in the future?
 
The "we don't care if there was data" comment in nsFaviconService::SetFaviconUrlForPage() could use fixing (even though it's not related to this patch).

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 354377 [details] [diff] [review]
Patch

please address comments #1 and #2.

Comment 4

[(no longer active)]

Attachment: Patch (v2)

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 355149 [details] [diff] [review]
Patch (v2)

you also need to move the code to get the history service :)

r=me otherwise.

Comment 6

[(no longer active)]

(In reply to comment #5)
> (From update of attachment 355149 [details] [diff] [review])
> you also need to move the code to get the history service :)

Sorry about the obvious mistake!

Landed on mozilla-central <http://hg.mozilla.org/mozilla-central/rev/0c7455076caa>

Comment 7

[(no longer active)]

Comment on attachment 355149 [details] [diff] [review]
Patch (v2)

This fixes a compiler warning.  It's not an essential fix for 1.9.1, but it's not a risky one either.  Nominating for approval to land on branch.

Comment 8

[[e-mail]]

Is it really a compiler warning or a real possible random return value?

Comment 9

[(no longer active)]

Both I guess.

Comment 10

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

Comment on attachment 355149 [details] [diff] [review]
Patch (v2)

a191=beltzner

Comment 11

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1580afaac596