Closed Bug 171808 Opened 17 years ago Closed 9 years ago

More use of already_AddRefed, please!


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






(Reporter: bzbarsky, Assigned: bzbarsky)




(2 files, 2 obsolete files)

Two places that I've changed in my tree to return already_AddRefed instead of a
raw pointer:


I'll try to attach the patch tomorrow.
Whiteboard: [dev notes]
Attached patch patch (obsolete) — Splinter Review
reviews please?
Comment on attachment 101302 [details] [diff] [review]

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 =
Attachment #101302 - Flags: review+
Comment on attachment 101302 [details] [diff] [review]

r ==> sr
Attachment #101302 - Flags: review+ → superreview+
Comment on attachment 101302 [details] [diff] [review]

Mmmmmm, cleanup.
Attachment #101302 - Flags: review+
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?

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...
Attached patch Patch to nsStyleContext.cpp (obsolete) — Splinter Review
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
Comment on attachment 101844 [details] [diff] [review]
Patch to nsStyleContext.cpp

Attachment #101844 - Flags: superreview+
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

Goodbye fragile pointers.
Attachment #101844 - Flags: review+
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.
Attachment #101302 - Attachment is obsolete: true
Comment on attachment 101844 [details] [diff] [review]
Patch to nsStyleContext.cpp

Checked those two reviewed patches in.
Attachment #101844 - Attachment is obsolete: true
Whiteboard: [dev notes] → [whitebox]
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.
Keywords: helpwanted
Priority: -- → P5
Target Milestone: --- → Future
QA Contact: ian → style-system
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.
Assignee: bzbarsky → nobody
Closed: 9 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [whitebox]
Assignee: nobody → bzbarsky
You need to log in before you can comment on or make changes to this bug.