75504 - |dont_AddRef| surprise

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[John Bandhauer]

Please read and vend some clues on the issues in the mail thread "dont_AddRef
wierdness" in your inbox.

The refcounting weirdness is one issue we care about. The question of *why* the
form of the code without the refcounting problem does not build on Mac is
another issue.

Your help would be much appreciated.

Thanks,

John.

.........................................................................
Subject: Re: dont_AddRef wierdness?
Date: Tue, 10 Apr 2001 21:06:47 -0700
From: John Bandhauer <[e-mail]>
Organization: minions of mozilla
To: Scott Collins <[e-mail]>, Johnny Stenback <[e-mail]>,
    Peter Van der Beken <[e-mail]>
References: 1 , 2 , 3 , 4 , 5

So... Stepping through this in the debugger I see...

case 1...
nsCOMPtr<XPCWrappedNative> wrapper(dont_AddRef(
    XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull)));

This works right. It goes through...

const already_AddRefed<T>
dont_AddRef( T* aRawPtr )

...and... uses the nsCOMPtr ctor:

nsCOMPtr( const already_AddRefed<T>& aSmartPtr )

(where 'T' is XPCWrappedNative).

case 2...
nsCOMPtr<nsIXPConnectJSObjectHolder> wrapper(dont_AddRef(
    XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull)));

This does *not* work as expected. It goes through...

const already_AddRefed<T>
dont_AddRef( T* aRawPtr )

...but.. uses the nsCOMPtr ctor:

nsCOMPtr( T* aRawPtr )

So that an *extra* reference is silently added.

I assume it is doing this because the dont_AddRef it typed as 'XPCNativeWrapper', 
but the nsCOMPtr is typed to a class that is a base class of that class, but
not an exact match.

Is this something that will only bite in the case where we are dealing with
a return value? If it did this with getter_AddRefs and 'out' pointer then we 
would be in much pain.

Nevertheless, this is not pretty.

Especially bad since we are still without understanding why case 1 will not
even build on Mac (when the nsCOMPtr<XPCWrappedNative> is actually used).

Any insight anyone?

John.  


