MediaDecoderReader shouldn't define AddRef/Release methods

RESOLVED FIXED in mozilla21

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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)

Comment 1

5 years ago
Created attachment 703168 [details] [diff] [review]
Remove reference counting
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)
(Assignee)

Comment 3

5 years ago
(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

5 years ago
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+

Updated

5 years ago
Blocks: 792263
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5db6d38f34ee
(Assignee)

Comment 6

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.