Closed Bug 947905 Opened 6 years ago Closed 6 years ago

[Audio] Release OmxDecoder on main thread

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Details

Attachments

(1 file, 1 obsolete file)

I discovered this problem while debugging bug 929029. OmxDecoder should always be released on the main thread.
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+
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)
Attachment #8345209 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/1ef3c3d86b98
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.