Closed Bug 1145405 Opened 9 years ago Closed 9 years ago

[EME] ClearKey CDM DecodingComplete race

Categories

(Core :: Audio/Video, defect, P2)

38 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: eflores, Assigned: eflores)

Details

Attachments

(1 file)

Chris pointed out that when we call DecodingComplete() on the CDM, the worker thread might still have queued tasks that will dispatch to the main thread and cause the CDM to segfault.
A bit quick and dirty.

Wrap tasks sent to main thread and make sure they won't try to access any dead state.
Attachment #8580408 - Flags: review?(cpearce)
Comment on attachment 8580408 [details] [diff] [review]
clearkey-decodingcomplete-race.patch

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

What if the task to call DecodingComplete() runs before the tasks you're dispatching run? Then you'll still crash. That is, at the time MaybeDispatchTask is called, there could be a task in the main thread's task queue waiting to run that calls DecodingComplete(), and the task that you add to the main thread's task queue will run after the DecodingComplete() task.

Maybe we should make *Decoder refcounted, and pass RefPtr<*Decoder> to WrapTask(). You then need to check the shutdown flag when your tasks run?
Attachment #8580408 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #2)
> What if the task to call DecodingComplete() runs before the tasks you're
> dispatching run? Then you'll still crash. That is, at the time
> MaybeDispatchTask is called, there could be a task in the main thread's task
> queue waiting to run that calls DecodingComplete(), and the task that you
> add to the main thread's task queue will run after the DecodingComplete()
> task.

Hm?

Since DecodingComplete waits for the worker thread to join, and only adds another task onto the main thread queue to delete |this|, VideoDecoder is only deleted once all the tasks that need it have completed.
Comment on attachment 8580408 [details] [diff] [review]
clearkey-decodingcomplete-race.patch

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

OK. I missed that.
Attachment #8580408 - Flags: review- → review+
Comment on attachment 8580408 [details] [diff] [review]
clearkey-decodingcomplete-race.patch

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

::: media/gmp-clearkey/0.1/AudioDecoder.cpp
@@ +257,5 @@
>  {
>    if (mWorkerThread) {
>      mWorkerThread->Join();
>    }
> +  mHasShutdown = true;

Whoops, note to self: mHasShutdown not initialised.
Better uplift to 38.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/6e36e76aa855
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8580408 [details] [diff] [review]
clearkey-decodingcomplete-race.patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: Potentially we could get crashes in EME CDMs
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.
[Risks and why]: Potentially this could break clearkey EME playback, but that's a toy decoder that no one uses for real EME content, only for testing.
[String/UUID change made/needed]: None.
Attachment #8580408 - Flags: approval-mozilla-aurora?
Attachment #8580408 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(edwin)
This needs bug 1138777, which I meant to request uplift on yesterday, but somehow forgot. :(
Flags: needinfo?(edwin)
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: