Closed Bug 104346 Opened 23 years ago Closed 14 years ago

need |nsRefPtr|

Categories

(Core :: XPCOM, defect, P1)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Whiteboard: [patch])

Attachments

(9 files, 6 obsolete files)

fancy version (don't try this at home), to be abandoned
5.91 KB, text/plain
Details
Tests that the previous code passes, with gcc3
2.02 KB, text/plain
Details
work in progress (works fine on gcc 3.0.1!)
27.12 KB, patch
Details | Diff | Splinter Review
patch to tests to go along with attachment 111746
15.27 KB, patch
Details | Diff | Splinter Review
a version of nsAutoPtr.h based on nsCOMPtr
35.17 KB, text/plain
Details
patch against attachment 111884 to fix msvc bustage
522 bytes, patch
Details | Diff | Splinter Review
remove operator= and add reset() v3
12.94 KB, patch
dbaron
: review-
Details | Diff | Splinter Review
patch to nsDerivedSafe
563 bytes, patch
jag+mozilla
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
attempt to fix otaku bustage
4.68 KB, patch
bbaetz
: review+
bryner
: superreview+
Details | Diff | Splinter Review 
We need a refcounting smart pointer that doesn't require inheritance from
nsISupports so that people don't overuse COM just because they want a refcount
and they like nsCOMPtr.  (See bug 104336.)

I'll attach an experimental implementation I was playing with and a test that
actually works on gcc3.  And then I'll start over from scratch (and from
nsCOMPtr.h) later to write something that we could really consider checking in.
Target Milestone: --- → mozilla0.9.6
should that topic be about nsAutoPtr? ;-) Though there is no impl file, should
this go into glue? Or not depend on glue, doubling already_AddRefed, or 
getting it out of nsCOMPtr.h?
There's no need for it to be in glue since it's entirely inlined.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Status: NEW → ASSIGNED
Nice start, but looks to me like these classes are a little too lean.

The project on which I am currently working has all of these types of pointers, 
but our implementations also cover the following:

	T& operator*() const
		{
		assert(p!=NULL);
		return *p;
		}
	T * operator->() const
		{
		assert(p!=NULL);
		return p;
		}
	//The assert on operator& usually indicates a bug.  If this is really
	//what is needed, however, take the address of the p member explicitly.
	T** operator&()
		{
		assert(p==NULL);
		return &p;
		}
	bool operator!() const
		{
		return (p == NULL);
		}
	bool operator<(T* pT) const
		{
		return p < pT;
		}
	bool operator==(T* pT) const
		{
		return p == pT;
		}
	bool operator!=(T* pT) const
		{
		return p != pT;
		}
	T* Detach()
		{
		T* pt = p;
		p = NULL;
		return pt;
		}

The first 3 are mostly just debug help over what you have today. On #3 though, 
I notice that you force the use of 'begin_assign' instead, which, I guess, 
seems okay, but it looks to me like it leaks:

T** begin_assignment() { mPtr = 0; return &mPtr; }

My #3 above asserts that the pointer is NULL, and you just set it to NULL.  
What if mPtr is already holding a pointer to allocated memory?  You haven't 
freed it.

The boolean operators are useful, since they make converting existing pointer 
code the an auto-pointer easier, and less prone to introduction of new bugs or 
compiler errors that need to be fixed.

And finally, a Detach method seems crucial for times when you want to release 
the auto-pointer from responsibility for freeing the object, because you are 
transfering that responsibility to another class or function.
Many of those operators you mention should be handled by implicit conversion --
I've yet to test whether that actually works across compilers.

Instead of |Detach| I used |release|, as std::auto_ptr does.

That is a mistake in begin_assignment -- I was thinking |*this = 0|.  I hadn't
tested that yet, though.

I should also probably make operator& private as for nsCOMPtr.
Yup.  All but '>' work on my compiler.

