Closed Bug 1160914 Opened 9 years ago Closed 9 years ago

[EME] Crash in VideoDecoder::SampleToVideoFrame()

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- unaffected
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Adobe reported that they are crashing in VideoDecoder::SampleToVideoFrame().

Call stack is:

eme-adobe.dll!VideoDecoder::SampleToVideoFrame(IMFSample * aSample, GMPVideoi420Frame * aVideoFrame, int newWidth, int newHeight, int newStride) Line 673
C++
  eme-adobe.dll!VideoDecoder::ReturnOutput(IMFSample * aSample, int newWidth, int newHeight, int newStride) Line 285
C++
  eme-adobe.dll!gmp_task_args_m_4<VideoDecoder *,void (__thiscall VideoDecoder::*)(IMFSample *,int,int,int),wmf::CComPtr<IMFSample>,int,int,int>::Run() Line 371
C++
  eme-adobe.dll!`VideoDecoder::MaybeRunOnMainThread'::`2'::MaybeRunTask::Run() Line 798
C++

The problem is I think that the main thread is spinning and processing events while waiting for a GMPVideoHost::CreateFrame() call to finish, and during that time a DecodingComplete() call comes in which deletes the VideoDecoder object, and then we deref free'd memory.
* Make VideoDecoder, AudioDecoder, and ClearKeySessionManager threadsafe refcounted.
* Make the tasks that call into the VideoDecoder, AudioDecoder, and ClearKeySessionManager on the main thread hold a refcount, so that if the decoder is shutdown, the memory being used isn't freed until after the call exits.
* Check return values of calls into GMPVideoHost or GMPVideoi420FrameImpl/GMPPlaneImpl so that we detect failure earlier.

I can't reproduce any failures, but this should help...
Attachment #8600752 - Flags: review?(edwin)
Fixing this crash in Adobe's CDM is a release blocker (for Adobe), but fixing this crash in the ClearKey CDM is not a Firefox release blocker.
Simple refcount-using-locks when <atomic> is not available:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab2710984cb
We can't just #include <atomic>, as that breaks on our build machines. So implement threadsafe refcounting via locks where <atomic> doesn't work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=604b511fca6f
Attachment #8601371 - Flags: review?(edwin)
Comment on attachment 8601371 [details] [diff] [review]
Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic is not easily available

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

::: media/gmp-clearkey/0.1/ClearKeySession.cpp
@@ +22,5 @@
>  
>  #include "gmp-api/gmp-decryption.h"
>  #include "Endian.h"
>  #include <assert.h>
> +#include <string.h>

Unnecessary, AFAICT. Debugging artefact?

::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp
@@ +23,5 @@
>  #include "ClearKeyUtils.h"
>  #include "ClearKeyBase64.h"
>  #include "ArrayUtils.h"
>  #include <assert.h>
> +#include <memory.h>

Same here.
Attachment #8601371 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #9)
> Comment on attachment 8601371 [details] [diff] [review]
> Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic
> is not easily available
> 
> Review of attachment 8601371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/gmp-clearkey/0.1/ClearKeySession.cpp
> @@ +22,5 @@
> >  
> >  #include "gmp-api/gmp-decryption.h"
> >  #include "Endian.h"
> >  #include <assert.h>
> > +#include <string.h>
> 
> Unnecessary, AFAICT. Debugging artefact?
> 
> ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp
> @@ +23,5 @@
> >  #include "ClearKeyUtils.h"
> >  #include "ClearKeyBase64.h"
> >  #include "ArrayUtils.h"
> >  #include <assert.h>
> > +#include <memory.h>
> 
> Same here.

These were required to fix non-unified build in my Linux VM.
Uplift to 39 needed.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/44317690f61d
https://hg.mozilla.org/mozilla-central/rev/0d4307edffc2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8600752 [details] [diff] [review]
Patch: Make gmp-clearkey's decoders threadsafe refcounted

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: Negligable. This fixes a crash in our our test CDM, so will help make our units tests more green.
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.
[Risks and why]: Low; this doesn't touch code that we expect users to use.
[String/UUID change made/needed]: None.
Attachment #8600752 - Flags: approval-mozilla-beta?
Comment on attachment 8601371 [details] [diff] [review]
Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic is not easily available

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: Negligable. This fixes a crash in our our test CDM, so will help make our units tests more green.
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.
[Risks and why]: Low; this doesn't touch code that we expect users to use.
[String/UUID change made/needed]: None.
Attachment #8601371 - Flags: approval-mozilla-beta?
The patch here is based on top of Bug 1160321, so we should land that on beta 38 first.
Flags: needinfo?(cpearce)
Flags: needinfo?(cpearce)
Bug 1160321 is still waiting for another patch to land (in bug 1164480) so let's wait to land them all together on 39.
Chris are you expecting this to uplift to 38.0.5?
No. I figured we should target EME fixes for 39.

I'd like to uplift this, and all other EME uplift requests, in time to be in the first 39 beta build, so that when 39 hits the beta channel all fixes are in.
Flags: needinfo?(cpearce)
Flags: needinfo?(cpearce)
Comment on attachment 8600752 [details] [diff] [review]
Patch: Make gmp-clearkey's decoders threadsafe refcounted

Approved for uplift to beta (39). Low risk change in test CDM.
Attachment #8600752 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8601371 [details] [diff] [review]
Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic is not easily available

Approved for uplift to beta (39) to improve EME test instability.
Attachment #8601371 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I give up trying to rebase/uplift this; and since it only really affect our toy CDM, it's not a huge loss.
Attachment #8600752 - Flags: approval-mozilla-beta+
Attachment #8601371 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.