Last Comment Bug 323377 - Lots of leaks in nsInternetSearchService
: Lots of leaks in nsInternetSearchService
Status: RESOLVED FIXED
[nvn-dl]
: fixed1.8.0.2, fixed1.8.1, mlk
Product: SeaMonkey
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-13 22:44 PST by Boris Zbarsky [:bz]
Modified: 2008-07-31 01:19 PDT (History)
8 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch? (2.36 KB, patch)
2006-01-14 15:01 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
axel: review-
Details | Diff | Review
Like this? (4.23 KB, patch)
2006-01-14 15:29 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
diff -w version of "Like this" (3.04 KB, patch)
2006-01-14 15:30 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
axel: review-
Details | Diff | Review
patch2 (4.30 KB, patch)
2006-01-14 15:35 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
patch2 diff -w (3.04 KB, patch)
2006-01-14 15:37 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
patch3 (4.30 KB, patch)
2006-01-14 15:53 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details | Diff | Review
patch3 diff -w (3.10 KB, patch)
2006-01-14 15:54 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
axel: review+
bzbarsky: superreview+
benjamin: approval‑branch‑1.8.1+
Details | Diff | Review
Merged to 1.8 branch (4.02 KB, patch)
2006-01-31 14:48 PST, Boris Zbarsky [:bz]
dveditz: approval1.8.0.2+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2006-01-13 22:44:29 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 06:15:02 PST
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 [:bsmedberg] 2006-01-14 09:06:21 PST
internetsearchservices is firefox+seamonkey
Comment 3 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:01:15 PST
Created attachment 208505 [details] [diff] [review]
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 2006-01-14 15:12:28 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:29:19 PST
Created attachment 208512 [details] [diff] [review]
Like this?

Ah, ok. Like this then perhaps?
Comment 6 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:30:12 PST
Created attachment 208513 [details] [diff] [review]
diff -w version of "Like this"
Comment 7 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:32:17 PST
Comment on attachment 208512 [details] [diff] [review]
Like this?

Sorry, forgot one.
Comment 8 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:35:25 PST
Created attachment 208514 [details] [diff] [review]
patch2
Comment 9 Axel Hecht 2006-01-14 15:36:15 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:37:05 PST
Created attachment 208515 [details] [diff] [review]
patch2 diff -w
Comment 11 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:53:06 PST
Created attachment 208518 [details] [diff] [review]
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 [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-14 15:54:12 PST
Created attachment 208519 [details] [diff] [review]
patch3 diff -w
Comment 13 Boris Zbarsky [:bz] 2006-01-15 08:50:01 PST
(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 [:bz] 2006-01-15 09:34:57 PST
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 [:bz] 2006-01-15 10:56:21 PST
Fixed on trunk.
Comment 16 Boris Zbarsky [:bz] 2006-01-15 10:56:48 PST
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 [:bz] 2006-01-31 10:37:38 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2006-01-31 13:14:51 PST
I don't have a branch build right now, so if you could do it, that would be great.
Comment 19 Boris Zbarsky [:bz] 2006-01-31 14:48:46 PST
Created attachment 210283 [details] [diff] [review]
Merged to 1.8 branch
Comment 20 Boris Zbarsky [:bz] 2006-01-31 14:49:18 PST
Fixed on 1.8 branch
Comment 21 Daniel Veditz [:dveditz] 2006-02-22 00:33:15 PST
Comment on attachment 210283 [details] [diff] [review]
Merged to 1.8 branch

approved for 1.8.0 branch, a=dveditz
Comment 22 Boris Zbarsky [:bz] 2006-02-22 20:31:17 PST
Fixed for 1.8.0.2
Comment 23 Dave Liebreich [:davel] 2006-03-01 16:30:36 PST
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.

Note You need to log in before you can comment on or make changes to this bug.