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

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla34
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.)
(Assignee)

Comment 1

4 years ago
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.)
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 8464906 [details] [diff] [review]
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.
Attachment #8464906 - Flags: review?(cpearce)
(Assignee)

Comment 5

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

Not consistently, per comment 1.
(Assignee)

Comment 6

4 years ago
Created attachment 8464907 [details] [diff] [review]
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. :)
Attachment #8464906 - Attachment is obsolete: true
Attachment #8464906 - Flags: review?(cpearce)
Attachment #8464907 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Attachment #8464907 - Attachment description: part 1 v2: → part 1 v2: make Create*Decoder decls use consistent style
(Assignee)

Comment 7

4 years ago
Created attachment 8464909 [details] [diff] [review]
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.
Attachment #8464909 - Flags: review?(cpearce)
(Assignee)

Comment 8

4 years ago
Created attachment 8464911 [details] [diff] [review]
part 2, "diff -w" version

(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+
(Assignee)

Comment 10

4 years ago
(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+
(Assignee)

Comment 12

4 years ago
Created attachment 8464938 [details] [diff] [review]
part 1 v3: make Create*Decoder decls use consistent style (now with AppleDecoderModule) [r=rillian]
Attachment #8464907 - Attachment is obsolete: true
Attachment #8464907 - Flags: review?(cpearce)
Attachment #8464938 - Flags: review+
(Assignee)

Updated

4 years ago
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!
(Assignee)

Comment 14

4 years ago
Created attachment 8464946 [details] [diff] [review]
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.
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?
(Assignee)

Comment 16

4 years ago
Created attachment 8464959 [details] [diff] [review]
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].)
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+
(Assignee)

Comment 18

4 years ago
(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?
(Assignee)

Comment 21

4 years ago
(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.)
(Assignee)

Comment 22

4 years ago
(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.
Attachment #8464946 - Flags: review+
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.
(Assignee)

Comment 26

4 years ago
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)
(Assignee)

Comment 27

4 years ago
Thanks for the reviews, rillian & cpearce!

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

Updated

4 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.