Last Comment Bug 59414 - misuse of nsCOMPtr::operator&
: misuse of nsCOMPtr::operator&
Status: RESOLVED FIXED
[xptest]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Linux
: P3 normal (vote)
: mozilla0.8
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
: Edward Kandrot
:
Mentors:
: 59069 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-11-07 13:39 PST by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2001-02-07 21:12 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(1) Changes to nsCOMPtr.h (1.56 KB, patch)
2000-11-07 13:48 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(1) Patch to fix incorrect usage (13.80 KB, patch)
2000-11-07 13:48 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(2) Patch to fix incorrect usage (13.80 KB, patch)
2000-11-07 13:49 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(3) Patch to convert justifiable (?) usage to get_address() (103.98 KB, patch)
2000-11-07 13:49 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(3)[ver. 2] patch to convert justifiable (?) usage to address_of (103.36 KB, patch)
2000-11-10 16:49 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(1)[ver. 2pre1] Changes to nsCOMPtr.h (that don't compile) (2.35 KB, patch)
2000-11-10 16:50 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(1)[ver 2] Changes to nsCOMPtr.h with address_of (1.90 KB, patch)
2000-11-10 19:36 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(2)[ver 2] patch to fix incorrect usage (xlib changes added) (16.30 KB, patch)
2000-11-12 07:39 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(1)[ver 3] Changes to nsCOMPtr.h, incorporating scc feedback and using address_of for NSCAP_LOG_ASSIGNMENT in ~nsGetterAddrefs (2.03 KB, patch)
2000-11-12 07:41 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(1)[ver 4] changes to nsCOMPtr.h incorporating more feedback and for first phase of checkin (3.18 KB, patch)
2000-11-14 13:48 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
dbaron's latest patch + small change to make canonical contributor section (3.06 KB, patch)
2000-11-20 17:23 PST, Scott Collins
no flags Details | Diff | Splinter Review
(1)[ver 6]patch that will really work for first-stage, with a non-const operator& too (3.91 KB, patch)
2000-12-01 13:17 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
[2](rev 3) remove unneeded initializations and a cast (16.37 KB, patch)
2000-12-04 11:51 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(2)[ver 4]Use nsCOMPtr<nsIURI>* in nsExpatTokenizer (16.91 KB, patch)
2000-12-05 13:28 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(4) Possible (!) patch to fix IRIX bustage caused by (1) (3.26 KB, patch)
2000-12-08 22:57 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(5) a few address_of changes I missed the first time around (2.19 KB, patch)
2000-12-10 14:05 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(6) patch to uncomment making operator& |private| and fix a few comments (2.49 KB, patch)
2000-12-10 14:06 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(7) some new address_of changes (926 bytes, patch)
2001-01-08 21:32 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(8) patch needed to build patch #6 on Windows (743 bytes, patch)
2001-01-25 13:57 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(9) more address_of changes (3.68 KB, patch)
2001-01-29 22:54 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
(10) another getter_AddRefs change (881 bytes, patch)
2001-01-31 22:55 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-07 13:39:13 PST
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).
Comment 1 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-07 13:48:25 PST
Created attachment 18889 [details] [diff] [review]
(1) Changes to nsCOMPtr.h
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-07 13:48:56 PST
Created attachment 18890 [details] [diff] [review]
(1) Patch to fix incorrect usage
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-07 13:49:01 PST
Created attachment 18891 [details] [diff] [review]
(2) Patch to fix incorrect usage
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-07 13:49:43 PST
Created attachment 18892 [details] [diff] [review]
(3) Patch to convert justifiable (?) usage to get_address()
Comment 5 Scott Collins 2000-11-07 14:27:05 PST
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.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-10 15:53:21 PST
*** Bug 59069 has been marked as a duplicate of this bug. ***
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-10 16:49:46 PST
Created attachment 19095 [details] [diff] [review]
(3)[ver. 2] patch to convert justifiable (?) usage to address_of
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-10 16:50:38 PST
Created attachment 19096 [details] [diff] [review]
(1)[ver. 2pre1] Changes to nsCOMPtr.h (that don't compile)
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-10 16:53:27 PST
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.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-10 16:58:26 PST
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 Scott Collins 2000-11-10 17:23:41 PST
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.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-10 19:36:55 PST
Created attachment 19098 [details] [diff] [review]
(1)[ver 2] Changes to nsCOMPtr.h with address_of
Comment 13 Scott Collins 2000-11-10 21:27:04 PST
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 :-)
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-12 07:39:19 PST
Created attachment 19126 [details] [diff] [review]
(2)[ver 2] patch to fix incorrect usage (xlib changes added)
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-12 07:41:04 PST
Created attachment 19127 [details] [diff] [review]
(1)[ver 3] Changes to nsCOMPtr.h, incorporating scc feedback and using address_of for NSCAP_LOG_ASSIGNMENT in ~nsGetterAddrefs
Comment 16 Scott Collins 2000-11-12 13:49:39 PST
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|