John Bandhauer wrote:
> 
> Hey! Didn't *anyone* see my message below???
> 
> scc: no comment? This is a completely heinous bug. Is this a
> known problem? Fixed on the trunk? Any idea?
> 
> peterv: I just spent five minutes trying to figure out how my
> recent changes could have caused a ton of leaks to spring up and
> then I realized that you must have checked in the change despite
> my message here that this causes a refcount misbalance.
> I'm going to change this back to a nsCOMPtr<XPCWrappedNative> in
> the tree for now. You can change it locally on your machine and
> let it leak there until we puzzle this out. OK?
> 
> scc: Besides the refcount craziness, can you vend a clue on why
> peterv would see Mac build failure with
> nsCOMPtr<XPCWrappedNative> (given the inheritance below)?
> 
> Thanks,
> 
> John.
> 
> John Bandhauer wrote:
> >
> > OK now I'm worried.
> >
> > It looks to me like nsCOMPtr and/or dont_AddRef is not doing the
> > right thing on XPCDOM_20010329_BRANCH.
> >
> > In the patch below I end up with an extra (leaked) reference.
> > after changing the type of the nsCOMPtr from the concrete type
> > to an abstract base type.
> >
> > I have an inheritance chain like:
> >
> > // in idl
> > interface nsIXPConnectJSObjectHolder : nsISupports
> > interface nsIXPConnectWrappedNative : nsIXPConnectJSObjectHolder
> >
> > // in C++
> > class XPCWrappedNative : public nsIXPConnectWrappedNative
> >
> > The static method XPCWrappedNative::GetNewOrUsed returns a
> > pointer to a XPCWrappedNative that has been addref'd.
> >
> > This code...
> >
> >   nsCOMPtr<nsIXPConnectJSObjectHolder> wrapper(dont_AddRef(
> >      XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull)));
> >
> > ...or this code...
> >
> >   nsCOMPtr<nsIXPConnectWrappedNative> wrapper(dont_AddRef(
> >      XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull)));
> >
> > ...will addref wrapper one more time than...
> >
> >   nsCOMPtr<XPCWrappedNative> wrapper(dont_AddRef(
> >      XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull)));
> >
> > The only thing different is the type of the nsCOMPtr
> >
> > This is in a debug build.
> >
> > The object's QI map looks like:
> >
> > NS_INTERFACE_MAP_BEGIN(XPCWrappedNative)
> >   NS_INTERFACE_MAP_ENTRY(nsIXPConnectWrappedNative)
> >   NS_INTERFACE_MAP_ENTRY(nsIXPConnectJSObjectHolder)
> >   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIXPConnectWrappedNative)
> > NS_INTERFACE_MAP_END_THREADSAFE
> >
> > This is really bad. Is this a known problem or what?
> >
> > Thanks,
> >
> > John.
> >
> > >   -----------------------------------------------------------------
> > > Index: xpccomponents.cpp
> > > ===================================================================
> > > RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v
> > > retrieving revision 1.37.2.1
> > > diff -u -r1.37.2.1 xpccomponents.cpp
> > > --- xpccomponents.cpp   2001/03/31 07:17:44     1.37.2.1
> > > +++ xpccomponents.cpp   2001/04/06 05:23:45
> > > @@ -1680,7 +1680,7 @@
> > >      if(!iface)
> > >          return JS_FALSE;
> > >
> > > -    nsCOMPtr<XPCWrappedNative> wrapper(dont_AddRef(
> > > +    nsCOMPtr<nsIXPConnectJSObjectHolder> wrapper(dont_AddRef(
> > >          XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface,
nsnull)));
> > >      if(!wrapper)
> > >          return JS_FALSE;
> > > @@ -1688,9 +1688,10 @@
> > >      aScope->SetComponents(components);
> > >
> > >      jsid id = ccx.GetRuntime()->GetStringID(XPCJSRuntime::IDX_COMPONENTS);
> > > -    JSObject* obj = wrapper->GetFlatJSObject();
> > > +    JSObject* obj;
> > >
> > > -    return obj && OBJ_DEFINE_PROPERTY(ccx,
> > > +    return NS_SUCCEEDED(wrapper->GetJSObject(&obj)) &&
> > > +           obj && OBJ_DEFINE_PROPERTY(ccx,
> > >                                        aGlobal, id, OBJECT_TO_JSVAL(obj),
> > >                                        nsnull, nsnull,
> > >                                        JSPROP_PERMANENT | JSPROP_READONLY,

Comment 1

[Scott Collins]

First, regarding the email:

You didn't send this message to [e-mail], so I never saw it.  I didn't
see it because (a) ISDN for Michigan people including myself, Kathy Brade, and
Mark Smith has been out for over a week, and (b) even when I do have access
through firewall, I use it only rarely since my cable modem is 12 times faster
than my ISDN ever was.

I even mention this at the top of my journal page.

  http://ScottCollins.net/journal/

It has it's own section, headline and all:

  why can't I reach Scott by email?

  Are you sending email to [e-mail]? If you're sending mail to my
  netscape.com address, be aware that Netscape's firewall stands between me
  and email sent to that address.

Netscape won't allow me to forward my netscape.com address to my mozilla.org
address.  Thank god you filed a bug, as you should have, since my bugzilla
address is [e-mail], the email address you should prefer for me in the
future.


Second, |dont_AddRef| wierdness:

You're saying that |GetNewOrUsed| returns a pointer to a concrete class, _not_
an interface, right?  Then it's not surprising that it doesn't match

  template <class T>
  nsCOMPtr<T>::nsCOMPtr( const dont_AddRef<T>& );

since |T| is already bound to the type |nsI...|, you haven't given it a
|dont_AddRef<T>| parameter.  But you have given it something that can be turned
into a T*.  Normally one wouldn't hit this, because they wouldn't try to use a
function that returned a pointer to a concrete class.  The traditional fix
(whereby I reported this as an error at compile time) to this problem requires
that compilers support template member functions, and that's beyond the pale of
our compiler suite.  What happens when you say

  nsCOMPtr<nsIXPConnectJSObjectHolder> wrapper(dont_AddRef(
    NS_STATIC_CAST(nsIXPConnectJSObjectHolder*,
      XPCWrappedNative::GetNewOrUsed(ccx, cholder, aScope, iface, nsnull))));

I would expect this to do the right thing, and more importantly _say_ the right
thing with respect to C++'s type system.

Comment 2

[Scott Collins]

Note: maybe this is happening in more places in the tree than we realize, and
you were the only one careful enough to catch it.  With that in mind...

Here's one other possible ``fix'' for this situation.  I could remove
|already_AddRefed<T>::operator T*() const|, forcing callers who just want the
pointer to say |.get()|.  The goal has been to encourage people writing
functions that return an already |AddRef|ed pointer to use this template type
instead of the raw pointer type, because it is (or at least without this change)
transparent to non-|nsCOMPtr| callers, and it lets |nsCOMPtr| callers always do
the right thing.  In fact, your specific problem would probably be fixed by
changing your declaration to

  already_AddRefed<nsIXPConnectJSObjectHolder>
  XPCWrappedNative::GetNewOrUsed( /* blah, blah, blah, blah */ );

...even without changing one single line inside the implementation (since the
conversion would happen correctly and automatically in your |return| statement).
 Callers who just held the answer in a pointer (currently) automatically get
one.  It's just as efficient as returning a pointer since the class is just type
magic and is exactly the same size and shape as the underlying pointer.  So,
this would be no change in any callers, and all assignments into |nsCOMPtr|s of
the appropriate (interface) type would start working.

On the other hand, if we removed the operator _and_ your function returned an
|already_AddRefed<T>|, then callers of functions like this who _weren't_ storing
the |AddRef|ed pointer in an |nsCOMPtr| would have to say something like this

  myRawPtr = XPCWrappedNative::GetNewOrUsed(...).get();

But |nsCOMPtr| clients have an easier time

  myCOMPtr = XPCWrappedNative::GetNewOrUsed(...);

...no cast, and no |dont_AddRef|.

I'll attach a patch for "nsCOMPtr.h" (to eliminate the implicit conversion
operator so the compiler reports problems like yours as an error) in a bit.  I
think it still needs some debate.  It's up to you, of course, to decide if you'd
like to produce the |already_AddRefed<T>| result directly (instead of creating
it with |dont_AddRef|).
  

Comment 3

[John Bandhauer]

> You didn't send this message to [e-mail], so I never saw it.

Sorry Scott, I've become used to discovering that the ultimate cause of so many 
of the problems I try to solve turns out to simply be that I'm an idiot. Here we 
have another case in point. I've fixed my addressbook. I'll write it on the 
blackboard a thousand times and maybe I'll remember.

> Note: maybe this is happening in more places in the tree than we realize

Yes, my real concern is that this might be happening elsewhere. The case we're 
discussing is deep inside xpconnect - which uses this pattern for no public 
interfaces. Making the problem go away inside xpconnect is not a problem at all. 
Understanding and avoiding it elsewhere was my real goal here.

I find declaring the method to return 
already_AddRefed<nsIXPConnectJSObjectHolder> unappealing.

For what its worth I think I'm just going to fix *all* the internal methods in 
xpconnect that return an addref'd pointer to do so with 'out' params (like the 
rest of the world). This will make xpconnect more maintainable anyway - since it 
also has many internal methods that vend xpcom pointers that are not addref'd. 
The distinction will be made much clearer.

I'm not sure which of your suggestions is better across the project. I suspect 
that removing |already_AddRefed<T>::operator T*() const| might burn some people 
who are successfully working around this issue. But I don't know.

Thanks.

Comment 4

[Scott Collins]

Attachment: patch to eliminate implicit conversion from |already_AddRefed|; I'm testing it now

Comment 5

[Scott Collins]

Patch attached above.  I'm building now to see if it catches anyone less
reference-counting observant than was jband.  May need to updated the patch if
it does.

Comment 6

[Scott Collins]

It is catching some bad uses of |getter_AddRefs|.  I'll attach a patch to fix
these when I have one that covers the whole tree.  So far, I've only fixed
"nsNetUtil.h", and "nsGlobalWindow.cpp".

Comment 7

[Scott Collins]

Attachment: this patch also fixes all the bad callers (several of whom were leaking)

Comment 8

[Håkan Waara]

The patch includes fixes for leaks in addressbook code, cc chuang.

Comment 9

[John Bandhauer]

r/sr=jband Go scc!

Comment 10

[Brendan Eich [:brendan]]

+++ nsStreamTransfer.cpp        2001/04/20 21:49:09
@@ -245,5 +245,5 @@
                 rv = bundleService->CreateBundle(
"chrome://global/locale/downloadProgress.properties",
                                                   locale, 
-                                                  getter_AddRefs( &bundle ) );
+                                                  &bundle );
                 if ( NS_SUCCEEDED( rv ) ) {
                     rv = bundle->GetStringFromName( NS_ConvertASCIItoUCS2(
"FilePickerTitle" ).GetUnicode(),
@...

Shouldn't you make bundle be nsCOMPtr<nsIStringBundle>, or else add an
NS_RELEASE(bundle) in the succeeded case?  The error here was the lack of
nsCOMPtr use in spite of the getter_AddRefs( &bundle ), compounded by the rogue
& in front of getter_AddRefs' argument -- or so it seems to me.

The rest look good.  Good catch jband!

/be

Comment 11

[Asa Dotzler [:asa]]

Are you still shooting for 0.9 on this? If so please email [e-mail]
with a status on you progress. If not please retarget against a later Milestone.
Thanks.

Comment 12

[chris hofmann]

one of the first little thingies for 0.9.1

Comment 13

[Scott Collins]

fixes checked in, including brendans suggestion with respect to
"nsStreamTransfer.cpp".