461087 - provide templates for automatically-releasing handles to foreign resources

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Karl Tomlinson (:karlt)]

It would be useful to have classes similar to nsAutoPtr but for resources
destroyed by means other than delete, and similar to nsRefPtr but for
resources that are not objects without AddRef() and Release() methods.

Often the resources are native objects where adding a custom delete or
AddRef() and Release() methods is not practical.  The handle or reference to
the resource may not even be a pointer.

Comment 1

[Karl Tomlinson (:karlt)]

I started something similar as gfxRefPtrWrap in attachment 344063 [details] [diff] [review] for
pointers to Pango and fontconfig objects.

This was a class with the object type as a template parameter, and with
Release() and when needed AddRef() methods that are specialized for each
object type.

In bug 449356 comment 9, roc commented

  "Seems like gfxRefPtrWrap should go into XPCOM and be split into "refcounted
   object" and "custom-destructor object". How about nsRefObj/nsAutoObj? (The
   template parameter doesn't need to be a pointer, after all --- nsAutoObj
   would be useful for HDCs, for example).

I'm currently favoring nsAutoRef or nsScopedRef, and nsCountedRef.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Those names sound good to me.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: nsAutoRef and nsCountedRef

This provides two main classes: nsAutoRef<T> and nsCountedRef<T>.  This
difference is not really in the type of destruction of T, which is handled by
template specialization, but in whether the object adds a reference on
construction.

nsAutoRef<T> is similar to scoped_ptr in that it does not have a copy
constructor, but nsAutoRef<T> has methods and class nsReturnRef<T> to explicitly pass the reference to a another smart ref or a raw ref.

Template specialization is required for each resource type. e.g.

// A ref counted object with a pointer handle
NS_SPECIALIZE_TEMPLATE inline void
nsAutoRefTraits<FcPattern>::Release(FcPattern *ptr) { FcPatternDestroy(ptr); }
NS_SPECIALIZE_TEMPLATE inline void
nsAutoRefTraits<FcPattern>::AddRef(FcPattern *ptr) { FcPatternReference(ptr); }

// A custom-delete object
NS_SPECIALIZE_TEMPLATE inline void
nsAutoRefTraits<PRFileDesc>::Release(PRFileDesc *ptr) { PR_Close(ptr); }

// Several objects may have the same traits
template <class T>
class gfxGObjectRefTraits {
public:
    typedef T* RawRef;
    static RawRef Void() { return NULL; }
    static void Release(RawRef aPtr) { g_object_unref(aPtr); }
    static void AddRef(RawRef aPtr) { g_object_ref(aPtr); }
};
NS_SPECIALIZE_TEMPLATE
class nsAutoRefTraits<PangoFont> : public gfxGObjectRefTraits<PangoFont> { };
NS_SPECIALIZE_TEMPLATE
class nsAutoRefTraits<GdkDrawable> : public gfxGObjectRefTraits<PangoFont> { };

// A non-pointer resource
class nsRawFD;
NS_SPECIALIZE_TEMPLATE
class nsAutoRefTraits<nsRawFD> {
public:
    typedef int RawRef;
    static int Void() { return -1; }
    static void Release(RawRef aFD) { close(aFD); }
};


// Example of use:

nsReturnRef<nsRawFD> get_file(const char *filename) {
    nsAutoRef<nsRawFD> fd(open(filename, O_RDONLY));
    fcntl(fd, F_SETFD, FD_CLOEXEC);
    return fd.out();
}

