Closed Bug 323377 Opened 15 years ago Closed 15 years ago

Lots of leaks in nsInternetSearchService

Categories

(SeaMonkey :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: martijn.martijn)

Details

(Keywords: fixed1.8.0.2, fixed1.8.1, memory-leak, Whiteboard: [nvn-dl])

Attachments

(3 files, 5 obsolete files)

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.
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?
internetsearchservices is firefox+seamonkey
Attached patch patch? (obsolete) — Splinter Review
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 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.
Attachment #208505 - Flags: review-
Attached patch Like this? (obsolete) — Splinter Review
Ah, ok. Like this then perhaps?
Attached patch diff -w version of "Like this" (obsolete) — Splinter Review
Attachment #208505 - Attachment is obsolete: true
Comment on attachment 208512 [details] [diff] [review]
Like this?

Sorry, forgot one.
Attachment #208512 - Attachment is obsolete: true
Attached patch patch2 (obsolete) — Splinter Review
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.
Attachment #208513 - Flags: review-
Attached patch patch2 diff -w (obsolete) — Splinter Review
Attachment #208513 - Attachment is obsolete: true
Attached patch patch3Splinter Review
> In the first chunk, you changed from get(0), remove(0) to remove(0), get(0).
Oops, sorry about that.
Attachment #208514 - Attachment is obsolete: true
Attachment #208515 - Attachment is obsolete: true
Attached patch patch3 diff -wSplinter Review
(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 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!
Attachment #208519 - Flags: superreview+
Attachment #208519 - Flags: review?(axel)
Attachment #208519 - Flags: review?(axel) → review+
Assignee: search → martijn.martijn
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Attachment #208519 - Flags: approval1.8.1?
Attachment #208519 - Flags: approval1.8.0.2?
Attachment #208519 - Flags: approval1.8.1? → branch-1.8.1+
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?
I don't have a branch build right now, so if you could do it, that would be great.
Fixed on 1.8 branch
Keywords: fixed1.8.1
Comment on attachment 210283 [details] [diff] [review]
Merged to 1.8 branch

approved for 1.8.0 branch, a=dveditz
Attachment #210283 - Flags: approval1.8.0.2+
Attachment #208519 - Flags: approval1.8.0.2?
Flags: blocking1.8.0.2+
Fixed for 1.8.0.2
Keywords: fixed1.8.0.2
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.
Whiteboard: [nvn-dl]
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.