323377 - Lots of leaks in nsInternetSearchService

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There's lots of nsISupportsArray misuse in this file.  In particular any line like:

  nsCOMPtr<nsISupports> isupports = mUpdateArray->ElementAt(0);

leaks.  It should be using dont_AddRef or something.  Or better yet using do_QueryElementAt accordingly if it doesn't need the actual nsISupports pointer.  Perhaps mUpdateArray shouldn't even be an nsISupportsArray?

I have this "fixed" in my tree (as part of a general "remove nsISupports::ElementAt" kinda thing), but that can't go into the mozilla.org tree, so this should be fixed for real.  Ccing some people who may be interested; if none of you guys want to do it, I guess I can.

Comment 1

[Martijn Wargers (dead)]

Isn't this SeaMonkey only (which I currently don't have a build of)?
http://lxr.mozilla.org/seamonkey/search?string=nsInternetSearchService
Or isn't that a problem?

Comment 2

[Benjamin Smedberg]

internetsearchservices is firefox+seamonkey

Comment 3

[Martijn Wargers (dead)]

Attachment: patch?

Well, this compiles. No idea if it solves the memory leak, though. How can I see these kinds of memory leaks on debug builds?

Comment 4

[Axel Hecht]

Comment on attachment 208505 [details] [diff] [review]
patch?

As the nsISupports pointers aren't used, do_QueryElement should go directly into
aRes, aSearchRoot, src
respectively.

Wow, what a crappy code format, giving aFoo names to local vars. But I don't think we should fix that in this bug.
The final patch will call for a diff -w, sadly. As you break the source history there, you could just change the aRes to be just res or so.

Comment 5

[Martijn Wargers (dead)]

Attachment: Like this?

Ah, ok. Like this then perhaps?

Comment 6

[Martijn Wargers (dead)]

Attachment: diff -w version of "Like this"

Comment 7

[Martijn Wargers (dead)]

Comment on attachment 208512 [details] [diff] [review]
Like this?

Sorry, forgot one.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch2

Comment 9

[Axel Hecht]

Comment on attachment 208513 [details] [diff] [review]
diff -w version of "Like this"

In the first chunk, you changed from get(0), remove(0) to remove(0), get(0).

I'm not sure if I'm the right one to r+ this, I guess bz should take that, or sr+ to make sure it's fixing what he thinks should be fixed, I'm just scratching the surface here.

Comment 10

[Martijn Wargers (dead)]

Attachment: patch2 diff -w

Comment 11

[Martijn Wargers (dead)]

Attachment: patch3

> In the first chunk, you changed from get(0), remove(0) to remove(0), get(0).
Oops, sorry about that.

Comment 12

[Martijn Wargers (dead)]

Attachment: patch3 diff -w

Comment 13

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #3)
> How can I see these kinds of memory leaks on debug builds?

Set XPCOM_MEM_LEAK_LOG to a filename to log leaks to, then start the browser, do stuff, and quit.  Look at the leak list. Note that just starting up and shutting down leaks _some_, so make sure to do a baseline measurement.  This works in general to detect leaks of XPCOM objects.

To reproduce one of the leaks here (the one in 
InternetSearchDataSource::GetSearchEngineToPing), make sure that your firefox doesn't have the google search bar in the toolbar.  Then hit Ctrl-K, and from the resulting dialog search (for "test", say).  Then wait at least 60 seconds (or munge SEARCH_UPDATE_TIMEOUT to something shorter beforehand or something) and quit.  Should nicely leak an RDF object.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208519 [details] [diff] [review]
patch3 diff -w

This fixes the leaks in question.  Axel, if you could ok it in context of the code in general, I'll land it.

Martijn, thanks again!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208519 [details] [diff] [review]
patch3 diff -w

I think it's worth fixing this on the branches... This should be a very safe fix, and leaks are a problem we're trying to deal with.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That patch doesn't apply to the 1.8 branch.  Martijn, do you have time to merge it to there, or do you want me to do it?

Comment 18

[Martijn Wargers (dead)]

I don't have a branch build right now, so if you could do it, that would be great.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to 1.8 branch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 210283 [details] [diff] [review]
Merged to 1.8 branch

approved for 1.8.0 branch, a=dveditz

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.0.2

Comment 23

[Dave Liebreich [:davel]]

marking [nvn-dl], which removes this bug from the "to be verified by QA" list
for Firefox 1.5.0.2.  I'm not sure how we should verify individual leak fixes - I'm open to suggestions.