273022 - early returns cause remaining voidarray members to leak

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[timeless]

yes this is a dupe, but i want someone else to fix it right, the first time.

the url contains code flow analysis, if you can't figure out what's going on
from the link then you aren't the right person to fix this bug. if you're
interested in fixing this bug, other developers on irc will gladly help.

Comment 1

[jpl24]

Attachment: patch_v1

Adds a simple helper object which deallocates the contents of the array when it goes out of scope.

Comment 2

[Jonathan Watt [:jwatt]]

Crap! The URL I use to get the list of my current bugs has prevented me from seeing this one. Totally forgot about it. Sorry!

jpl24: feel free to take this.

Comment 3

[David Bradley]

Minor nit, I think the helper class should be moved down to just above the function that uses it rather than several functions above.

If someone's ambitious enough, all this logic could be refactored better. I think the helper class could provide a nice abstraction and serve more than just cleanup. Have the constructor build the list and sort. Provide either an enumeration interface or array accessor. Then have the following loop use that instead of nsAutoVoidArray. But this patch is good as it is, since it takes care of the leak.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that when it works this should just use nsTArray<nsAutoPtr<pluginFileinDirectory>> to start with.  Then it will all Just Work.

Comment 5

[jpl24]

If the nsTArray functions like a standard library vector(I think it does), only reference counted pointers are allowed in an nsTArray. I remember some comments in the nsTArray bug talking about this issue, maybe that has changed since I read the bug. I had originally considered using nsRefPtrs, but I believe that would require the struct inheriting from nsISupports, and I thought the addrefing might slow down the sort.

The original version of the patch I wrote actually converted the nsVoidArray into an nsTArray, but I thought given that this code seemed pretty fragile it might be better to make the code changes as small as possible. So I trimmed the patch down to the bare minimum. I'm of course happy to redo the patch in whatever is thought to be best from a perf/code maintenance standpoint.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Right.  Hence the "when it works" part of my comment... ;)

Comment 7

[jpl24]

Whoops, reading was never my strength. This is probably the topic for another bug, but boost(C++ standard library extension), has a vector specifically for this purpose. I think it's called ptr_vector, and it doesn't do any reference counting but deletes all it's contained pointers memory when it's done. Handy for avoiding leaks without hurting performance.

http://www.boost.org/libs/ptr_container/doc/ptr_container.html

Comment 8

[David Bradley]

I'd be all for using Boost code. I'm not sure if there are any licensing issues or portabilities issues. The latter may be the kicker since, while Boost is portable, I'm not sure they cater to all the platforms Mozilla does. But this is no longer bug dicussion but architecture and should be discussed elsewhere.

Comment 9

[jpl24]

Attachment: patch_v2

Move definition of the helper class directly before the function. I agree there is definitely room for more use of the helper class, I'll file a follow up bug on that after the leak is fixed.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 208711 [details] [diff] [review]
patch_v2

r+sr=jst

Comment 11

[jpl24]

I don't have a CVS account, so if somebody could commit this when they get a chance that'd be great. Thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in on the trunk.  Thanks for the patch!