I don't see 'release' in the posted patch, but perhaps I am just not good at 
reading these yet.  The term would get pretty overloaded on the nsRefPtr, I'd 
imagine, where release should imply a ref count decrement.

Given the ref counting issue, we use 'release' to 'free' (either by 
decrementing, deleting, or freeing), and 'detach' to remove the pointer 
without 'freeing'.

There's a newer version checked in to the tree.  There is no |release| on
nsRefPtr.  Perhaps it needs a |swap| like the patch for nsCOMPtr in bug 78016.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.7
-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
r=jag on what's currently in the tree.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.1
Target Milestone: mozilla1.1alpha → Future
I want to use nsRefPtr<> on a COM object (it implements nsIDocumentObserver) so
that I don't have to create an interface and IID for the concrete class.  I have
found that I can use an nsCOMPtr<> in the meantime (since I only implement one
interface) but I'll try switching on nsRefPtr<> first since I think that's
really what I'm trying to do.  It turns out I don't need operator-> but I'll
test that anyway.

Any chance we can make nsCOMPtr<T> extend from nsRefPtr<T>?  It seems like they
have mostly the same purpose (nsRefPtr<T> just has *extra* stuff) and we could
save some lines of code that way (and obscure bugs--copying implementations from
nsCOMPtr save us the headache of repeating history).  The specialized
nsCOMPtr<nsISupports> is the only thing I'm worried about there.  Some kind of
funkiness is going on extending from nsCOMPtr_base.  Anyhow, I'll give this a
shot.  At the least, I think nsRefPtr method implementations should look like
nsCOMPtr's do, whether that means changing nsRefPtr or changing nsCOMPtr :)

Finally, should we comment out everything else in nsAutoPtr.h (nsAutoPtr<T>,
nsAutoArrayPtr<T> and nsMemoryAutoPtr<T>) or is this a "include one, include
all" thing?  I think until we need them we should #if 0 out the other classes.
Messing with nsCOMPtr that much is scary and, in my opinion, unnecessary.
hmmm, why write our own if we can use the pointers from boost?
i might have a look at finishing this. One bug i found was that nsRefPtr needs
to "NS_IF_ADDREF" rather then "NS_ADDREF", i.e. it needs to nullcheck before
calling Addref/Release
all begin_assignment needs to exit() the current pointer before returning.
Attached patch fixes a few random things (obsolete) — — Splinter Review 
This fixes:
* comment 15 and comment 16
* make nsRefPtr return nsDeriviedSafe to protect against addreffing and
  releasing an nsRefPtr
* fixes a typo in nsMemoryAutoPtr::operator=
* removes the implementations of all operator& since they should not be called


There are a few things i'm pondering:
1. Scott raises a good question on if we should be doing our own implementation

   at all. There are two things i don't like with boost
   a) They keep the refcount outside the object. Their pointer therefor bigger
      then 4 bytes, and they have an extra heap-allocated object for each
      refcounted object to keep the count.
   b) AFAICT there is no way to get an equivalent to nsMemoryAutoPtr with
boost.
      How big of a loss this is I don't know.
   If we can live with these things, or if it would be worth using boost for
the
   other pointertypes, I don't know.
   
2. Should we really have |operator=| and |operator T*| on the autoptrs. On one
   hand we do on nsRefPtr and nsCOMPtr, but it seems easier to forget to call 
   .release() on an autoptr and crash. Maybe it's the |operator T*| that is
   dangerous, if the only way to get the pointer was through .get() and
   .release() then people might be less likly to accidently get the pointer
   without properly managing the ownership.
   If we decide not to have |operator=| we should probably not have
   |getter_Transfers| either.

3. |explicit| on the ctors seems like a nice thing to have. If we have an
   operator=(T*) then that should be enough to get |nsAutoPtr<T> ptr = foo;| to

   compile. This is of course related to the previous point.
I believe there was a very good reason std::auto_ptr didn't provide an opertor
=(T*); Unfortunately I don't remember the reason, just that there was one.

