Closed Bug 1046282 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(5 files, 2 obsolete files)

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.)
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.)
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. :)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Note that we keep these in RefPtr, not an nsRefPtr.
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.
Attachment #8464906 - Flags: review?(cpearce)
(In reply to Ralph Giles (:rillian) from comment #3)
> Note that we keep these in RefPtr, not an nsRefPtr.

Not consistently, per comment 1.
(Noticed one more "mozilla::" prefix-removal that needed doing)

Rillian, feel free to steal any of the reviews here, if you like. :)
Attachment #8464906 - Attachment is obsolete: true
Attachment #8464906 - Flags: review?(cpearce)
Attachment #8464907 - Flags: review?(cpearce)
Attachment #8464907 - Attachment description: part 1 v2: → part 1 v2: make Create*Decoder decls use consistent style
One more stylistic fix, in code that's going to be touched by the "real" fix here -- this removes an unnecessary "else" after return.
Attachment #8464909 - Flags: review?(cpearce)
(Here's the "diff -w" version of part 2, in case it makes it easier to review.)
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.
Attachment #8464907 - Flags: review+
(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 on attachment 8464909 [details] [diff] [review]
part 2: remove else-after-return

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

LGTM.
Attachment #8464909 - Flags: review+
Attachment #8464909 - Flags: review?(cpearce)
(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!
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.
Attachment #8464946 - Flags: review?(giles)
Attachment #8464946 - Flags: feedback?(jwalden+bmo)
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?
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].)
Attachment #8464959 - Flags: review?(giles)
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.
Attachment #8464946 - Flags: review?(giles) → review+
(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.)
(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 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?
(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.)
(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.)
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 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.
Attachment #8464959 - Flags: review?(giles) → review+
This change is fine with me.
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.]
Attachment #8464946 - Flags: feedback?(jwalden+bmo)
Thanks for the reviews, rillian & cpearce!

Try run: https://tbpl.mozilla.org/?tree=Try&rev=7c9a03b2a9ba
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.