Closed
Bug 59414
Opened 24 years ago
Closed 24 years ago
misuse of nsCOMPtr::operator&
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla0.8
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [xptest])
Attachments
(21 files)
There's a bit of code that uses & instead of getter_AddRefs. Most of this code is dealing with an nsCOMPtr that is null, so it doesn't leak. However, it's still wrong and it confuses the leak loging. If the nsCOMPtr isn't null, then it's an automatic leak. I'm going to attach: (1) a patch that makes nsCOMPtr::operator& private and provides nsCOMPtr::get_address() for legitimate users (2) a patch that fixes all the incorrect uses of nsCOMPtr::operator& (3) a patch that changes all the legitimate (?) users to use get_address() (2) should definitely get checked in (possibly with modifications, of course). We should discuss what we want to do with (1) and (3).
Assignee | ||
Comment 1•24 years ago
|
||
Assignee | ||
Comment 2•24 years ago
|
||
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
Comment 5•24 years ago
|
||
The only thing I don't like is that |get_address| is a member function. I know this sounds nit-picky, but I would prefer non-member functions like so template <class T> inline nsCOMPtr<T>* address_of( nsCOMPtr<T>& aPtr ) { return &aPtr; } template <class T> inline const nsCOMPtr<T>* address_of( const nsCOMPtr<T>& aPtr ) { return &aPtr; } These two template functions would be |friend|s of |nsCOMPtr|. It's silly, I know. I just prefer |address_of(fooP)| over |fooP.get_address()|. I could probably be argued out of this ... but I have always disliked excessive member functions in smart-pointers, because they break you out of the idiom of a smart pointer. I made |get| a member, because that is a member of |std::auto_ptr|, but when faced with adding new members, I put |getter_AddRefs|, et al, outside the class. The reason I could possibly be argued out of this is because we may _want_ to break out of the idiom to prevent this bug ... it just makes me feel oogy.
Assignee | ||
Comment 7•24 years ago
|
||
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Changing all the usage to address_of() was easy -- I just had to do a search replace on the patch, unapply the old one, and apply the new one. However, I couldn't get the necessary changes to nsCOMPtr.h to compile. I had to introduce a bunch of forward declarations (which I suspect may not be portable, considering some of the other comments in nsCOMPtr.h), but I still failed with: /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h: In function `nsCOMPtr<T> *address_of (nsCOMPtr<T> &) [with T = nsISupports]': /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h:994: instantiated from here /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h:838: `const nsCOMPtr<nsISupports> *nsCOMPtr<nsISupports>::operator& () const' is private /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h:902: within this context /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h:902: cannot convert `const nsCOMPtr<nsISupports> *' to `nsCOMPtr<nsISupports> *' in return The changes I used are attached above.
Assignee | ||
Comment 10•24 years ago
|
||
And once I fix the obvious mistake (s/nsISupports/T/g) in the forward declarations, I get different errors: /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h: In function `nsCOMPtr<T> *address_of (nsCOMPtr<T> &) [with T = nsISupports]': /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h:994: instantiated from here /builds/seamonkey/mozilla/xpcom/base/nsCOMPtr.h:902: cannot convert `const nsCOMPtr<nsISupports> *' to `nsCOMPtr<nsISupports> *' in return
Comment 11•24 years ago
|
||
This may sound goofy, but you could keep the public member function |get_address| and implement |address_of| in terms of |get_address|. This is the equivalent of how |getter_AddRefs|, et al, work. ... and for just the reason you're encountering.
Assignee | ||
Comment 12•24 years ago
|
||
Comment 13•24 years ago
|
||
exactly ... though you might want to remove the rogue |&| in the body of the |const| version of |address_of|. You should also add yourself to the contributors list at the top of the file :-)
Assignee | ||
Comment 14•24 years ago
|
||
Assignee | ||
Comment 15•24 years ago
|
||
Comment 16•24 years ago
|
||
To eliminate |const| warnings (which you might not have noticed because of selective template instantiation), you may want to either provide _two_ forms of |get_address| for each of |nsCOMPtr<T>| and |nsCOMPtr<nsISupports>| (my recommendation), or else use a |NS_CONST_CAST| in the |const| version of |address_of| --- you'll need to cast away |const|-ness to call your non-|const| version of |get_address|. To match my formatting, you should put the return value of |get_address| on the a separate line. While we're in there, we should decide if we want anyone to ever use |&| (i.e., |friends|). If not, then there's no reason not to make the definition of |&| be doubly poisonous, like so void operator&() const { } since it doesn't return a value, even if a |friend| tries to use it, it will be a syntax error. If we want to use it ourselves, then we should provide both forms (|const| and non-|const|). I recommend the poisonous form, which is normally how I turn off |operator=| et al. Sooner or later, someone is going to add new code that says |&some_nsCOMPtr| and be surprised when it doesn't compile. We'll want to put a big comment by |operator&| saying why we made |private|, and to prefer (first) passing by reference, or else (second) using |address_of|. I've updated the |nsCOMPtr| User's Guide to add a section talking about using |nsCOMPtr|s in APIs <http://www.mozilla.org/projects/nsCOMPtr.html#guide_nsCOMPtrs_in_APIs> and it mentions |address_of| and this bug, as well; so let's make this come true soon :-) We should probably put the above link into the comment at |operator&| to point confused readers at the rules for passing |nsCOMPtr|s around. If you have any concerns at all about this being ready to checkin ... here is the order to do it in that will make life the best: (1) check in all the changes to |nsCOMPtr| _except_ making |operator&| |private| ... now |address_of| is available to anyone who knows what an |nsCOMPtr| is (2) check in, at your own convenience, all your changes switching clients from |&| to |address_of| (3) when no ordinary callers of |&| remain ... make |operator&| |private|
Assignee | ||
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
sr=scc. Now we just have to figure out who else is enough of a C++ weenie to review this stuff. If anybody else had done this work, it would have been me and dbaron reviewing it :-)
Comment 19•24 years ago
|
||
To clarify, my [s]r=scc is for "11/14/00 13:48 (1)[ver 4]..." changes attached here <http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19230>
Comment 20•24 years ago
|
||
This code makes sense to me, r=jag. One of these days I'll be that C++ weenie ;-)
Comment 21•24 years ago
|
||
Comment 22•24 years ago
|
||
(1)[ver 4] (though I included my changed comments) builds and runs on Mac, David. You've tested linux, I imagine. That's two platforms ... probably enough, but if you can get someone to build and run on Windows then it's a no-brainer.
Comment 23•24 years ago
|
||
r=jag on (3)[ver.2] and (2)[ver 2]
Assignee | ||
Comment 24•24 years ago
|
||
Two issues jag and I discussed on IRC while he was reviewing: * I changed nsExpatTokenizer.cpp not to use nsCOMPtr because nsCOMPtr doesn't have a good way of dealing with in/out parameters. The current code relied on the fact that (nsIURI **)&uri gave &uri->mRawPtr, which seems ugly. * I'm not sure whether it's better style to initialize or not to initialize an |nsresult rv| before calling do_GetService(..., &rv), since do_GetService should always (I think) set rv. I did it some places and not others, but I should probably pick one.
Comment 25•24 years ago
|
||
If you want to do in/out parameters, pass the |nsCOMPtr| by reference. Raw pointers make bad in/out parameters, because the ownership model is not specified. Technically, the client code can't know whether to |Release| the old value before assigning in the new. Using |nsCOMPtr| here makes it clear to both caller and callee, and no funny business is required. void use_foo( nsCOMPtr<nsIFoo>& aFoo ) { // ... aFoo = some_new_foo; // old value |Release|d, new value |AddRef|ed // ... }
Assignee | ||
Comment 26•24 years ago
|
||
Assignee | ||
Comment 27•24 years ago
|
||
waterson says attachment 20043 [details] [diff] [review] works on gcc 2.7.3.
Comment 28•24 years ago
|
||
for the 12/01/00 13:17 (1)[ver 6] patch, r=scc
Assignee | ||
Comment 29•24 years ago
|
||
Comment 30•24 years ago
|
||
r=jag on [2](rev 3) and (1)[ver 6].
Assignee | ||
Comment 31•24 years ago
|
||
Comment 32•24 years ago
|
||
r=jag on (2)[ver 4]
Comment 33•24 years ago
|
||
r=waterosn on (1)[ver 6] and (2)[ver 4].
Comment 34•24 years ago
|
||
Ok, treat above r= as sr=, and also sr=waterson on (3)[ver. 2].
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
Assignee | ||
Comment 35•24 years ago
|
||
Comment 36•24 years ago
|
||
seems like that _should_ fix it; you don't need an r= to fix bustage ... but you've got one anyway. This is temporary no matter what, since the final change will compile everywhere with only one instance.
Assignee | ||
Comment 37•24 years ago
|
||
Assignee | ||
Comment 38•24 years ago
|
||
Comment 39•24 years ago
|
||
Sorry David, completely forgot to jot my r= in here. r=jag on "(5) a few address_of changes I missed the first time around".
Comment 40•24 years ago
|
||
Apologies -- I forgot to put my sr=brendan@mozilla.org on "(5) a few address_of changes" last week. /be
Comment 41•24 years ago
|
||
Yeah, I guess patch (6) <http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20450> is ready. sr=scc
Assignee | ||
Updated•24 years ago
|
Whiteboard: [xptest]
Assignee | ||
Comment 42•24 years ago
|
||
Comment 43•24 years ago
|
||
r=jag on (7)
Comment 44•24 years ago
|
||
sr=waterson on `01/08/01 21:32 (7) some new address_of changes'
Assignee | ||
Comment 45•24 years ago
|
||
Patch #6 built and ran for sfraser on Mac with no changes.
Assignee | ||
Comment 46•24 years ago
|
||
Patch #6 built for bryner on Windows with the one change I will attach below.
Assignee | ||
Comment 47•24 years ago
|
||
Comment 48•24 years ago
|
||
r=jag on #8 and #6.
Assignee | ||
Comment 49•24 years ago
|
||
Assignee | ||
Comment 50•24 years ago
|
||
Comment 51•24 years ago
|
||
r=jag on #9 and #10
Assignee | ||
Comment 52•24 years ago
|
||
Fix checked in 2001-02-04, although I think there may still be some incorrect usage in the commercial tree that was patch with address_of().
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9 → mozilla0.8
You need to log in
before you can comment on or make changes to this bug.
Description
•