And there's issues to deal with protecting const pointers. And some of the
solutions may not be supported across the wide variety platforms we work on.

Also, if we're going to do something different than what std::auto_ptr does, you
might think about changing the name.

As far as the reference counted pointers go. A friend of mine created a template
implementation that allowed the user to decide how the reference counting system
was implemented. This allowed us to handle both intrusive and passive ref
counting mechanisms. It could be a little edgy on templates for our uses, but I
don't think it would be. In addition to the type of pointer, we had an
additional template parameter which was the class to implement the ref counting.
This essentially would allow all ref counting code to be based of the same base
template, whether it was XPCOM or regular pointer, or some other system. I'd
have to sit down and remember the full implementation. It's been like 4 years ago.




I need this for some deCOMtamination work I'm doing.  Any chance we can come to
a decision on this and get nsRefPtr turned on?
1. Comment #14 and Comment #17 couldn't we use the stuff that fits our needs? I
don't know about licensing issues and such, though.
2. Why is this in XPCOM? What does it have to do with XPCOM?
Skimming I noticed the assignment operators for most if not all the class are
not self assignment safe. They also don't take into account you could be
assigning the same pointer value but using two different pointer class
instances. In both these cases you'll end up with class holding pointers to dead
objects.

So I think there's some work to be done even if we use the interfaces provided.
(it still makes me nervous to allow conventions that other implementations have
disallowed to avoid accidental misuse, such as non-explicit constructors and
assignment operators taking pointer types)


Since bryner wants to be able to use something quickly, here's a version of
nsAutoPtr.h based on nsCOMPtr, which is something that we know works and meets
our portability requirements.
This is a patch to TestAutoPtr.  It compiles with the above patch.

However, a bunch of the tests fail, mostly related to casting to base classes
(perhaps only dealing with equality).  I still haven't investigated why.
This one passes the equality tests, at least with gcc 3.2.1.

I think I'd like to check in these changes (to nsAutoPtr and to the tests) so
we can test this on multiple platforms.  Does that seem reasonable?
Attachment #111746 - Attachment is obsolete: true
Also, what do you think about the name |forget()|?  People didn't like the use
of |release()| as in the standard auto_ptr since we use |Release()| for
something else.  |drop()| was also suggested above, I think, but I just happened
to think of |forget()| and like it.
Also, we should probably port the equality fixes to nsCOMPtr (although I
actually worry such comparisons may not compile cross-platform).
Visual C++ doesn't quite like this:

../../dist/include/xpcom\nsAutoPtr.h(1059) : error C2440: 'reinterpret_cast' : 
cannot convert from 'const class TestRefObject *' to 'class 
nsDerivedSafe<class TestRefObject const > *'
        ../../dist/include/xpcom\nsAutoPtr.h(1058) : while compiling class-
template member function 'class nsDerivedSafe<class TestRefObject const > 
*__thiscall
 nsRefPtr<class TestRefObject const >::get(void) const'
make[1]: *** [TestAutoPtr.obj] Error 2
Blocks: 114713
Just a note, it looks like nsCOMPtr may go the with swap instead of a 'forget'
type function. Thought I'd mention in case you want to keep the two in sync or
have other ideas.
Attached patch remove operator= and add reset() (obsolete) — — Splinter Review 
This patch removes the potentially harmful operator= and introduces a
reset-method instead. It allows

myAutoPtr.reset();
myAutoPtr.reset(myRawPtr);
myAutoPtr.reset(myOtherAutoPtr);

It also removes getter_Transfers() since it is pretty much as dangerous as
operator=. I'm a little unsure about removing it though feel free to speak up
if you want it to stick around. The new way of doing the same is

Foo* rawptr
obj->GetFoo(&rawptr);
nsAutoPtr<Foo> ptr(rawptr);

or

nsAutoPtr<Foo> ptr;
...
Foo* rawptr
obj->GetFoo(&rawptr);
ptr.reset(rawptr);

