Closed Bug 1173195 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

(Keywords: assertion)

Attachments

(1 file)

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++
Keywords: assertion
Priority: -- → P1
Attached patch PatchSplinter Review
Don't assert we were successful unless we were actually successful...
Assignee: edwin → cpearce
Status: NEW → ASSIGNED
Attachment #8634466 - Flags: review?(edwin)
Comment on attachment 8634466 [details] [diff] [review]
Patch

Comment should be moved with the assert as well.
Attachment #8634466 - Flags: review?(edwin) → review+
https://hg.mozilla.org/mozilla-central/rev/f81abae3fcea
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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.
Attachment #8634466 - Flags: approval-mozilla-beta?
Attachment #8634466 - Flags: approval-mozilla-aurora?
Comment on attachment 8634466 [details] [diff] [review]
Patch

Approving for uplift to Aurora as its a simple fix.
Attachment #8634466 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(cpearce)
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+
Attachment #8634466 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.