925371 - Inconsistent behavior between nsRefPtr.forget() and RefPtr.forget()

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Eric Rescorla (:ekr)]

nsRefPtr and RefPtr .forget() behave very differently:

- nsRefPtr returns an already_AddRefed<> which has an empty dtor
- RefPtr returns a TemporaryRef<> which unrefs in the door.

The implication here is that:

{
  nsRefPtr<T> t(new T();
  t.forget();
}

Leaves a live object.


Whereas:
{
 RefPtr<T> t(new T());
 t.forget();
}

Destroys the object.

This seems fairly dangerous since there seems to be some feeling that
people should move from nsRefPtr to RefPtr, and obviously that's not safe
in this instance.

Comment 1

[Eric Rescorla (:ekr)]

FWIW, my proposal would be to change RefPtr. We should

1. Audit to see if a lot of people use RefPtr.forget(). If not, let's just change it.
2. If a lot of people use it, let's globally do s/RefPtr.forget()/s/RefPtr.convert_to_temporary()/ and produce a new .forget() or maybe .give_up() to avoid people getting confused.

Comment 2

[(no longer active)]

At the risk of converting this into a philosophical discussion, I always assumed that the reason why RefPtr exists is to ease porting code using wtf::RefPtr.  Is that still true?  If yes, maybe we should consider renaming its functions so that they don't look like nsRefPtr at all...  After all, these two classes share almost none of each other's semantics.

Comment 3

[Eric Rescorla (:ekr)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> At the risk of converting this into a philosophical discussion, I always
> assumed that the reason why RefPtr exists is to ease porting code using
> wtf::RefPtr.  Is that still true?  If yes, maybe we should consider renaming
> its functions so that they don't look like nsRefPtr at all...  After all,
> these two classes share almost none of each other's semantics.

If we are to do that, then we should probably consider making RefPtr<T>
not work with the existing nsISupports-conforming reference counting
interface so that people don't get confused about what they are supposed
to use.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Seems to be fixed by bug 1207245 removing the old RefPtr and renaming nsRefPtr — the new RefPtr has the old nsRefPtr's type for forget(), returning an already_AddRefed.