1046282 - PlatformDecoderModule "CreateH264Decoder" / "CreateAACDecoder" objects should return already_AddRefed

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, CreateH264Decoder and CreateAACDecoder return raw pointers to freshly-allocated, not-yet-AddRef'ed, refcounted objects.

Their return type should instead be "already_AddRefed" (generated by a local nsRefPtr.forget()), to ensure that their return value is stuck in a nsRefPtr (instead of, say, a raw pointer), which reduces leaks and footguns.

(Noticed this while skimming the EME impl of these methods, while trying to fix bug 1046263.)

Comment 1

[Daniel Holbert [:dholbert]]

Actually, it looks like we're inconsistent about whether this raw pointer gets stuck in a nsRefPtr vs. a RefPtr.

In MP4Reader.cpp, we have:
> mAudio.mDecoder = mPlatform->CreateAACDecoder(...
and
> mVideo.mDecoder = mPlatform->CreateH264Decoder(...
where those mDecoder variables have type RefPtr<mozilla::MediaDataDecoder>.

In EMEDecoderModule.cpp, we have:
> nsRefPtr<MediaDataDecoder> decoder(mPDM->CreateH264Decoder(...
which is obviously sticking the result in a nsRefPtr.

IIRC, the semantics are slightly different between RefPtr vs. nsRefPtr, and we should only use RefPtr for things that derive from RefCounted<T>.  So I think the RefPtr usages here are broken.

(Using already_AddRefed in the return type will help us catch this sort of bug in the future, because already_AddRefed can't be assigned to a RefPtr.)

Comment 2

[Daniel Holbert [:dholbert]]

Tentatively taking, since I'm already looking at this, though I may punt to someone on the media team if it's more trouble than I'm expecting. :)

Comment 3

[Ralph Giles (:rillian)]

Note that we keep these in RefPtr, not an nsRefPtr.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 1: make Create*Decoder decls use consistent style

Firstly, this makes the Create*Decoder decls use a consistent style, on:
 1. putting the return type on its own line (because otherwise it's way too wide) (note that it'll be even wider after the return type becomes already_AddRefed<>)
 2. Not bothering with "mozilla::" namespace prefix (since this is all inside of namespace mozilla
 3. Putting MOZ_OVERRIDE at the end of the final line, instead of on its own line.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Ralph Giles (:rillian) from comment #3)
> Note that we keep these in RefPtr, not an nsRefPtr.

Not consistently, per comment 1.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: make Create*Decoder decls use consistent style

(Noticed one more "mozilla::" prefix-removal that needed doing)

Rillian, feel free to steal any of the reviews here, if you like. :)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 2: remove else-after-return

One more stylistic fix, in code that's going to be touched by the "real" fix here -- this removes an unnecessary "else" after return.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 2, "diff -w" version

(Here's the "diff -w" version of part 2, in case it makes it easier to review.)

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8464907 [details] [diff] [review]
part 1 v2: make Create*Decoder decls use consistent style

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

LGTM. You may also wish to apply this fix to AppleDecoderModule which I just landed on inbound.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Ralph Giles (:rillian) from comment #9)
> LGTM. You may also wish to apply this fix to AppleDecoderModule which I just
> landed on inbound.

Thanks for the heads-up -- I'll update my patches to fix that as well.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8464909 [details] [diff] [review]
part 2: remove else-after-return

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

LGTM.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: part 1 v3: make Create*Decoder decls use consistent style (now with AppleDecoderModule) [r=rillian]

Comment 13

[Ralph Giles (:rillian)]

(In reply to Daniel Holbert [:dholbert] from comment #12)

> part 1 v3: make Create*Decoder decls use consistent style (now with
> AppleDecoderModule) [r=rillian]

Thanks!

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: part 3: Use nsRefPtr instead of RefPtr

This patch converts a RefPtr "mDecoder" to be nsRefPtr instead, per comment 1 / comment 3. (This also converts another member-var ("mTaskQueue") alongside it, too, for consistency & good measure.)

These pointers aren't used very thoroughly -- they're just for lifetime-management, I think. For example, we don't call .forget() on them anywhere (It'd be a small problem if we did, since the forget() return-type differs between nsRefPtr & RefPtr).

Here are their usages:
http://mxr.mozilla.org/mozilla-central/search?string=mDecoder&find=content%2Fmedia%2Ffmp4%2FMP4Reader
http://mxr.mozilla.org/mozilla-central/search?string=mTaskQueue&find=content%2Fmedia%2Ffmp4%2FMP4Reader&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

So, I think it doesn't matter whether they're nsRefPtr or RefPtr. So, this should have no functional effect.  (I'm doing it because it's necessary for the next part -- to be able to assign this member-var to an already_AddRefed<> ptr.)

Requesting a sanity-check from Waldo on RefPtr-->nsRefPtr conversion "gotchas", at Ms2ger's suggestion.

Comment 15

[Ralph Giles (:rillian)]

I don't understand our smart pointers all that well. What's the advantage of this over changing the EME uses to RefPtr? I naively prefer the mfbt class because it's smaller and easier to read.

I believe, RefPtr is always already_AddRefed. In particular, RefPtr.h says:

> * RefCounted<T> is created with refcount == 0.  Newly-allocated
> * RefCounted<T> must immediately be assigned to a RefPtr to make the
> * refcount > 0.  It's an error to allocate and free a bare
> * RefCounted<T>, i.e. outside of the RefPtr machinery.  Attempts to
> * do so will abort DEBUG builds.

Would returning an RefPtr<Foo> instead of Foo* from Create() give you the protection you want?

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: part 4: make Create*Decoder return already_AddRefed

Here's the actual fix here -- using nsRefPtr local-variables in each of these function impls, which we call forget() on to get the return-value.

(This is functionally equivalent to what we already have, except we'll now do our AddRef() a little earlier (going from refcount 0 to 1), _before_ returning from Create*Decoder.  We also get stronger type-safety, and we'll assert if anyone drops our return-value on the floor [to prevent leaks].)

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8464946 [details] [diff] [review]
part 3: Use nsRefPtr instead of RefPtr

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

OK, I see. You're saying NS_INLINE_DECL_THREADSAFE_REFCOUNTING implies nsRefPtr instead of RefPtr.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Ralph Giles (:rillian) from comment #15)
> I don't understand our smart pointers all that well. What's the advantage of
> this over changing the EME uses to RefPtr? I naively prefer the mfbt class
> because it's smaller and easier to read.

RefPtr is deprecated, as far as I know, because (a) it's subtly different from nsRefPtr (I forget exactly how), and that difference causes surprise and is a footgun, and (b) we have better tooling/logging with nsRefPtr.

> I believe, RefPtr is always already_AddRefed. In particular, RefPtr.h says:

> > * RefCounted<T> is created with refcount == 0.  Newly-allocated
> > * RefCounted<T> must immediately be assigned to a RefPtr to make the
> > * refcount > 0.  It's an error to allocate and free a bare
> > * RefCounted<T>, i.e. outside of the RefPtr machinery.  Attempts to
> > * do so will abort DEBUG builds.

None of that applies here -- that's talking about classes that derive from RefCounted<T> (to provide AddRef() and Release()), but these classes do not derive from RefCounted<T>

e.g. WMFMediaDataDecoder derives from MediaDataDecoder, which derives from MediaDataDecoder, which doesn't derive from anything (and which uses NS_INLINE_DECL_THREADSAFE_REFCOUNTING to provide its AddRef & Release impls)

See e.g. http://mxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h?rev=14fe3ddc2e9c#156 for a class which *does* derive from RefCounted<T>.

> Would returning an RefPtr<Foo> instead of Foo* from Create() give you the
> protection you want?

I don't think so -- the correct RefPtr analog would be to declare a local RefPtr and return *its* .forget() method (which is TemporaryRef<>, which is the RefPtr equivalent of already_AddRefed).

(In reply to Ralph Giles (:rillian) from comment #17)
> OK, I see. You're saying NS_INLINE_DECL_THREADSAFE_REFCOUNTING implies
> nsRefPtr instead of RefPtr.

I'm not 100% sure it *has* to imply nsRefPtr (I think I've seen NS_INLINE*REFCOUNTING mixed with RefPtr elsewhere), but it's probably better to be consistent about ns vs. non-ns, yeah.  (Plus, I'm told RefPtr is deprecated.)

Comment 19

[Ralph Giles (:rillian)]

(In reply to Daniel Holbert [:dholbert] from comment #18)

> RefPtr is deprecated, as far as I know, because (a) it's subtly different
> from nsRefPtr (I forget exactly how), and that difference causes surprise
> and is a footgun, and (b) we have better tooling/logging with nsRefPtr.

I don't agree with that. There seem to be too camps, each without much success convincing the other to switch. I thought the logging support got fixed, though.

> I'm not 100% sure it *has* to imply nsRefPtr (I think I've seen
> NS_INLINE*REFCOUNTING mixed with RefPtr elsewhere), but it's probably better
> to be consistent about ns vs. non-ns, yeah.

It just seems to depend on presence of AddRef/Release methods so I agree both types work.

> the correct RefPtr analog would be to declare a local RefPtr and return
> *its* .forget() method (which is TemporaryRef<>, which is the RefPtr
> equivalent of already_AddRefed).

Ok, no complexity win there. Just returning a RefPtr<> as such doubles (triples?) the add/release calls.

Comment 20

[Ralph Giles (:rillian)]

Comment on attachment 8464959 [details] [diff] [review]
part 4: make Create*Decoder return already_AddRefed

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

Does what it says on the tin. I want to check with cpearce before giving final media peer approval though. We have an meeting in an hour; I'll respond after that.

::: content/media/fmp4/apple/AppleDecoderModule.cpp
@@ +73,5 @@
>                                        MediaTaskQueue* aVideoTaskQueue,
>                                        MediaDataDecoderCallback* aCallback)
>  {
> +  nsRefPtr<MediaDataDecoder> decoder =
> +    AppleVTDecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer);

Need a 'new' here?

::: content/media/fmp4/ffmpeg/FFmpegDecoderModule.h
@@ +33,5 @@
>                      MediaDataDecoderCallback* aCallback) MOZ_OVERRIDE
>    {
> +    nsRefPtr<MediaDataDecoder> decoder =
> +      new FFmpegH264Decoder<V>(aVideoTaskQueue, aCallback, aConfig,
> +                               aImageContainer);

Do the arguments fit on one 80 column line now?

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Ralph Giles (:rillian) from comment #19)
> > RefPtr is deprecated, as far as I know
> 
> I don't agree with that. There seem to be too camps, each without much
> success convincing the other to switch. I thought the logging support got
> fixed, though.

Maybe "deprecated" was overstating it (though that was my impression).

> > I'm not 100% sure it *has* to imply nsRefPtr (I think I've seen
> > NS_INLINE*REFCOUNTING mixed with RefPtr elsewhere), but it's probably better
> > to be consistent about ns vs. non-ns, yeah.
> 
> It just seems to depend on presence of AddRef/Release methods so I agree
> both types work.

Yeah, I think so. We just need to be consistent about the type, so that we can strengthen these ::Create* methods to return something that can't be dropped on the floor (like a raw pointer can). And each of our options (TemporaryRef vs already_AddRefed) will only work with one of the reference-counting smart pointer types.

> Ok, no complexity win there. Just returning a RefPtr<> as such doubles
> (triples?) the add/release calls.

(Yup, returning a RefPtr would incur extra Add/Release calls, as you say. Fortunately, TemporaryRef/already_AddRefed are great for avoiding that.)

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Ralph Giles (:rillian) from comment #20)
> Does what it says on the tin. I want to check with cpearce before giving
> final media peer approval though. We have an meeting in an hour; I'll
> respond after that.

Thanks! (He recently added the EMEDecoderModule.cpp nsRefPtr-usage quoted in comment 1, so I suspect he'll be happy with this; otherwise, EMEDecoderModule.cpp will needs a lot of ns-removing conversion.)

> ::: content/media/fmp4/apple/AppleDecoderModule.cpp
> > +  nsRefPtr<MediaDataDecoder> decoder =
> > +    AppleVTDecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer);
> 
> Need a 'new' here?

Oops! Thanks. As you can see, I haven't tried to compile on Mac yet. :)

> ::: content/media/fmp4/ffmpeg/FFmpegDecoderModule.h
> @@ +33,5 @@
> >                      MediaDataDecoderCallback* aCallback) MOZ_OVERRIDE
> >    {
> > +    nsRefPtr<MediaDataDecoder> decoder =
> > +      new FFmpegH264Decoder<V>(aVideoTaskQueue, aCallback, aConfig,
> > +                               aImageContainer);
> 
> Do the arguments fit on one 80 column line now?

Not quite. (If I remove the newline before aImageContainer, I get an 86-character line.)

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

I started the RefPtr vs nsRefPtr thread, and my takeaway was that the nsRefPtr folks outnumbered the RefPtr folks, so the mob ruled in favour of nsRefPtr.

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8464959 [details] [diff] [review]
part 4: make Create*Decoder return already_AddRefed

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

I'll take that as a yes. r=me with missing 'new' addressed.

> Not quite. (If I remove the newline before aImageContainer, I get an 86-character line.)

Ok, just wanted to check. It's hard to tell in the review interface.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

This change is fine with me.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8464946 [details] [diff] [review]
part 3: Use nsRefPtr instead of RefPtr

[Canceling Waldo feedback request on part 3, since [after reading the thread] I confirmed that the main thing to be concerned about when converting from RefPtr to nsReftr is semantics around what .forget() produces -- and no one's calling .forget() on these member-variables, so we shouldn't have anything to worry about.]

[Waldo, do still feel free to chime in if you have any thoughts/concerns on this, though.]

Comment 27

[Daniel Holbert [:dholbert]]

Thanks for the reviews, rillian & cpearce!

Try run: https://tbpl.mozilla.org/?tree=Try&rev=7c9a03b2a9ba

Comment 28

[Daniel Holbert [:dholbert]]

Try looks good.

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa04e03bb82
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2457c3f9b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b20c7a46fca
https://hg.mozilla.org/integration/mozilla-inbound/rev/17bbd05e72d7

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fa04e03bb82
https://hg.mozilla.org/mozilla-central/rev/eb2457c3f9b3
https://hg.mozilla.org/mozilla-central/rev/1b20c7a46fca
https://hg.mozilla.org/mozilla-central/rev/17bbd05e72d7