831640 - MediaDecoderReader shouldn't define AddRef/Release methods

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

MediaDecoderReader.h is defined as:

class MediaDecoderReader {
...
  NS_INLINE_DECL_REFCOUNTING(MediaDecoderReader)
..
};

But it is held via an nsAutoPtr in MediaDecoderStateMachine:

  nsAutoPtr<MediaDecoderReader> mReader;

Nothing uses the refcounting definitions. In some code I used an nsRefPtr to hold an mReader in an event, thinking this was safe as it had reference counting methods, but this caused early destruction of the reader when  the nsRefPtr destructed.

The NS_INLINE_DECL_REFCOUNTING line should probably be removed.

Comment 1

[cajbir (:cajbir)]

Attachment: Remove reference counting

Comment 2

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

Comment on attachment 703168 [details] [diff] [review]
Remove reference counting

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

I think Ehsan's patch over in bug 792263 actually needs MediaDecoderReader to be refcounted, and indeed his patch even changes MediaDecoderReader's Addref/Release methods to be NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDecoderReader). Right Ehsan?

Comment 3

[cajbir (:cajbir)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> I think Ehsan's patch over in bug 792263 actually needs MediaDecoderReader
> to be refcounted, and indeed his patch even changes MediaDecoderReader's
> Addref/Release methods to be
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDecoderReader). Right Ehsan?

It's fine if MediaDecoderReader is going to change to be refcounted to control the lifetime. If so this bug will morph into "Don't use nsAutoPtr to hold a reference to a MediaDecoderReader" as is done in MediaDecoderStateMachine. That should be an nsRefPtr if the intent is for these objects to be refcounted.

Comment 4

[(no longer active)]

Comment on attachment 703168 [details] [diff] [review]
Remove reference counting

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

I actually don't _need_ MediaDecoderReader to be refcounted, I was just misguided by the fact that it had defined AddRef/Release.  In fact, if you're up to see where this AddRef/Release definition comes from (and have a chuckle at the same time), look at bug 812572.  Turns out that MediaDecoderReader was an nsRunnable for no good reason.  Then I removed that inheritance and added the inline refcounting machinery without actually checking whether that's needed, and then started to make my patch depend on them.

Sorry!  :-)

Comment 5

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5db6d38f34ee

Comment 6

[cajbir (:cajbir)]

(In reply to :Ehsan Akhgari from comment #4)
> 
> I actually don't _need_ MediaDecoderReader to be refcounted, I was just
> misguided by the fact that it had defined AddRef/Release. 

Same thing happened to me with the current patch I'm working on. I used reference counting because it looked like it supported it then had crashes because the refcount would destroy the object, later followed by the nsAutoPtr, oops!

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/5db6d38f34ee