1173195 - [EME] Fatal Shmem::AssertInvariants if GMPLoaderImpl::Load fails

Status: RESOLVED FIXED

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

Description

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

If I change GMPLoaderImpl::Load() to return false, I get an assertion failure running mse-clearkey:


mozilla::ipc::Shmem::AssertInvariants() Line 317	C++
mozilla::ipc::Shmem::Size<unsigned char>() Line 173	C++
mozilla::gmp::GMPSharedMemManager::MgrAllocShmem(mozilla::gmp::GMPSharedMem::GMPMemoryClasses aClass, unsigned int aSize, mozilla::ipc::SharedMemory::SharedMemoryType aType, mozilla::ipc::Shmem * aMem) Line 44	C++
mozilla::gmp::GMPVideoEncodedFrameImpl::CreateEmptyFrame(unsigned int aSize) Line 138	C++
mozilla::dom::CreateFrame(GMPVideoHost * aHost) Line 353	C++
mozilla::dom::TestGMPVideoDecoder::InitGMPDone(GMPVideoDecoderProxy * aGMP, GMPVideoHost * aHost) Line 454	C++
nsRunnableMethodArguments<GMPVideoDecoderProxy *,GMPVideoHost *>::apply<mozilla::dom::TestGMPVideoDecoder,void (__thiscall mozilla::dom::TestGMPVideoDecoder::*)(GMPVideoDecoderProxy *,GMPVideoHost *)>(mozilla::dom::TestGMPVideoDecoder * o, void (GMPVideoDecoderProxy *, GMPVideoHost *) * m) Line 647	C++
nsRunnableMethodImpl<void (__thiscall mozilla::dom::TestGMPVideoDecoder::*)(GMPVideoDecoderProxy *,GMPVideoHost *),1,GMPVideoDecoderProxy *,GMPVideoHost *>::Run() Line 827	C++
nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 846	C++
NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 265	C++

Comment 1

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

Attachment: Patch

Don't assert we were successful unless we were actually successful...

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8634466 [details] [diff] [review]
Patch

Comment should be moved with the assert as well.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f81abae3fcea

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f81abae3fcea

Comment 5

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

Comment on attachment 8634466 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: EME/OpenH264
[User impact if declined]: None. This path moves an assertion so that we only assert that a function's output is valid if the function succceeded. This assertion is failing a lot, which is making verifying other EME fixes hard on Aurora/Beta builds.
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.
[Risks and why]: Low. It's just moving an assert around.
[String/UUID change made/needed]:  None.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8634466 [details] [diff] [review]
Patch

Approving for uplift to Aurora as its a simple fix.

Comment 7

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/57564319395c

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8634466 [details] [diff] [review]
Patch

Given the simplicity of this change and that it will help debug failures, I think it's reasonable to take this fix in beta 6. Beta+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9190dedf78a2