Closed Bug 831640 Opened 8 years ago Closed 8 years ago

MediaDecoderReader shouldn't define AddRef/Release methods

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attachment #703168 - Flags: review?(cpearce)
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?
Attachment #703168 - Flags: feedback?(ehsan)
(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 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!  :-)
Attachment #703168 - Flags: review?(cpearce)
Attachment #703168 - Flags: review+
Attachment #703168 - Flags: feedback?(ehsan)
Attachment #703168 - Flags: feedback+
Blocks: 792263
(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!
https://hg.mozilla.org/mozilla-central/rev/5db6d38f34ee
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.