Deprecate already_AddRefed now that RefPtr supports move construction

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

There should be no advantage to using already_AddRefed anymore.
But already_AddRefed is not just above Move semantics.  It's also about asserting if nothing takes ownership.  Is that possible with just plain RefPtr now?
Yeah, I think I agree with bkelly.

If MyFunction() returns already_AddRefed<T>, then you can't do:

  T* t = MyFunction();

but if it returns RefPtr<T>, then I believe you can, and if you do, you end up with a dangling pointer.
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #2)
> But already_AddRefed is not just above Move semantics.  It's also about
> asserting if nothing takes ownership.  Is that possible with just plain
> RefPtr now?

MOZ_MUST_USE?
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #3)
> Yeah, I think I agree with bkelly.
> 
> If MyFunction() returns already_AddRefed<T>, then you can't do:
> 
>   T* t = MyFunction();
> 
> but if it returns RefPtr<T>, then I believe you can, and if you do, you end
> up with a dangling pointer.

This is guaranteed against, at least in msvc2015:

9:40.17 c:/dev/mozilla/gecko-cinn/dom/canvas/WebGLContext.cpp(142): error C2280: 'RefPtr<mozilla::WebGLContext>::operator T(void) const &&': attempting to reference a deleted function
 9:40.17         with
 9:40.17         [
 9:40.17             T=mozilla::WebGLContext
 9:40.17         ]
 9:40.17 c:\dev\mozilla\gecko-cinn-obj\dist\include\mozilla/RefPtr.h(303): note: see declaration of 'RefPtr<mozilla::WebGLContext>::operator T'
 9:40.17         with
 9:40.17         [
 9:40.17             T=mozilla::WebGLContext
 9:40.17         ]

For:

static RefPtr<WebGLContext>
Foo()
{
    return nullptr;
}

[...]
Ll41    WebGLContext* foo = Foo().get();
Ll42    WebGLContext* bar = Foo();
[...]
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #3)
> If MyFunction() returns already_AddRefed<T>, then you can't do:
> 
>   T* t = MyFunction();
> 
> but if it returns RefPtr<T>, then I believe you can, and if you do, you end
> up with a dangling pointer.

Not so.  In RefPtr:

  // Don't allow implicit conversion of temporary RefPtr to raw pointer,
  // because the refcount might be one and the pointer will immediately become
  // invalid.
  operator T*() const && = delete;

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #2)
> It's also about asserting if nothing takes ownership.  Is that possible with
> just plain RefPtr now?

It probably wouldn't be difficult to add a static analysis detecting temporary RefPtrs that are dropt on the floor.  But given that assigning a RefPtr to a T* doesn't compile, there isn't the leak/crash hazard that already_AddRefed presents, that mandated its was-consumed assertion.

And I confess I don't think it's a widely pressing problem if a function that returns a RefPtr is called and drops it on the ground without using it.  The instances where you'd *want* to do this seem rare, and the addref/release to create/destroy the temporary -- if it actually matters as overhead -- is something you necessarily have if you return already_AddRefed now (because already_AddRefed implies the addref, and the something that must take ownership of it implies the release).  There's no efficiency win to already_AddRefed that returning RefPtr lacks.
Comment on attachment 8849751 [details]
Bug 1349379 - Clarify when to use already_AddRefed. -

https://reviewboard.mozilla.org/r/122522/#review124722

::: mfbt/AlreadyAddRefed.h:31
(Diff revision 1)
>   * pointer.
>   *
>   * TODO Move already_AddRefed to namespace mozilla.  This has not yet been done
>   * because of the sheer number of usages of already_AddRefed.
> + *
> + * @deprecated Unnecessary now that RefPtr supports move construction.

This is a little terse, IMO.  "This class is unnecessary now that RefPtr supports move construction.  New code should return a RefPtr instead of an already_AddRefed."  Otherwise, fine by me.  (And from past discussion, more recently than the last newsgroup discussions of this issue, I can say froydnj is of similar mind.)
Attachment #8849751 - Flags: review?(jwalden+bmo) → review+
So, I guess the other question:  does RefPtr integrate with XPCOM_MEM_COMPTR_LOG, which makes it substantially easier to debug reference-counting leaks of frequently refcounted objects?
Also see the issue raised by Boris in the dev-platform list last September: 

