Closed
Bug 1145405
Opened 9 years ago
Closed 9 years ago
[EME] ClearKey CDM DecodingComplete race
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: eflores, Assigned: eflores)
Details
Attachments
(1 file)
11.75 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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-
Assignee | ||
Comment 3•9 years ago
|
||
(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 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e36e76aa855
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6e36e76aa855
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 9•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox38:
--- → affected
Updated•9 years ago
|
Attachment #8580408 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 11•9 years ago
|
||
This needs bug 1138777, which I meant to request uplift on yesterday, but somehow forgot. :(
Flags: needinfo?(edwin)
Updated•9 years ago
|
Flags: needinfo?(cpearce)
You need to log in
before you can comment on or make changes to this bug.
Description
•