I also made all ctors explicit. (should the empty ctor be explicit?)
Attachment #110185 - Attachment is obsolete: true
Does |TestAutoPtr| still work with this patch?
This includes neccesary changes to make TestAutoPtr.cpp build on windows. It
also adds some more tests.

There are two things that i find strange:

1. The following compiles even though the ctor taking a raw-pointer is
explicit:

   nsAutoPtr<TestObject> TestObjectFactory()
   {
       return new TestObject;
   }

   Not that i see a great risk in this, i just found it a bit strange and
thought
   it'd be best to get tested on all platforms

2. I had to make the ctor taking an nsAutoPtr be *not* explicit. Otherwise I
   could not get functions like:
   void Sink(nsAutoPtr<TestObject> aObject);
   to compile, no matter how i tried to call it. I kept getting a compileerror
   saying "cannot convert parameter 1 from 'class nsAutoPtr<TestObject>' to
   'class nsAutoPtr<TestObject>'. No copy constructor supplied"
   nsAutoPtr<TestObject>"
Attachment #113346 - Attachment is obsolete: true
one thing i thought about if we really really want |getter_Transfers| is to
force a syntax like

nsAutoPtr<Foo> foo;
...
getFoo(getter_Transfers(foo.reset());

that way it should be apparent to anyone that whatever foo is pointing to is
deleted. I would prefer removing getter_Transfers entierly though.
I'm comfortable with removing getter_Transfers entirely.  In fact, now that you
mention it, I'm strongly in favor of removing it entirely.
exactly the same as the above but with an additional test that simulates
out-of-memory
Attachment #113368 - Attachment is obsolete: true
Attachment #113368 - Attachment is obsolete: false
Attachment #113368 - Flags: review?(dbaron)
Comment on attachment 113556 [details] [diff] [review]
remove operator= and add reset() v3

cool. Then i propose that we go with this interface.
Attachment #113556 - Flags: review?(dbaron)
I think adding explicit to the default constructor has no meaning. And why not
just remove the default constructor and use the T * constructor with a default
of null. explicit nsAutoPtr(T * aRawPtr = 0);
I'm not sure that nsAutoPtr(T * aRawPtr = 0) would work across all compilers.
nsCOMPtr has separate ctors
There's a potential problem on platforms where HAVE_CPP_ACCESS_CHANGING_USING
is not defined.  The fact that nsDerivedSafe declares AddRef and Release as
virtual functions can change the object layout, since they may be non-virtual
on the template class.	There's really no _need_ for them to be marked virtual
on nsDerivedSafe (a call to them won't compile anyway!) so this patch removes
the virtual.
Attachment #114729 - Flags: review?(dbaron)
Comment on attachment 114729 [details] [diff] [review]
patch to nsDerivedSafe

There *might* be an issue with this on Windows because of the calling
convention stuff hidden in |NS_IMETHOD_|.  (|virtual| should be implied when
the method is |virtual| on the base class.)  Did you test on Windows?
yeah, i compiled and ran on windows and things appeared to be fine.
Comment on attachment 114729 [details] [diff] [review]
patch to nsDerivedSafe

Scott, what do you think?
Attachment #114729 - Flags: superreview?(dbaron)
Attachment #114729 - Flags: review?(scc)
Attachment #114729 - Flags: review?(dbaron)
Attachment #114729 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 114729 [details] [diff] [review]
patch to nsDerivedSafe

r=jag.
Attachment #114729 - Flags: review?(scc) → review+
Comment on attachment 115277 [details] [diff] [review]
attempt to fix otaku bustage

r=bbaetz
Attachment #115277 - Flags: review+
Attachment #115277 - Flags: superreview+
dbaron, i'll leave it up to you whether this bug should be resolved as fixed now.
IMHO we should still check in the interface change in attachment 113556 [details] [diff] [review]. It
makes the interface "safer" and more similar to other smart-pointer
implementations out there.

Bug 185797 (huge rewrite of the XSLT part of transformiix) is very dependant on
nsAutoPtr. If we're going to go with the current interface i'd like to know so
that i can start using it as is. Right now i'm waiting for the patch to be
checked in so that i can start using it.
I don't think I'm the person that put those modifiers on there.  I think that
was a fix for some platform horkage.  My advice is to make sure you try building
on the compilers that use that hunk with your changes.
Comment on attachment 113556 [details] [diff] [review]
remove operator= and add reset() v3

Removing getter_Transfers is a good idea.  I'm not so sure about the operator=
changes -- is the idea to make it so that you can't use |=| syntax?  What about
compiler generated operator=?  Do the same things compile and not compile
across platforms?
Yes, the idea is that the |=| syntax since it might not be obvious that the old
object will be deleted (though I forgot to make a private non-implemented one).
It's code like this that I want to avoid:

nsAutoPtr<Foo> foo(new Foo);
...
Foo* old = foo;
foo = getFoo();
...
old->useFoo();


It is IMHO not apparant that this will crash due to the first Foo being deleted
when |foo| gets a new value. So the idea is to force

nsAutoPtr<Foo> foo(new Foo);
...
foo.reset(getFoo());

which should make it more apparent that the old value gets deleted. This is the
same reason that I wanted to get rid of getter_Transfers, that it isn't apparent
that the old value will unmercifully be deleted.

Looking at scoped_ptr from boost and auto_ptr from stl it seems like they
instead protect against this be not having an |operator T*()| but instead force
you to call .get(). Though scoped_ptr doesn't have an operator= either.


The same things should compile across all platforms. I added every possible
thing I could think of to TestAutoPtr.cpp so as long as that behaves properly it
should work.
So on OS X I get the following compilation error for
    nsAutoPtr<foo> bar = new foo();

no matching function for call to `nsAutoPtr<T>::nsAutoPtr(nsAutoPtr<T>)'
candidates are: nsAutoPtr<T>::nsAutoPtr(nsAutoPtr<T>&) [with T = foo]
                nsAutoPtr<T>::nsAutoPtr(T*) [with T = foo]
initializing temporary from result of `nsAutoPtr<T>::nsAutoPtr(T*) [with T = foo]'
i've started using nsAutoPtr and am more and more convinced that we should get
rid of the operator=. Even in cases where you do want the old value to be
deleted it is pretty unclear that it actually is. For example in functions like

void
txStylesheetCompilerState::popChooseGotoList()
{
    // this will delete the old value
    mChooseGotoList = popObject();
}

i've found that i always end up commenting that the old value is deleted. It
would IMHO look much better as

void
txStylesheetCompilerState::popChooseGotoList()
{
    mChooseGotoList.reset(popObject());
}

David: you mentioned that you were worried that we would have cross-platform
issues with my patch. Could you explain what you suspect would/wouldn't compile
where? I'd happily try out different things on neccesary platforms.
I'm worried that if you remove operator= then using operator= (but not in the
ctor, like peterv used it) will compile on some platforms and not others. 
Although the ctor case matters to.  We need to make things so that we're as
close as possible to having the same set of things compile across platforms.
I also find the reset syntax a bit clumsy, especially when dealing with member
variables.  Furthermore, I don't think people have had a problem with nsCOMPtr
and realizing that the object can't be accessed once the nsCOMPtr holds another
value.
another thing we should protect against is constructing an nsAutoPtr<A> from an
nsAutoPtr<B> (where B inherits A). For example in constructs like

class B : public A {
};

void sink(nsAutoPtr<A> aItem) {
}


  nsAutoPtr<B> b = new B;
  sink(b);
should either not compile, or it should behave like
  nsAutoPtr<A> a = new A;
  sink(a);
i.e. pass the value to the sink and then loose its value.
Attached patch patch to remove getter_Transfers (obsolete) — — Splinter Review 
This subsumes part of attachment 113556 [details] [diff] [review], and I'd like to get it in sooner
rather than later.
Comment on attachment 113556 [details] [diff] [review]
remove operator= and add reset() v3

Marking review- pending testing that this doesn't make the set of things that
compile inconsistent between platforms.  Also note that the test functions like

+nsAutoPtr<TestObject> TestObjectFactory()
+{
+    return new TestObject;
+}

won't compile on gcc3, since they give the error (at least without the
nsAutoPtr.h changes, and I expect with them as well):

