[Audio] Release OmxDecoder on main thread

RESOLVED FIXED in mozilla29

Status

()

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

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

Trunk
mozilla29
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I discovered this problem while debugging bug 929029. OmxDecoder should always be released on the main thread.
Created attachment 8344618 [details] [diff] [review]
[01] Bug 947905: Release OmxDecoder on main thread
Attachment #8344618 - Flags: review?(cpearce)
Comment on attachment 8344618 [details] [diff] [review]
[01] Bug 947905: Release OmxDecoder on main thread

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

I'm not really qualified to review changes to the OMX decoder, Chris Double is probably the best person.

I recommend that you add MOZ_ASSERT(NS_IsMainThread()) calls in the ctor and dtor of OmxDecoder to shake out more of these issues however.

::: content/media/omx/OmxDecoder.cpp
@@ +51,5 @@
> +
> +  NS_METHOD Run() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    return NS_OK; // mOmxDecoder is released by destructor

I recommend that you manually set mOmxDecoder to null here. If someone changes the code to hold a reference to the runnable on the thread that calls NS_DispatchToMainThread() then the event can run to completion before NS_DispatchToMainThread(r) returns, and if so the dtor will run on the dispatching thread, i.e. not the thread you want it to.

That can't happen now because you're not storing the runnable in a refptr on the dispatching thread, but I think it pays to be defensive.
Attachment #8344618 - Flags: review?(cpearce)
Attachment #8344618 - Flags: review?(chris.double)
Attachment #8344618 - Flags: feedback+
Created attachment 8345209 [details] [diff] [review]
[01] Bug 947905: Release OmxDecoder on main thread

Thanks! I updated the patch according to your feedback.
Attachment #8344618 - Attachment is obsolete: true
Attachment #8344618 - Flags: review?(chris.double)
Attachment #8345209 - Flags: review?(chris.double)

Updated

4 years ago
Attachment #8345209 - Flags: review?(chris.double) → review+
Thanks!

https://hg.mozilla.org/integration/b2g-inbound/rev/1ef3c3d86b98

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=1ef3c3d86b98
https://hg.mozilla.org/mozilla-central/rev/1ef3c3d86b98
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.