947905 - [Audio] Release OmxDecoder on main thread

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I discovered this problem while debugging bug 929029. OmxDecoder should always be released on the main thread.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 947905: Release OmxDecoder on main thread

Comment 2

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

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.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 947905: Release OmxDecoder on main thread

Thanks! I updated the patch according to your feedback.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks!

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

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=1ef3c3d86b98

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ef3c3d86b98