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

RESOLVED FIXED in Firefox 40

Status

()

P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug, {assertion})

unspecified
mozilla42
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 wontfix, firefox40 fixed, firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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++
Assignee: nobody → edwin
Keywords: assertion
Priority: -- → P1
(Assignee)

Comment 1

3 years ago
Created attachment 8634466 [details] [diff] [review]
Patch

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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Comment 5

3 years ago
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?

Updated

3 years ago
status-firefox41: --- → affected

Comment 6

3 years ago
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+
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/57564319395c
status-firefox41: affected → fixed
(Assignee)

Updated

3 years ago
status-firefox39: --- → wontfix
status-firefox40: --- → affected
(Assignee)

Updated

3 years ago
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+
(Assignee)

Updated

3 years ago
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.