Closed
Bug 323377
Opened 18 years ago
Closed 18 years ago
Lots of leaks in nsInternetSearchService
Categories
(SeaMonkey :: Search, defect)
SeaMonkey
Search
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)
4.30 KB,
patch
|
Details | Diff | Splinter Review | |
3.10 KB,
patch
|
axel
:
review+
bzbarsky
:
superreview+
benjamin
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
4.02 KB,
patch
|
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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•18 years ago
|
||
internetsearchservices is firefox+seamonkey
Assignee | ||
Comment 3•18 years ago
|
||
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•18 years ago
|
||
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-
Assignee | ||
Comment 5•18 years ago
|
||
Ah, ok. Like this then perhaps?
Assignee | ||
Comment 6•18 years ago
|
||
Attachment #208505 -
Attachment is obsolete: true
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 208512 [details] [diff] [review] Like this? Sorry, forgot one.
Attachment #208512 -
Attachment is obsolete: true
Assignee | ||
Comment 8•18 years ago
|
||
Comment 9•18 years ago
|
||
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-
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #208513 -
Attachment is obsolete: true
Assignee | ||
Comment 11•18 years ago
|
||
> 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
Assignee | ||
Comment 12•18 years ago
|
||
Reporter | ||
Comment 13•18 years ago
|
||
(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.
Reporter | ||
Comment 14•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #208519 -
Flags: review?(axel) → review+
Reporter | ||
Updated•18 years ago
|
Assignee: search → martijn.martijn
Reporter | ||
Comment 15•18 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 16•18 years ago
|
||
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?
Updated•18 years ago
|
Attachment #208519 -
Flags: approval1.8.1? → branch-1.8.1+
Reporter | ||
Comment 17•18 years ago
|
||
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?
Assignee | ||
Comment 18•18 years ago
|
||
I don't have a branch build right now, so if you could do it, that would be great.
Reporter | ||
Comment 19•18 years ago
|
||
Comment 21•18 years ago
|
||
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+
Updated•18 years ago
|
Attachment #208519 -
Flags: approval1.8.0.2?
Updated•18 years ago
|
Flags: blocking1.8.0.2+
Comment 23•18 years ago
|
||
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]
Updated•16 years ago
|
Product: Core → SeaMonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•