Comment 17 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-14 13:48:29 PST
Created attachment 19230 [details] [diff] [review]
(1)[ver 4] changes to nsCOMPtr.h incorporating more feedback and for first phase of checkin
Comment 18 Scott Collins 2000-11-19 16:38:39 PST
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 Scott Collins 2000-11-19 16:41:18 PST
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 Peter ``jag'' Annema 2000-11-19 17:06:50 PST
This code makes sense to me, r=jag.

One of these days I'll be that C++ weenie ;-)
Comment 21 Scott Collins 2000-11-20 17:23:35 PST
Created attachment 19539 [details] [diff] [review]
dbaron's latest patch + small change to make canonical contributor section
Comment 22 Scott Collins 2000-11-20 19:45:38 PST
(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 Peter ``jag'' Annema 2000-11-28 13:19:28 PST
r=jag on (3)[ver.2] and (2)[ver 2]
Comment 24 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-11-28 13:39:15 PST
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 Scott Collins 2000-11-29 17:50:43 PST
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
      // ...
    }
Comment 26 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-01 13:17:25 PST
Created attachment 20043 [details] [diff] [review]
(1)[ver 6]patch that will really work for first-stage, with a non-const operator& too
Comment 27 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-01 20:12:34 PST
waterson says attachment 20043 [details] [diff] [review] works on gcc 2.7.3.
Comment 28 Scott Collins 2000-12-04 11:14:42 PST
for the 12/01/00 13:17 (1)[ver 6] patch, r=scc
Comment 29 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-04 11:51:28 PST
Created attachment 20160 [details] [diff] [review]
[2](rev 3) remove unneeded initializations and a cast
Comment 30 Peter ``jag'' Annema 2000-12-04 13:13:09 PST
r=jag on [2](rev 3) and (1)[ver 6].
Comment 31 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-05 13:28:40 PST
Created attachment 20196 [details] [diff] [review]
(2)[ver 4]Use nsCOMPtr<nsIURI>* in nsExpatTokenizer
Comment 32 Peter ``jag'' Annema 2000-12-05 13:38:25 PST
r=jag on (2)[ver 4]
Comment 33 Chris Waterson 2000-12-05 16:16:44 PST
r=waterosn on (1)[ver 6] and (2)[ver 4].
Comment 34 Chris Waterson 2000-12-05 19:56:40 PST
Ok, treat above r= as sr=, and also sr=waterson on (3)[ver. 2].
Comment 35 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-08 22:57:15 PST
Created attachment 20395 [details] [diff] [review]
(4) Possible (!) patch to fix IRIX bustage caused by (1)
Comment 36 Scott Collins 2000-12-09 16:53:13 PST
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.
Comment 37 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-10 14:05:04 PST
Created attachment 20447 [details] [diff] [review]
(5) a few address_of changes I missed the first time around
Comment 38 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2000-12-10 14:06:50 PST
Created attachment 20450 [details] [diff] [review]
(6) patch to uncomment making operator& |private| and fix a few comments
Comment 39 Peter ``jag'' Annema 2000-12-10 21:22:22 PST
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 Brendan Eich [:brendan] 2000-12-16 12:06:04 PST
Apologies -- I forgot to put my sr=brendan@mozilla.org on "(5) a few address_of
changes" last week.

/be
Comment 41 Scott Collins 2000-12-29 07:52:14 PST
Yeah, I guess patch (6)
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20450> is ready.  sr=scc
Comment 42 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-01-08 21:32:55 PST
Created attachment 22132 [details] [diff] [review]
(7) some new address_of changes
Comment 43 Peter ``jag'' Annema 2001-01-08 21:41:09 PST
r=jag on (7)
Comment 44 Chris Waterson 2001-01-09 19:59:07 PST
sr=waterson on `01/08/01 21:32 (7) some new address_of changes'
Comment 45 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-01-24 20:45:56 PST
Patch #6 built and ran for sfraser on Mac with no changes.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-01-25 13:56:49 PST
Patch #6 built for bryner on Windows with the one change I will attach below.
Comment 47 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-01-25 13:57:58 PST
Created attachment 23472 [details] [diff] [review]
(8) patch needed to build patch #6 on Windows
Comment 48 Peter ``jag'' Annema 2001-01-27 18:46:14 PST
r=jag on #8 and #6.
Comment 49 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-01-29 22:54:37 PST
Created attachment 23791 [details] [diff] [review]
(9) more address_of changes
Comment 50 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-01-31 22:55:58 PST
Created attachment 24067 [details] [diff] [review]
(10) another getter_AddRefs change
Comment 51 Peter ``jag'' Annema 2001-01-31 23:02:41 PST
r=jag on #9 and #10
Comment 52 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-02-07 21:12:28 PST
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().

Note You need to log in before you can comment on or make changes to this bug.