https://groups.google.com/forum/m/#!searchin/mozilla.dev.platform/already_AddRefed/mozilla.dev.platform/ffXWJaMo7Ig 

Without already_AddRefed you cannot be sure the consumer of your API took ownership. This makes writing OMT code that deals with MT objects a lot harder.
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #8)
> So, I guess the other question:  does RefPtr integrate with
> XPCOM_MEM_COMPTR_LOG, which makes it substantially easier to debug
> reference-counting leaks of frequently refcounted objects?

Since I've never seen its mechanisms, I'm assuming it tracks actual AddRef/Release, which should have the same pattern for move construction as with already_AddRefed. (which is really just an ancestor to move construction)



(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #10)
> Also see the issue raised by Boris in the dev-platform list last September: 
> 
> https://groups.google.com/forum/m/#!searchin/mozilla.dev.platform/
> already_AddRefed/mozilla.dev.platform/ffXWJaMo7Ig 
> 
> Without already_AddRefed you cannot be sure the consumer of your API took
> ownership. This makes writing OMT code that deals with MT objects a lot
> harder.

`MOZ_MUST_USE RefPtr<> Foo()` ought to have as least as good of guarantees as `already_AddRefed<> Foo()`. (better actually, since it's statically ensured!)
The issue with XPCOM_MEM_COMPTR_LOG is that I'd oppose conversion of nsCOMPtr and nsRefPtr to mozilla::RefPtr until it's properly integrated with XPCOM_MEM_COMPTR_LOG, which (last I checked) it wasn't.
(I think the end state of bug 935778 was that we had refcount logging but not COMPTR logging (which records the pointer identity of the smart pointer with addrefs and releases so that matched reference counting that comes from smart pointers can be eliminated from a balance tree.)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)
> The issue with XPCOM_MEM_COMPTR_LOG is that I'd oppose conversion of
> nsCOMPtr and nsRefPtr to mozilla::RefPtr until it's properly integrated with
> XPCOM_MEM_COMPTR_LOG, which (last I checked) it wasn't.

Seems reasonable. I don't think we're proposing this here, though. Just that mfbt's already_AddRefed should be deprecated for mfbt's RefPtr where possible.
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Seems reasonable. I don't think we're proposing this here, though. Just that
> mfbt's already_AddRefed should be deprecated for mfbt's RefPtr where
> possible.

But I think there's only one already_AddRefed; I thought they were merged, and nsCOMPtr shares the implementation.  So you'd be deprecating the thing that nsCOMPtr uses.

(And yes, nsRefPtr is gone, and we lost the logging there already...)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > Seems reasonable. I don't think we're proposing this here, though. Just that
> > mfbt's already_AddRefed should be deprecated for mfbt's RefPtr where
> > possible.
> 
> But I think there's only one already_AddRefed; I thought they were merged,
> and nsCOMPtr shares the implementation.  So you'd be deprecating the thing
> that nsCOMPtr uses.
> 
> (And yes, nsRefPtr is gone, and we lost the logging there already...)

That's true.
Here let's only deprecate already_AddRefed for RefPtr, not nsCOMPtr.
I'll file a follow-up where we can look at supporting RefPtr&& in nsCOMPtr.
Sorry, my link in comment 10 was broken.  Link to the issue raised on dev-platform previously:

https://groups.google.com/d/msg/mozilla.dev.platform/ffXWJaMo7Ig/3ty1pKy4AAAJ

Jeff, please address this before trying to remove already_AddRefed. Its a legit use case that we have to deal with in OMT code like workers. AFAIK RefPtr move semantics don't offer these guarantees and it would make it hard to write/review this code.
Flags: needinfo?(jgilbert)
This message in the same thread might better describe it:

https://groups.google.com/d/msg/mozilla.dev.platform/ffXWJaMo7Ig/Mly1w0y3AAAJ

> It's worse than that.  For a wide range of callers (anyone handing the
> ref across threads), the caller must check immediately whether the
> callee actually took the value and if not make sure things are released
> on the proper thread...
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #18)
> Sorry, my link in comment 10 was broken.  Link to the issue raised on
> dev-platform previously:
> 
> https://groups.google.com/d/msg/mozilla.dev.platform/ffXWJaMo7Ig/3ty1pKy4AAAJ
> 
> Jeff, please address this before trying to remove already_AddRefed. Its a
> legit use case that we have to deal with in OMT code like workers. AFAIK
> RefPtr move semantics don't offer these guarantees and it would make it hard
> to write/review this code.

I think there could still be a forget() method, returning a new RefPtr that takes over the ownership without touching the ref counter.
(In reply to Gerald Squelart [:gerald] from comment #20)
> I think there could still be a forget() method, returning a new RefPtr that
> takes over the ownership without touching the ref counter.

I don't see how this buys us anything without a type that asserts in its destructor that ownership has been taken by another RefPtr.
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #21)
> (In reply to Gerald Squelart [:gerald] from comment #20)
> I don't see how this buys us anything without a type that asserts in its
> destructor that ownership has been taken by another RefPtr.

What I understood from bz's post that you cited in comment 18, was the issue with touching the ref-counter (of a possibly-non-threadsafe object) in a different thread, which was solved by doing a double forget(), which I think will still be solvable with an updated forget() that returns a RefPtr instead.

The other issue of asserting that ownership has been taken is a separate thing, and indeed I'm not sure there's a way to do that with just RefPtr.

Anyway, sorry for interrupting, I should let Jeff respond!
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #19)
> This message in the same thread might better describe it:
> 
> https://groups.google.com/d/msg/mozilla.dev.platform/ffXWJaMo7Ig/Mly1w0y3AAAJ
> 
> > It's worse than that.  For a wide range of callers (anyone handing the
> > ref across threads), the caller must check immediately whether the
> > callee actually took the value and if not make sure things are released
> > on the proper thread...

Please describe how `already_AddRefed` differs from returning `MOZ_MUST_USE RefPtr` in this case.
Flags: needinfo?(jgilbert) → needinfo?(bkelly)
I haven't explicitly checked, but I expect they differ in a few ways:

1) `Unused << Foo()` will assert if Foo() returns already_AddRefed, but not with `MOZ_MUST_USE RefPtr`.
2) `if (!Foo())` will assert (or not compile) if Foo() returns already_AddRefed, but not with `MOZ_MUST_USE RefPtr`.
3) `Foo()->Bar()` will assert (or not compile) if Foo() returns already_AddRefed, but not with `MOZ_MUST_USE RefPtr`.

