Benjamin Smedberg recently announced that he was handing over XPCOM module ownership duties to me. XPCOM contains basic data structures used throughout the Mozilla codebase, so changes to its code can have wide-ranging effects. I’m honored to have been given responsibility for a core piece of the Gecko platform.
One issue that’s come up recently and I’m sure will continue to come up is changing XPCOM data structures to support two new C++11 features, rvalue references and their killer app, move constructors. If you aren’t familiar with C++11’s new rvalue references feature, I highly recommend C++ Rvalue References Explained. Move constructors are already being put to good use elsewhere in the codebase, notably mozilla::UniquePtr, which can be used to replace XPCOM’s nsAutoPtr and nsAutoRef (bug 1055035). And some XPCOM data structures have received the move constructor treatment, notably nsRefPtr (bug 980753) and nsTArray (bug 982212).
A recent discussion and the associated bug, however, decided that the core referenced-counted smart pointer class in XPCOM, nsCOMPtr, shouldn’t support move constructors. While move constructors could have replaced the already_AddRefed usage associated with nsCOMPtr, such as:
already_AddRefed<nsIMyInterface>
NS_DoSomething(...)
{
nsCOMPtr<nsIMyInterface> interface = ...;
// do some initialization stuff
return interface.forget();
}
with the slightly shorter:
nsCOMPtr<nsIMyInterface>
NS_DoSomething(...)
{
nsCOMPtr<nsIMyInterface> interface = ...;
// do some initialization stuff
return interface;
}
There were two primary arguments against move constructor support. The first argument was that the explicitness of having to call .forget() on an nsCOMPtr (along with the explicitness of the already_AddRefed type), rather than returning it, is valuable for the code author, the patch reviewer, and subsequent readers of the code. When dealing with ownership issues in C++, it pays to be more explicit, rather than less. The second argument was that due to the implicit conversion of nsCOMPtr<T> to a bare T* pointer (a common pattern in smart pointer classes), returning nsCOMPtr<T> from functions makes it potentially easy to write buggy code:
// What really happens in the below piece of code is something like:
//
// nsIMyInterface* p;
// {
// nsCOMPtr<nsIMyInterface> tmp(NS_DoSomething(...));
// p = tmp.get();
// }
//
// which is bad if NS_DoSomething is returning the only ref to the object.
// p now points to deleted memory, which is a security risk.
nsIMyInterface* p = NS_DoSomething(...);
(I should note that we can return nsCOMPtr<T> from functions today, and in most cases, thanks to compiler optimizations, it will be as efficient as returning already_AddRefed. But Gecko culture is such that a function returning nsCOMPtr<T> would be quite unusual, and therefore unlikely to pass code review.)
The changes to add move constructors to nsRefPtr and nsTArray? They were reviewed by me. And the nixing of move constructors for nsCOMPtr? That was also done by me (with a lot of feedback from other people).
I accept the charge of inconsistency. However, I offer the following defense. In the case of nsTArray, there are no ownership issues like there are with nsCOMPtr: you either own the array, or you don’t, so many of the issues raised about nsCOMPtr don’t apply in that case.
For the case of nsRefPtr, it is true that I didn’t seek out as much input from other people before approving the patch. But the nsRefPtr patch was also done without the explicit goal of removing already_AddRefed from the code base, which made it both smaller in scope and more palatable. Also, my hunch is that nsRefPtr is used somewhat less than nsCOMPtr (although this may be changing somewhat given other improvements in the codebase, like WebIDL), and so it provides an interesting testbed for whether move constructors and/or less explicit transfers of ownership are as much of a problem as argued above.