misuse of nsCOMPtr::operator&

RESOLVED FIXED in mozilla0.8

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla0.8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xptest])

Attachments

(21 attachments)

1.56 KB, patch
Details | Diff | Splinter Review
13.80 KB, patch
Details | Diff | Splinter Review
13.80 KB, patch
Details | Diff | Splinter Review
103.98 KB, patch
Details | Diff | Splinter Review
103.36 KB, patch
Details | Diff | Splinter Review
2.35 KB, patch
Details | Diff | Splinter Review
1.90 KB, patch
Details | Diff | Splinter Review
16.30 KB, patch
Details | Diff | Splinter Review
2.03 KB, patch
Details | Diff | Splinter Review
3.18 KB, patch
Details | Diff | Splinter Review
3.06 KB, patch
Details | Diff | Splinter Review
3.91 KB, patch
Details | Diff | Splinter Review
16.37 KB, patch
Details | Diff | Splinter Review
16.91 KB, patch
Details | Diff | Splinter Review
3.26 KB, patch
Details | Diff | Splinter Review
2.19 KB, patch
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
926 bytes, patch
Details | Diff | Splinter Review
743 bytes, patch
Details | Diff | Splinter Review
3.68 KB, patch
Details | Diff | Splinter Review
881 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

17 years ago
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

17 years ago
Created attachment 18889 [details] [diff] [review]
(1) Changes to nsCOMPtr.h
(Assignee)

Comment 2

17 years ago
Created attachment 18890 [details] [diff] [review]
(1) Patch to fix incorrect usage
(Assignee)

Comment 3

17 years ago
Created attachment 18891 [details] [diff] [review]
(2) Patch to fix incorrect usage
(Assignee)

Comment 4

17 years ago
Created attachment 18892 [details] [diff] [review]
(3) Patch to convert justifiable (?) usage to get_address()

Comment 5

17 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 6

17 years ago
*** Bug 59069 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

17 years ago
Created attachment 19095 [details] [diff] [review]
(3)[ver. 2] patch to convert justifiable (?) usage to address_of
(Assignee)

Comment 8

17 years ago
Created attachment 19096 [details] [diff] [review]
(1)[ver. 2pre1] Changes to nsCOMPtr.h (that don't compile)
(Assignee)

Comment 9

17 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

17 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

17 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

17 years ago
Created attachment 19098 [details] [diff] [review]
(1)[ver 2] Changes to nsCOMPtr.h with address_of

Comment 13

17 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

17 years ago
Created attachment 19126 [details] [diff] [review]
(2)[ver 2] patch to fix incorrect usage (xlib changes added)
(Assignee)

Comment 15

17 years ago
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

17 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

17 years ago
Created attachment 19230 [details] [diff] [review]
(1)[ver 4] changes to nsCOMPtr.h incorporating more feedback and for first phase of checkin

Comment 18

17 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

17 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

17 years ago
This code makes sense to me, r=jag.

One of these days I'll be that C++ weenie ;-)

Comment 21

17 years ago
Created attachment 19539 [details] [diff] [review]
dbaron's latest patch + small change to make canonical contributor section

Comment 22

17 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

17 years ago
r=jag on (3)[ver.2] and (2)[ver 2]
(Assignee)

Comment 24

17 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

17 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

17 years ago
Created attachment 20043 [details] [diff] [review]
(1)[ver 6]patch that will really work for first-stage, with a non-const operator& too
(Assignee)

Comment 27

17 years ago
waterson says attachment 20043 [details] [diff] [review] works on gcc 2.7.3.

Comment 28

17 years ago
for the 12/01/00 13:17 (1)[ver 6] patch, r=scc
(Assignee)

Comment 29

17 years ago
Created attachment 20160 [details] [diff] [review]
[2](rev 3) remove unneeded initializations and a cast

Comment 30

17 years ago
r=jag on [2](rev 3) and (1)[ver 6].
(Assignee)

Comment 31

17 years ago
Created attachment 20196 [details] [diff] [review]
(2)[ver 4]Use nsCOMPtr<nsIURI>* in nsExpatTokenizer

Comment 32

17 years ago
r=jag on (2)[ver 4]

Comment 33

17 years ago
r=waterosn on (1)[ver 6] and (2)[ver 4].

Comment 34

17 years ago
Ok, treat above r= as sr=, and also sr=waterson on (3)[ver. 2].
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 35

17 years ago
Created attachment 20395 [details] [diff] [review]
(4) Possible (!) patch to fix IRIX bustage caused by (1)

Comment 36

17 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

17 years ago
Created attachment 20447 [details] [diff] [review]
(5) a few address_of changes I missed the first time around
(Assignee)

Comment 38

17 years ago
Created attachment 20450 [details] [diff] [review]
(6) patch to uncomment making operator& |private| and fix a few comments

Comment 39

17 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".
Apologies -- I forgot to put my sr=brendan@mozilla.org on "(5) a few address_of
changes" last week.

/be

Comment 41

17 years ago
Yeah, I guess patch (6)
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20450> is ready.  sr=scc
(Assignee)

Updated

17 years ago
Whiteboard: [xptest]
(Assignee)

Comment 42

17 years ago
Created attachment 22132 [details] [diff] [review]
(7) some new address_of changes

Comment 43

17 years ago
r=jag on (7)

Comment 44

17 years ago
sr=waterson on `01/08/01 21:32 (7) some new address_of changes'
(Assignee)

Comment 45

17 years ago
Patch #6 built and ran for sfraser on Mac with no changes.
(Assignee)

Comment 46

17 years ago
Patch #6 built for bryner on Windows with the one change I will attach below.
(Assignee)

Comment 47

17 years ago
Created attachment 23472 [details] [diff] [review]
(8) patch needed to build patch #6 on Windows

Comment 48

17 years ago
r=jag on #8 and #6.
(Assignee)

Comment 49

17 years ago
Created attachment 23791 [details] [diff] [review]
(9) more address_of changes
(Assignee)

Comment 50

17 years ago
Created attachment 24067 [details] [diff] [review]
(10) another getter_AddRefs change

Comment 51

17 years ago
r=jag on #9 and #10
(Assignee)

Comment 52

16 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
Last Resolved: 16 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.