All of these cases use the return value, but don't take ownership.  Again, I haven't actually tested them.
Flags: needinfo?(bkelly)
1) Why would anyone care about this?  Asserting is only a good thing to detect/correct a leak.  There's no leak if RefPtr's returned.
2) Again, why would anyone care about this?  Same rationale.
3) This is not a problematic use case.  Foo()'s RefPtr will not be destructed until Bar() has completed, so |this| will remain addref'd the entire time.  And the standard refcounting contract is that you have to addref passed arguments that you stash away for later, so the release when Foo()->Bar() has run presents no problem.

Those uses, frankly, seem like over-fitting to how already_AddRefed happens to work without actually considering whether there's any reason it *should* work that way, in a C++11 world.

As to .forget(), you can pass a RefPtr without an addref by passing Move(ptr) where the parameter's type is RefPtr.
If I have code like

already_AddRefed<Foo>
TakeFoo()
{
  return mFoo.forget();
}

The API is self documenting in that I am clearly passing ownership to the caller.  You must take ownership or it will assert.  Direct use of the return value as a pointer will also just not compile.

As stated previously this matters for code running off-the-main-thread that still needs to deal with main thread objects.  We want to ensure that Release() is never called OMT.  Therefore we not only need to move the ref, but ensure that ownership is taken in these cases.

If I have to return RefPtr<> instead of already_AddRefed<> reviewers must audit every callsite of the function to ensure ownership is taken.  In addition, the API suddenly allows unsafe usage of the method that previously would not compile with already_AddRefed<>.

For example, if we are forced to change TakeFoo() to:'

  MOZ_MUST_USE RefPtr<Foo>
  TakeFoo()
  {
    return Move(mFoo);
  }

Then someone can accidentally write:

  if (rareCondition && !TakeFoo()) {
    // do something
  }

If rareCondition never triggers in a test then we will end up releasing Foo on the wrong thread.  Making sure this doesn't happen increases the load on the reviewer.

What I don't understand is the desire to remove already_AddRefed<>.  Nothing stops you from using Move(RefPtr).  Go ahead and use it.  But why do you insist on removing the tool we have for explicit ownership passing?  Why can't they both be in the tree?
> If rareCondition never triggers in a test then we will end up releasing Foo on the wrong thread.

Sorry, I meant to say:

If rareCondition never triggers in a test then we may not catch this condition and ship code that releases Foo in the wrong thread.
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #26)
> If I have code like
> 
> already_AddRefed<Foo>
> TakeFoo()
> {
>   return mFoo.forget();
> }

It is also possible to write:
already_AddRefed<Foo>
TakeFoo()
{
  RefPtr<Foo> foo = mFoo;
  return foo.forget();
}

You still need to review the implementation to know if the ownership is transferred from mFoo or not.
Is there anything guaranteeing one must use Move() in the method implementations? If not, is there anything guaranteeing we don't end up seeing more AddRef/Release calls (last time I checked, there wasn't such guarantee when returning RefPtr).
already_AddRefed ensures the right/expected AddRef/Release calls.

Also, in general, why to use Move() with RefPtr when we already have already_AddRefed?
(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> (In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment
> #19)
> > This message in the same thread might better describe it:
> > 
> > https://groups.google.com/d/msg/mozilla.dev.platform/ffXWJaMo7Ig/Mly1w0y3AAAJ
> > 
> > > It's worse than that.  For a wide range of callers (anyone handing the
> > > ref across threads), the caller must check immediately whether the
> > > callee actually took the value and if not make sure things are released
> > > on the proper thread...
> 
> Please describe how `already_AddRefed` differs from returning `MOZ_MUST_USE
> RefPtr` in this case.

The referred-to message on dev-platform concerns taking RefPtr/RefPtr&& vs. already_AddRefed for parameter types, not return value types.  MOZ_MUST_USE doesn't help you here.
I suggest changing the comments to look something like this instead of deprecating/removing already_AddRefed<>:

 // When should I use already_AddRefed<>?
 //   This type enforces that the ownership of the ref-counted object
 //   is taken be the consuming code.  You should use it when you
 //   want to ensure ownership is passed.
 //
 // When should I use a Move(RefPtr)?
 //   Many code wants to return a ref-counted object, but does not
 //   care if the object is owned by the caller or used as a temporary.
 //   In these cases its preferable to return Move(RefPtr)

Or something to that effect.

Again, it seems that they have different uses and both can live in the tree at the same time.  I don't disagree that many callsites would benefit from returning a movable RefPtr<>.  But we should still be able to express explicit ownership passing via already_AddRefed<>.
Comment on attachment 8849795 [details]
Bug 1349379 - Deprecate already_AddRefed only with RefPtr, not with nsCOMPtr. -

https://reviewboard.mozilla.org/r/122556/#review125212

I don't think this really represents the discussion above regarding parameters, such as comment 19 and comment 30 (which doesn't seem specific to nsCOMPtr/RefPtr).
Attachment #8849795 - Flags: review?(dbaron) → review-
Attachment #8849795 - Attachment is obsolete: true
Comment on attachment 8849751 [details]
Bug 1349379 - Clarify when to use already_AddRefed. -

https://reviewboard.mozilla.org/r/122522/#review126006
Attachment #8849751 - Flags: review?(dbaron) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcdbcf403b9c
Clarify when to use already_AddRefed. - r=dbaron,waldo
https://hg.mozilla.org/mozilla-central/rev/bcdbcf403b9c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.