Closed Opened 10 years ago Closed 10 years ago

Not set
normal

RESOLVED FIXED
mozilla21

## Attachments

### (1 file)

 1.18 KB, patch ehsan.akhgari : review+ ehsan.akhgari : feedback+ Details | Diff | Splinter Review
```MediaDecoderReader.h is defined as:

...
..
};

But it is held via an nsAutoPtr in MediaDecoderStateMachine:

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]:
-----------------------------------------------------------------

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

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
`https://hg.mozilla.org/integration/mozilla-inbound/rev/5db6d38f34ee`
```(In reply to :Ehsan Akhgari from comment #4)
>
> I actually don't _need_ MediaDecoderReader to be refcounted, I was just
`https://hg.mozilla.org/mozilla-central/rev/5db6d38f34ee`