171808 - More use of already_AddRefed, please!

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

reviews please?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 101302 [details] [diff] [review]
patch

r ==> sr

Comment 4

[John Keiser (jkeiser)]

Comment on attachment 101302 [details] [diff] [review]
patch

r=jkeiser
Mmmmmm, cleanup.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Alec Flett]

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.

Comment 7

[Doug Turner (:dougt)]

>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(). 

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Doug Turner (:dougt)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Doug Turner (:dougt)]

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...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to nsStyleContext.cpp

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...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to nsHashtable

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 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 101844 [details] [diff] [review]
Patch to nsStyleContext.cpp

r/sr=dbaron

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Changes to nsSupportsArray (just the class itself and the idl, not the callers)

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...

Comment 16

[Alec Flett]

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 17

[John Keiser (jkeiser)]

Comment on attachment 101844 [details] [diff] [review]
Patch to nsStyleContext.cpp

r=jkeiser
Goodbye fragile pointers.

Comment 18

[John Keiser (jkeiser)]

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 :)

Comment 19

[John Keiser (jkeiser)]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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...

Comment 21

[Alec Flett]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 23

[Alec Flett]

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 :)

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 101844 [details] [diff] [review]
Patch to nsStyleContext.cpp

Checked those two reviewed patches in.

Comment 26

[Markus Hübner]

What is still there that needs to be done?

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.