Two places that I've changed in my tree to return already_AddRefed instead of a raw pointer: nsICSSStyleRule::GetImportantRule nsHTMLValue::GetISupportsValue I'll try to attach the patch tomorrow.

reviews please?

Comment on attachment 101302 [details] [diff] [review] patch r=dbaron, although both sets of + nsCOMPtr<nsISupports> rule = aValue.GetISupportsValue(); + nsCOMPtr<nsICSSStyleRule> cssRule = do_QueryInterface(rule); could be rewritten, if you want, as nsCOMPtr<nsICSSRule> cssRule = do_QueryInterface(nsCOMPtr<nsISupports>(aValue.GetISupportsValue()));

Comment on attachment 101302 [details] [diff] [review] patch r ==> sr

Comment on attachment 101302 [details] [diff] [review] patch r=jkeiser Mmmmmm, cleanup.

OK. Prompted by discussion in bug 172030 I started looking for functions that returned an addrefed nsIFoo*. I've found about 300 possible candidates (based on a regexp match) of which I looked at about 30 so far. Of these 30, 12 actually returned an addrefed pointer. Of these 12, 4 had at least one caller that leaked the object. Not a good record. ;) I've converted those 12 over to already_AddRefed and I guess I'll go through the whole tree and perform this conversion... After that we can declare functions that return addrefed nsIFoo* illegal if we want. A few things I ran into as I did this: 1) 60% of the non-leaking callers had comments like "this addrefs". I left those in, but a better name for .get() per bug 172030 would really help the code clarity. 2) nsIAtom has functions like NS_NewAtom that return nsIAtom* (addrefed) and also the do_GetAtom wrappers around them which return already_AddRefed<nsIAtom>. Is there a reason to expose the former, really? 3) Is it OK to change xpcom glue code? (alecf, you may know this). Eg, nsMemory.h has a |static NS_COM nsIMemory* GetSomethingGlobal()| that addrefs. Is it acceptable to change the return value of that?

yikes! as for changing glue, yes its safe. the glue gets statically linked into an app, so it can be changed at will. cc'ing dougt for the specific change you're talking about.

>3) Is it OK to change xpcom glue code? (alecf, you may know this). Eg nsMemory.h has a |static NS_COM nsIMemory* GetSomethingGlobal()| that addrefs. why would you want to change this? I would rather see you fix callers of GetGlobalMemoryService to use NS_GetMemoryManager().

Doug, the point is that returning an addrefed nsIFoo* is very leak-prone because callers have to look at the implementation of your function to find out whether they're getting a strong or weak pointer... Hence it would be a good idea to completely eliminate such functions from our code if we can. If there's an altenative to GetGlobalMemoryService that behaves more "normally", would it make sense to eliminate GetGlobalMemoryService altogether? I'd be happy to convert callers to NS_GetMemoryManager(), then.

why are we talking about this: there is only one caller in lxr! :-)

Because I haven't lxr'ed yet (I'm doing devel on a machine with no web access and grep is slow... ;)). More seriously, what about the nsIAtom stuff? I know for a fact there are plenty of callers of that.

I am not sure, but the sytle of these already_AddRefed return result is u-g-l-y. I personally would just make the return pointer a out parameter -- but that is just me. Why didn't i do this when I created the nsIAtom.idl, I can't say...

I think it's better to do this in little bits that, in addition to being easier to review, are also each debatable on its own merits. This patch fixes GetParent on style contexts. The main effect is to make the code prettier...

This fixes nsSupportsHashtable's Get() method. I'm not sure what I think about this one, myself.... the way most callers use this method is to directly cast the pointer to some other type (not even QI). This is ugly no matter what, especially if they then want to assign into an nsCOMPtr.... This patch fixes two leaks in nsScriptSecurityManager and one in nsBindingManager.

Comment on attachment 101844 [details] [diff] [review] Patch to nsStyleContext.cpp r/sr=dbaron

This patch has issues similar to the nsHashtable one.... callers tend to just cast the nsISupports they get out directly. Those that _are_ QIing it should just move to do_QueryElementAt and they will be happy. The others end up having to use syntax like that in the nsHashtable patch (they already have to use it, btw). I'm not sure what people's take on this change will be. I think the IDL looks _ugly_ like this. In fact, I'm somewhat tempted to eliminate ElementAt on nsISupportsArray completely... Hence this is not a full patch fixing all callers but just a basis for discussion. Whatever we do, this change is one that should happen in some fashion, imo. I'm tired of having to look at the class definition all the time to see whether ElementAt is on a supportsarray or a voidarray... With this change that looking should be unnecessary -- the code will either work correctly or not compile. Oh, needless to say there are a few callers of ElementAt that leak...

Boris: I've investigated removing ElementAt() in the past, and switching callers to do_QueryElementAt() - its not actually too bad, I don't think there were that many callers. I'll review if you do the patch. :)

Comment on attachment 101844 [details] [diff] [review] Patch to nsStyleContext.cpp r=jkeiser Goodbye fragile pointers.

Alec: are you sure you were comparing to what we are doing in this situation? We're talking about doing ElementAt() to get the nsISupports and then *directly downcasting* to get the nsIMyInterface versus doing a QueryInterface. I am very dubious that that change would be small--you are competing with a virtual method call and at least a few IID compares. The cleaner way to handle the wicked casts is probably to move these classes over to be subclasses of an nsDoubleHashtable variant that does the same casting--probably map key to void*. One could also extend nsHashtable, of course, but if we're going to all that trouble we may as well use the new hashtable as well :)

OK, very poor segue in my comment. First paragraph speaks of nsSupportsArray changes, second paragraph speaks of nsHashtable.

Yeah. What jkeiser said. I assume the callers are doing this for perf, not just because they feel like it. Oh, and some callers are using REINTERPRET_CAST, not STATIC_CAST, as I said, and casting to a concrete class. There's no really good way to replace that with a QI...

another thought is to look at nsCOMArray<T> which I'm implementing in bug 162115 - you get type safety and accessors which don't AddRef, which is useful if you're only using the entries in the array with short-lived variables.

Ooh. I bet most of the users of nsISupportsArray _really_ want to be using that! For one thing, it handles all the casting for them. OK, I'll hold off on the array changes till that's in the tree. ;) Could we have a templatized hashtable too? ;)

yup, that was my thought. you'll even see I converted a few consumers and saw some reduction in addref/releases. bug 162115 should be landing today if the tree ever opens :)

Excellent. Then I have a few weeks to get this in shape for landing when the tree reopens for 1.3a.

Comment on attachment 101844 [details] [diff] [review] Patch to nsStyleContext.cpp Checked those two reviewed patches in.

What is still there that needs to be done?

Well, checking in the nsHashtable patches and nsSupportsArray patches would be a start... ;) I've actually updated all the callers at this point in my tree (a while ago); once I have time to clean up the multi-hundred-file patch that results I'll post it here. Whenever that is. This is low-priority for me, so it's not likely to happen soon; if you plan to work on this let me know and I'll assign the bug to you and mail you a tree- diff of my tree.

I don't think I care about this anymore.... And those patches likely bitrotted, if I can even find them. Resolving fixed for the things that actually landed.