Lots of leaks in nsInternetSearchService

RESOLVED FIXED

Status

SeaMonkey
Search
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: bz, Assigned: Martijn Wargers (dead))

Tracking

({fixed1.8.0.2, fixed1.8.1, mlk})

Trunk
fixed1.8.0.2, fixed1.8.1, mlk
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nvn-dl])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 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?
internetsearchservices is firefox+seamonkey
(Assignee)

Comment 3

12 years ago
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

12 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

12 years ago
Created attachment 208512 [details] [diff] [review]
Like this?

Ah, ok. Like this then perhaps?
(Assignee)

Comment 6

12 years ago
Created attachment 208513 [details] [diff] [review]
diff -w version of "Like this"
Attachment #208505 - Attachment is obsolete: true
(Assignee)

Comment 7

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

Sorry, forgot one.
Attachment #208512 - Attachment is obsolete: true
(Assignee)

Comment 8

12 years ago
Created attachment 208514 [details] [diff] [review]
patch2

Comment 9

12 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

12 years ago
Created attachment 208515 [details] [diff] [review]
patch2 diff -w
Attachment #208513 - Attachment is obsolete: true
(Assignee)

Comment 11

12 years ago
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.
Attachment #208514 - Attachment is obsolete: true
Attachment #208515 - Attachment is obsolete: true
(Assignee)

Comment 12

12 years ago
Created attachment 208519 [details] [diff] [review]
patch3 diff -w
(Reporter)

Comment 13

12 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

12 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

12 years ago
Attachment #208519 - Flags: review?(axel) → review+
(Reporter)

Updated

12 years ago
Assignee: search → martijn.martijn
(Reporter)

Comment 15

12 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 16

12 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?
Attachment #208519 - Flags: approval1.8.1? → branch-1.8.1+
(Reporter)

Comment 17

12 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

12 years ago
I don't have a branch build right now, so if you could do it, that would be great.
(Reporter)

Comment 19

12 years ago
Created attachment 210283 [details] [diff] [review]
Merged to 1.8 branch
(Reporter)

Comment 20

12 years ago
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+
(Reporter)

Comment 22

12 years ago
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.