/builds/trunk/mozilla/xpcom/tests/TestAutoPtr.cpp: In function
`nsAutoPtr<TestObject> TestObjectFactory()':
/builds/trunk/mozilla/xpcom/tests/TestAutoPtr.cpp:57: error: no matching
function for call to `nsAutoPtr<TestObject>::nsAutoPtr(nsAutoPtr<TestObject>)'
../../dist/include/xpcom/nsAutoPtr.h:93: error: candidates are:
nsAutoPtr<T>::nsAutoPtr(nsAutoPtr<T>&) [with T = TestObject]
../../dist/include/xpcom/nsAutoPtr.h:87: error: 	       
nsAutoPtr<T>::nsAutoPtr(T*) [with T = TestObject]
/builds/trunk/mozilla/xpcom/tests/TestAutoPtr.cpp:57: error:   initializing
temporary from result of `nsAutoPtr<T>::nsAutoPtr(T*) [with T = TestObject]'
Attachment #113556 - Flags: review?(dbaron) → review-
Attached patch patch to remove getter_Transfers (obsolete) — — Splinter Review 
This one also removes |begin_assignment| from nsRefPtr as cleanup that should
have happened when I fixed some build bustage before that was caused by having
the non-inlined function in the header.
Attachment #125243 - Flags: superreview?(jaggernaut)
Attachment #125243 - Flags: review?(bugmail)
Comment on attachment 125243 [details] [diff] [review]
patch to remove getter_Transfers

Oh, and I'll also remove |CreateTestObject| from TestAutoPtr.cpp since this
makes it unused, but I won't bother attaching a new patch just for that.
Comment on attachment 125243 [details] [diff] [review]
patch to remove getter_Transfers

Oh, never mind, transformix is using StartAssignment directly.
Attachment #125243 - Flags: superreview?(jaggernaut)
Attachment #125243 - Flags: superreview-
Attachment #125243 - Flags: review?(bugmail)
Attachment #125243 - Flags: review-
Attachment #125243 - Attachment is obsolete: true
Attachment #125243 - Flags: superreview-
Attachment #125243 - Flags: review-
Can't we just change those two places in Transformiix where we use
StartAssignment to something else?
Is it needed?  What I wanted to remove would remove the ability to do that kind
of thing entirely in any way other than casting and knowing what the internals
are.  (However, either way, you shouldn't be using |StartAssignment| directly.)
Yeah, i wasn't sure if using StartAssignment was a good idea. I had a gut
feeling it wasn't. However it performed exactly the functionality that I wanted
so in a weird way it kinda made sense to use it.

What I had was a class-member that is an nsAutoPointer, and I need to get a
pointer to the internal rawpointer in there. Is there a preffered way to do
this, or is that what getter_Transfers is for.

Anyhow, we need a way to get a pointer to the internal pointer, and that
function should probably delete+nullify the internal value. Personally I like
.StartAssignment() better since it better describes what actually happens, but
OTOH getter_Transfers is closer to getter_AddRefs so I guess it makes more sense.

David, i'm not sure what you mean with "casting and knowing what the internals
are", do you mean that I should do |&mFirstInstruction.mRawPtr|?

I'm perfectly fine with using any of
mNextInstrPtr = &mFirstInstruction.mRawPtr;
mNextInstrPtr = getter_Transfers(mFirstInstruction);
or
mNextInstrPtr = mFirstInstruction.StartAssignment();
QA Contact: scc → xpcom
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: