732821 - Remove RefPtr's implicit conversion to TemporaryRef

Status: RESOLVED INVALID

(Core :: MFBT, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

The point of a TemporaryRef is returning a pointer with a strong reference to a caller, while avoiding an unnecessary (and possibly slow) addref-release pair. As shown in bug 702799, though, implicitly clearing out the RefPtr can cause issues when returning a member variable rather than a local one. That bug changed the implicit conversion to, instead of passing the reference on to the caller, implicitly addref. However, as can be seen in this patch, this is the wrong choice for most cases.

As there are two reasonable behaviours here, it seems better to require an explicit decision on the caller's side. This is also in line with already_AddRefed.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 602745 [details] [diff] [review]
Patch v1

Review of attachment 602745 [details] [diff] [review]:
-----------------------------------------------------------------

This change seems analogous to WTF's adoptRef function, right?  The different is just that our forget() returns the temporary ref (TemporaryRef), whereas their leakRef() nulls out and returns the raw pointer, which adoptRef then packages up into a temporary ref (PassRefPtr).  It seems at first glance that what we do is better, because we don't expose any way to null out the pointer and leak the raw (addrefed) pointer (absent deliberate obtuseness by the author, but they'd have to try to do it).  Is that right?  Just want to be sure I understand all the tradeoffs here.

r=me assuming my understanding in the above paragraph is correct, otherwise poke me on IRC so we can make sure I understand everything here before we pull the trigger on this.

::: gfx/2d/DrawTargetCairo.cpp
@@ +673,5 @@
>  TemporaryRef<SourceSurface>
>  DrawTargetCairo::OptimizeSourceSurface(SourceSurface *aSurface) const
>  {
> +  RefPtr<SourceSurface> surface = aSurface;
> +  return surface.forget();

This seems simpler and clearer as |return TemporaryRef<SourceSurface>(aSurface);|.

::: gfx/2d/PathCairo.cpp
@@ +277,5 @@
>  TemporaryRef<CairoPathContext>
>  PathBuilderCairo::GetPathContext()
>  {
> +  RefPtr<CairoPathContext> pathContext = mPathContext;
> +  return pathContext.forget();

|return TemporaryRef<CairoPathContext>(mPathContext);|

@@ +341,5 @@
>  TemporaryRef<CairoPathContext>
>  PathCairo::GetPathContext()
>  {
> +  RefPtr<CairoPathContext> pathContext = mPathContext;
> +  return pathContext.forget();

|return TemporaryRef<CairoPathContext>(mPathContext);|

Comment 2

[Nika Layzell [:nika] (ni? for response)]

TemporaryRef doesn't exist anymore, as it's been replaced in MFBT by already_AddRefed