884027 - MFBT RefPtr interoperability with WTF refcounted objects is a bad idea, because of differing refcounted semantics

Status: NEW

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Here is a basic difference between Mozilla-style and WebKit-style refcounting:
 - Mozilla (also COM) objects are created with a refcount of 0.
 - WebKit (also Skia) objects are created with a refcount of 1.

I feel that that difference may have been overlooked in the decision to support WTF interop in MFBT RefCounted, namely, this:

http://hg.mozilla.org/mozilla-central/file/b7175c5829b5/mfbt/RefPtr.h#l85

    86     void ref() { AddRef(); }
    87     void deref() { Release(); }
    88     int refCount() const { return refCnt; }

Here is a practical example of how the false sense of interoperability that this can give, is dangerous:

    RefPtr<T> p = new T;

This is a very common idiom in Mozilla land, and works of course fine with Mozilla refcounted objects, but this leaks when T is a WTF refcounted type, because T gets created with a refcount of 1 (instead of 0 for mozilla refcounted types), then RefPtr::operator= increments the refcount to 2, and when eventually the RefPtr goes out of scope, the refcount only goes back to 1, so the object is not actually free'd.

Comment 1

[Jeff Walden [:Waldo]]

The interop argument was mostly about the name, not about the functionality being identical.  COM reference counting is never going to be compatible with RefPtr/RefCounted semantics, so someone would have to go out of their way to screw things up, probably.  But your point is noted.

I believe WTF added an adoptRef standalone function to be used in cases like that (returning something like our already_AddRefed<T>, and then RefPtr would skip an addref when constructing), and they deleted the RefPtr<T>::RefPtr(T*) constructor.  (Or at least they were incrementally adding a whole bunch of adoptRef calls for a very long time, in pursuit of *some* goal something like this.)  Such a function, if we implemented one, could assert the current reference count to be > 0, to make instances of the problem painfully obvious.

Something AdoptRef-ish (with removing the problematic constructor, and having only ref/deref and no AddRef/Release), if it actually addresses the issue, is totally cool with me.  I almost certainly don't have the time to go out of my way to change anything here myself, tho.

Comment 2

[Jeff Walden [:Waldo]]

Bug 732821 is faintly related to this, if you squint slightly, I think.