void f() {
    unsigned char buf[1024];

    nsAutoRef<nsRawFD> fd1(get_file("/etc/hosts"));

    nsAutoRef<nsRawFD> fd2;
    fd2.steal(fd1);
    ssize_t count = read(fd1, buf, 1024); // error
    count = read(fd2, buf, 1024);

    get_file("/etc/login.defs"); // login.defs is closed

    fd1 = get_file("/etc/passwd");
    int rawfd = fd1.disown();

    fd1.own(open("/proc/1/maps");
    // fd1 closes maps and fd2 closes hosts on destruction
    // but passwd is not closed.
}


Note that there are two methods to pass the reference out of the smart ref.

Using out() ensures that the resource will either be released or passed to
another smart pointer that will release unless explicitly told otherwise.
out() is related to already_AddRefed<T> nsRefPtr<T>::forget() in that it
indicates that the resource must be released in a way that accepting objects
understand.  Unlike this forget(), out() ensures that the resource is released
even when not taken by another object.

disown() is similar to T* nsAutoPtr::forget() in that it provides a raw ptr,
containing no information about ownership.  The caller needs to ensure that
this is handled correctly.

I haven't used the method name "forget" here because it is not clear which is
the forget method, so it is better to use distinct names for distinct
functions.  Also I like the own/disown inverses better than remember/forget.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> NS_SPECIALIZE_TEMPLATE
> class nsAutoRefTraits<GdkDrawable> : public gfxGObjectRefTraits<PangoFont> { };

FWIW that should be

NS_SPECIALIZE_TEMPLATE
class nsAutoRefTraits<GdkDrawable> : public gfxGObjectRefTraits<GdkDrawable> { };

Comment 5

[Benjamin Smedberg]

Comment on attachment 345558 [details] [diff] [review]
nsAutoRef and nsCountedRef

>diff --git a/xpcom/base/Makefile.in b/xpcom/base/Makefile.in

> EXPORTS		= \
> 		nsAgg.h \
> 		nsAutoPtr.h \
>+		nsAutoRef.h \

I'm a little sad that you couldn't implement the existing nsAutoPtr and nsRefPtr stuff in terms of this new class, rather than having an entirely separate header. Is that just a time constraint that could be fixed later, or are there fundamental difficulties?

>diff --git a/xpcom/base/nsAutoRef.h b/xpcom/base/nsAutoRef.h

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

Actually, copyright from all our work is assigned to the foundation.

Overall, it was very difficult to read this file from the point of view "how would I use these classes". I would like an overview comment to guide newcomers telling them which class to use.

>+/**
>+ * template <class T> class nsAutoRefTraits

Having the opening comment be about nsAutoRefTraits<T> is unfortunate. Could we forward-declare the traits class and put the default implementation at the end of the file? Or better yet, don't have a default implementation at all, and require every client to provide a specialization according to a common pattern as you've described in the comment?

I especially don't like that AddRef and Release are declared but not implemented. This means that instead of issuing a compile error, clients of these classes won't get an error until link time, when it's much harder to debug (and linkers have much less detailed error messages in general).

>+ * For simple pointer references, it is sufficient to specialize
>+ * |nsAutoRefTraits<T>::Release(RawRef)| to enable |nsAutoRef<T>| for
>+ * custom-deleted or ref-counted resources.

I'd like this to be more active-voice: "specializations must override Release to properly finalize the handle for custom deletion or ref-counted resources.

>+ * For pointer references to ref-counted resources, also specializing
>+ * |nsAutoRefTraits::AddRef(RawRef)| enables |nsCountedRef<T>|.

"Specializations for handles which are reference-counted must..."

>+template <class T>
>+class nsAutoRefTraits {

nit: standard XPCOM style now starts class and function blocks on the following line:
class nsAutoRefTraits
{

>+public:
>+    // The type of the handle to the resource.
>+    // This default implementation of nsAutoRefTraits provides pointers to T.
>+    typedef T* RawRef;

It's not clear: are specializations required to provide a typedef of this name?

>+/**
>+ * template <class T> class nsSimpleRef
>+ *
>+ * Constructs a non-smart reference, and provides methods to test whether
>+ * there is an associated resource and (if so) get its raw handle.  Not
>+ * intended for direct use but used by |nsAutoRef<T>| and thus
>+ * |nsCountedRef<T>|.

If this class is not intended for direct use, and is only subclassed by one other class, why does it exist? Is it meant to be specialized? It is not clear from the docs.

>+    // A class to allow ownership to be transferred from nsReturnRef function
>+    // return values.  The conversion operator on nsReturnRef constructs a
>+    // temporary wrapper of class Returning around a non-const reference to
>+    // the nsReturnRef.  The wrapper can then be passed as an rvalue
>+    // parameter.
>+    class Returning {

Does this class need to be inner to nsSimpleRef? Couldn't it either be inner to nsReturnRef or a toplevel class?

>+    // A class providing access to protected nsSimpleRef<T> constructors
>+    // for temporary simple (non-ThisClass) references.

This comment is completely unclear; under what circumstances would client code choose to use a LocalSimpleRef?

>+/**
>+ * template <class T> class nsAutoRef
>+ *
>+ * A class that creates (adds) a new reference to a resource on construction
>+ * or assignment and releases on destruction.

You mean nsCountedRef here... the specific difference between this and an nsAutoRef is that copy-constructors and assignment operators are available, right? Perhaps that could be made more explicit.

Some of these issues are perhaps caused by C++ template metaprogramming being hard to read in general. They could perhaps be addressed by simply instructing clients reading the header to skip the helper classes and go straight to nsAutoRef/nsCountedRef?

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> (From update of attachment 345558 [details] [diff] [review])
> >diff --git a/xpcom/base/Makefile.in b/xpcom/base/Makefile.in
> 
> > EXPORTS		= \
> > 		nsAgg.h \
> > 		nsAutoPtr.h \
> >+		nsAutoRef.h \
> 
> I'm a little sad that you couldn't implement the existing nsAutoPtr and
> nsRefPtr stuff in terms of this new class, rather than having an entirely
> separate header. Is that just a time constraint that could be fixed later, or
> are there fundamental difficulties?

There are definitely similarities here.  This is indicated by the fact that
the following would make nsAutoRef<AClassCreatingWithNew> behave similarly to
an nsAutoPtr<AClassCreatingWithNew> (without a copy constructor or copy
assignment operator):

  NS_SPECIALIZE_TEMPLATE inline void
  nsAutoRefTraits<AClassCreatingWithNew>::Release(RawRef aPtr) { delete aPtr; }

Also, the following would make nsCountedRef<ARefCountedClass> behave
similarly to nsRefPtr<ARefCountedClass>:

  NS_SPECIALIZE_TEMPLATE inline void
  nsAutoRefTraits<ARefCountedClass>::Release(RawRef aPtr) { aPtr->Release(); }
  NS_SPECIALIZE_TEMPLATE inline void
  nsAutoRefTraits<ARefCountedClass>::AddRef(RawRef aPtr) { aPtr->AddRef(); }


I'm not clear whether you are imagining replacing nsAutoPtr and nsRefPtr with
this new class (throughout the code) or implementing nsAutoPtr and nsRefPtr as
classes derived from this new class?

One issue with replacing would be that the default implementation would either
be for new/delete objects or refcounted objects.  The other set of objects I
think would either need a separate implementation, or a specialization for each
class, or perhaps a different implementation of nsAutoRefBase with more
complex template parameter logic may be possible.

Regarding implementing nsAutoPtr and nsRefPtr as classes derived from this new
class, I'm not convinced that it would save many lines of code because
constructors and assignment operators are not inherited.  I think it would
make nsAutoPtr and nsRefPtr harder to read.

> Overall, it was very difficult to read this file from the point of view "how
> would I use these classes". I would like an overview comment to guide newcomers
> telling them which class to use.

I'll add some overview documentation, probably using the examples in comment 3.

> 
> >+/**
> >+ * template <class T> class nsAutoRefTraits
> 
> Having the opening comment be about nsAutoRefTraits<T> is unfortunate. Could we
> forward-declare the traits class and put the default implementation at the end
> of the file?

nsAutoRefTraits<T> is the class that needs to be considered first when adding
support for a new resource type.

However, once this has been set up, it is other classes that are most
interesting.  I can move them earlier in the file.

> Or better yet, don't have a default implementation at all, and
> require every client to provide a specialization according to a common pattern
> as you've described in the comment?
> 
> I especially don't like that AddRef and Release are declared but not
> implemented. This means that instead of issuing a compile error, clients of
> these classes won't get an error until link time, when it's much harder to
> debug (and linkers have much less detailed error messages in general).

The intention here was that, for many resources with pointer references, only
the definition of nsAutoRefTraits<T>::Release(RawRef) needs to be specialized:
(This requires a declaration in the default class.)

  NS_SPECIALIZE_TEMPLATE inline void
  nsAutoRefTraits<PRFileDesc>::Release(PRFileDesc *ptr) { PR_Close(ptr); }

One possibility may be to provide a base traits class for pointers

  template <class T>
  class nsPointerRefTraits {
  public:
      typedef T* RawRef;
      static RawRef Void() { return nsnull };
  }

to slightly simplify the client's class specialization

  NS_SPECIALIZE_TEMPLATE inline void
  nsAutoRefTraits<PRFileDesc> : public nsPointerRefTraits<PRFileDesc> {
      static void Release(PRFileDesc *ptr) { PR_Close(ptr); }
  }

Thoughts?

> >+public:
> >+    // The type of the handle to the resource.
> >+    // This default implementation of nsAutoRefTraits provides pointers to T.
> >+    typedef T* RawRef;
> 
> It's not clear: are specializations required to provide a typedef of this name?

Yes.  I'll clarify that.

> 
> >+/**
> >+ * template <class T> class nsSimpleRef
> >+ *
> >+ * Constructs a non-smart reference, and provides methods to test whether
> >+ * there is an associated resource and (if so) get its raw handle.  Not
> >+ * intended for direct use but used by |nsAutoRef<T>| and thus
> >+ * |nsCountedRef<T>|.
> 
> If this class is not intended for direct use, and is only subclassed by one
> other class, why does it exist? Is it meant to be specialized? It is not clear
> from the docs.

It is meant for specialization when there is no such thing as a void value for
the resource handle.  i.e. when the default nsSimpleRef<T> implementation with
nsAutoRefTraits<T> is not general enough.

The docs say "Specialize this class if there is no particular void value for
the resource handle." but I can at least move that to a new paragraph.

> 
> >+    // A class to allow ownership to be transferred from nsReturnRef function
> >+    // return values.  The conversion operator on nsReturnRef constructs a
> >+    // temporary wrapper of class Returning around a non-const reference to
> >+    // the nsReturnRef.  The wrapper can then be passed as an rvalue
> >+    // parameter.
> >+    class Returning {
> 
> Does this class need to be inner to nsSimpleRef?

It is inner to nsAutoRefBase.

> Couldn't it either be inner to nsReturnRef or a toplevel class?

I'll think about this.  I currently like inner to nsReturnRef.

I thought a protected class inner to nsAutoRefBase gave the best indication of
who needed to care and who had access to what.  nsReturnRef constructs this
class, nsAutoRefBase and nsAutoRef access mReturnRef, and nsCountedRef accept
parameters of this class.

But maybe the fact that public interfaces take this as a parameter means the
user will look for documentation on this class and so hiding it is not such a
good idea.

A toplevel class with friend nsReturnRef for a private constructor and public
mReturnRef is possible.  Choosing a name is difficult: nsReturningRef and
nsReturnRefRef are confusing.  An inner class has the advantage of being more
obviously different from nsReturnRef.

If inner to nsReturnRef, then it would either be public or nsAutoRef and
nsCountedRef would need to be friends of nsReturnRef.

> 
> >+    // A class providing access to protected nsSimpleRef<T> constructors
> >+    // for temporary simple (non-ThisClass) references.
> 
> This comment is completely unclear; under what circumstances would client code
> choose to use a LocalSimpleRef?

LocalSimpleRef is protected and only intended for use within nsAutoRefBase and
derived classes.  I'll make this more clear.

nsSimpleRef constructors were made protected to indicate that nsSimpleRef
objects should not be constructed by clients.  LocalSimpleRef works around
that.

> 
> >+/**
> >+ * template <class T> class nsAutoRef
> >+ *
> >+ * A class that creates (adds) a new reference to a resource on construction
> >+ * or assignment and releases on destruction.
> 
> You mean nsCountedRef here... the specific difference between this and an
> nsAutoRef is that copy-constructors and assignment operators are available,
> right? Perhaps that could be made more explicit.

Right.  OK.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> > >+    class Returning {

> > Couldn't it either be inner to nsReturnRef or a toplevel class?
> 
> I'll think about this.  I currently like inner to nsReturnRef.

Returning cannot be inner to nsReturnRef<T> and used in nsAutoRefBase<T>
because nsReturnRef<T> is derived from nsAutoRefBase<T>.

template <class T> struct C; // nsReturnRef

template <class T>
struct B { // nsAutoRefBase
  B();
  explicit B(const typename C<T>::O& o);  // line 6
};

template <class T>
struct C : public B<T> { // line 10
  class O {}; // Returning
};

template <class T>
struct C2 {
  class O {};
};

void fun() {
  C2<int> c2; // fine because C2 is not derived from B
  C<int> c; // line 21
}

g++ -c inner.cpp
inner.cpp: In instantiation of 'B<int>':
inner.cpp:10:   instantiated from 'C<int>'
inner.cpp:21:   instantiated from here
inner.cpp:6: error: no type named 'O' in 'struct C<int>'

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: nsAutoRef and nsCountedRef v2

The major changes here are:

Rearranged classes and added documentation.
nsAutoRefBase<T>::Returning -> nsReturningRef<T>.
No default nsAutoRefTraits<T>.

Comment 9

[Benjamin Smedberg]

Comment on attachment 346255 [details] [diff] [review]
nsAutoRef and nsCountedRef v2

Yeah, much nicer! Please put up docs on MDC after you land this, and file a followup to implement nsRefPtr<Class> in terms of nsCountedRef and nsAutoPtr<T> in terms of nsAutoRef. (No tree-wide code changes, just changing the implementation).

Comment 10

[Karl Tomlinson (:karlt)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/8262c2e1f3d9

Filed bug 463337.  Still need to put up docs.

Comment 11

[Karl Tomlinson (:karlt)]

Docs at

https://developer.mozilla.org/En/NsAutoRef
https://developer.mozilla.org/En/NsAutoRefTraits
https://developer.mozilla.org/En/NsCountedRef

Filed bug 472392.