Closed
Bug 1358373
Opened 7 years ago
Closed 7 years ago
Assertion in ChromiumCDMChild::Allocate due lack of appropriately-sized shmen
Categories
(Core :: Audio/Video: GMP, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mozbugz, Assigned: cpearce)
References
Details
Attachments
(1 file)
With a local build, on Mac. STR: 1. Navigate to https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/eme/ 2. In the left column next to the paused video, select "Big Buck Bunny (PlayReady/Widevine)" Expected: Video plays Actual: "Video can't be played because the file is corrupt", and drop-down "The WidevineCdm plugin has crashed" In the launch console, I see the following stack trace: > Assertion failure: false, at dom/media/gmp/ChromiumCDMChild.cpp:147 > #01: mozilla::gmp::ChromiumCDMChild::Allocate(unsigned int)[XUL +0x3cd257e] > #02: non-virtual thunk to mozilla::gmp::ChromiumCDMChild::Allocate(unsigned int)[XUL +0x3cd283f] > #03: ???[gmp-widevinecdm/1.4.8.903/libwidevinecdm.dylib +0x5982] > #04: GetCdmVersion[gmp-widevinecdm/1.4.8.903/libwidevinecdm.dylib +0x135503] > #05: GetCdmVersion[gmp-widevinecdm/1.4.8.903/libwidevinecdm.dylib +0x135386] > #06: GetCdmVersion[gmp-widevinecdm/1.4.8.903/libwidevinecdm.dylib +0x1351ed] > #07: GetCdmVersion[gmp-widevinecdm/1.4.8.903/libwidevinecdm.dylib +0x20c3e] > #08: GetCdmVersion[gmp-widevinecdm/1.4.8.903/libwidevinecdm.dylib +0x1f7a4] > #09: mozilla::gmp::ChromiumCDMChild::RecvDecryptAndDecodeFrame(mozilla::gmp::CDMInputBuffer const&)[XUL +0x3cd5ed7] > #10: mozilla::gmp::PChromiumCDMChild::OnMessageReceived(IPC::Message const&)[XUL +0x1266b6c] > #11: mozilla::gmp::PGMPContentChild::OnMessageReceived(IPC::Message const&)[XUL +0xd64c14] > #12: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)[XUL +0xc45f73] > #13: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)[XUL +0xc4421b] > #14: mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)[XUL +0xc45186] > #15: mozilla::ipc::MessageChannel::MessageTask::Run()[XUL +0xc458c8] > #16: MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>)[XUL +0xb2ff82] > #17: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&)[XUL +0xb308e9] > #18: MessageLoop::DoWork()[XUL +0xb30f94] > #19: base::MessagePumpDefault::Run(base::MessagePump::Delegate*)[XUL +0xb32354] > #20: MessageLoop::RunInternal()[XUL +0xb2fce5] > #21: MessageLoop::RunHandler()[XUL +0xb2fc45] > #22: MessageLoop::Run()[XUL +0xb2fbed] > #23: XRE_InitChildProcess(int, char**, XREChildData const*)[XUL +0x7206797] > #24: mozilla::BootstrapImpl::XRE_InitChildProcess(int, char**, XREChildData const*)[XUL +0x7214967] > #25: content_process_main(mozilla::Bootstrap*, int, char**)[plugin-container +0x142a] > #26: main[plugin-container +0x14e5] The assertion has the following comment above it: > // The parent process should have bestowed upon us a shmem of appropriate > // size, but did not! > MOZ_ASSERT(false);
Updated•7 years ago
|
Severity: normal → major
Rank: 15
Priority: -- → P1
Assignee | ||
Comment 1•7 years ago
|
||
I can repro on Nightly Win64, and I've also seen this on Lightbox.co.nz. Increasing media.eme.chromium-api.video-shmems fixes it, though it would be nice if we could have a way to recover from not having enough shmems instead of crashing.
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•7 years ago
|
||
Chris, you've attached your patch to this duplicate bug instead of bug 1359621. I think you'll need to either reopen this one, or move your patches to the other one.
Assignee: nobody → cpearce
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8861762 [details] Bug 1358373 - Handle underestimating how many shmems the CDM needs to return decoded video frames to Gecko. https://reviewboard.mozilla.org/r/133690/#review136690 r+ with nits/suggestions. If you move patches to another bug, I'll rubber-stamp them there... ::: dom/media/gmp/ChromiumCDMChild.cpp:64 (Diff revision 1) > } > > + CDMShmemBuffer(ChromiumCDMChild* aProtocol, > + ipc::Shmem aShmem, > + WidevineBuffer* aLocalBuffer) > + : CDMShmemBuffer(aProtocol, Move(aShmem)) `Move` doesn't hurt, but is useless here, as the other constructor takes a Shmem by value anyway. Alternatively, you could always take the Shmem by rref, and change all the calls to use Move. ::: dom/media/gmp/ChromiumCDMChild.cpp:586 (Diff revision 1) > output.DecryptedBuffer() > ? reinterpret_cast<CDMShmemBuffer*>(output.DecryptedBuffer()) > : nullptr; Just noticed this reinterpret_cast, please change to static_cast (more details in next issue). And you should fire your previous reviewer for this glaring omission! And while we're here: Since you've modified the Allocate function, couldn't mCDM->Decrypt() also return a WidevineBuffer? In which case you'll need to modify this code to extract and send the correct stuff. Though I'm guessing the parent should have sent the correct shmem already (right?), so it should only be a CDMShmemBuffer here; in which case I'd suggest adding a `MOZ_RELEASE_ASSERT(mType==SHARED)` (or a friendlier failure) for peace of mind. ::: dom/media/gmp/ChromiumCDMChild.cpp:737 (Diff revision 1) > uint64_t duration = 0; > if (mFrameDurations.Find(aFrame.Timestamp(), duration)) { > output.mDuration() = duration; > } > > - Unused << SendDecoded(output); > + CDMBuffer* base = reinterpret_cast<CDMBuffer*>(aFrame.FrameBuffer()); You must use static_cast when downcasting pointers within a class hierarchy, as it could actually move the pointer! (Maybe not in this particular case, but reinterpret_cast is still incorrect and a potential timebomb.) Same with the other two below. ::: dom/media/gmp/ChromiumCDMChild.cpp:737 (Diff revision 1) > - Unused << SendDecoded(output); > + CDMBuffer* base = reinterpret_cast<CDMBuffer*>(aFrame.FrameBuffer()); > + if (base->mType == CDMBuffer::SHARED) { > + CDMShmemBuffer* buffer = > + reinterpret_cast<CDMShmemBuffer*>(aFrame.FrameBuffer()); > + Unused << SendDecodedShmem(output, buffer->ExtractShmem()); > + } else { > + WidevineBuffer* buffer = > + reinterpret_cast<WidevineBuffer*>(aFrame.FrameBuffer()); > + Unused << SendDecodedData(output, buffer->ExtractBuffer()); > + } Probably over-engineering here, but I feel compelled to suggest the OO option: Instead of a public mType, CDMBuffer could have a `virtual SendDecoded(PChromiumCDMChild&, const CDMVideoFrame&) = 0`, and the overrides in CDMShmemBuffer and WidevineBuffer would just make the correct SendDecodedXXX call. (But up to you, you can ignore this if you're busier than usual!) ::: dom/media/gmp/ChromiumCDMParent.h:166 (Diff revision 1) > > int32_t mVideoFrameBufferSize = 0; > > + // Count of the number of shmems in the set used to return decoded video > + // frames from the CDM to Gecko. > + uint32_t mVideoShmemCount = 0; No need to initialize it here, as you do it in the only constructor. ::: dom/media/gmp/ChromiumCDMParent.cpp:846 (Diff revision 1) > - // we've seen cases where it allocates two buffers, returns one and holds > - // onto the other. So the minimum number of shmems we give to the CDM > - // must be at least two, and the default is three for safety. > + // we've seen cases where it allocates four buffers, returns one and holds > + // onto the rest. So the minimum number of shmems we give to the CDM > + // should be at least four. Instead of saying "two" or "four" (which if unlucky we may need to adjust again), just say "multiple", and that the minimum comes from a pref.
Attachment #8861762 -
Flags: review?(gsquelart) → review+
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861762 [details] Bug 1358373 - Handle underestimating how many shmems the CDM needs to return decoded video frames to Gecko. https://reviewboard.mozilla.org/r/133690/#review136690 > Just noticed this reinterpret_cast, please change to static_cast (more details in next issue). > And you should fire your previous reviewer for this glaring omission! > > And while we're here: > Since you've modified the Allocate function, couldn't mCDM->Decrypt() also return a WidevineBuffer? In which case you'll need to modify this code to extract and send the correct stuff. > Though I'm guessing the parent should have sent the correct shmem already (right?), so it should only be a CDMShmemBuffer here; in which case I'd suggest adding a `MOZ_RELEASE_ASSERT(mType==SHARED)` (or a friendlier failure) for peace of mind. Thanks for pointing out the casting issue. I'll assert the type is shared. We also assert at the start of this function MOZ_ASSERT(HasShmemOfSize(outputShmemSize)), as the parent is indeed supposed to provide the required shmem before calling us. > Probably over-engineering here, but I feel compelled to suggest the OO option: > > Instead of a public mType, CDMBuffer could have a `virtual SendDecoded(PChromiumCDMChild&, const CDMVideoFrame&) = 0`, and the overrides in CDMShmemBuffer and WidevineBuffer would just make the correct SendDecodedXXX call. > > (But up to you, you can ignore this if you're busier than usual!) I had a go at this, and adding a virtual send to the buffer class breaks the encapsulation of the buffer class only being about holding a buffer. Also it means that we need to expose the ChromiumCDMChild to the buffer types, the send functions are also private, so we need trampolines for the child's send functions too. So I went with eliminating most of the casts by having As$Type() functions on the base class, with the derived class implementing the appropriate ones.
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Gerald Squelart [:gerald] from comment #4) > Chris, you've attached your patch to this duplicate bug instead of bug > 1359621. I think you'll need to either reopen this one, or move your patches > to the other one. Thanks, yes I will re-open this one since the review is here.
Assignee | ||
Updated•7 years ago
|
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: DUPLICATE → ---
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861762 [details] Bug 1358373 - Handle underestimating how many shmems the CDM needs to return decoded video frames to Gecko. https://reviewboard.mozilla.org/r/133690/#review136690 > I had a go at this, and adding a virtual send to the buffer class breaks the encapsulation of the buffer class only being about holding a buffer. Also it means that we need to expose the ChromiumCDMChild to the buffer types, the send functions are also private, so we need trampolines for the child's send functions too. > > So I went with eliminating most of the casts by having As$Type() functions on the base class, with the derived class implementing the appropriate ones. Looking good, thank you for implementing this.
Comment 13•7 years ago
|
||
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0b8bf5cb743f Handle underestimating how many shmems the CDM needs to return decoded video frames to Gecko. r=gerald
Comment 14•7 years ago
|
||
backed this out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=94690016&repo=autoland&lineNumber=15459
Flags: needinfo?(cpearce)
Comment 15•7 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/68532b7b6ca5 Backed out changeset 0b8bf5cb743f for build bustage. a=backout
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2dfacbfaeda5 Handle underestimating how many shmems the CDM needs to return decoded video frames to Gecko. r=gerald
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Iris Hsiao [:ihsiao] from comment #14) > backed this out for build bustage like > https://treeherder.mozilla.org/logviewer. > html#?job_id=94690016&repo=autoland&lineNumber=15459 Thank you for the backout and need-info. I forgot to update before pushing.
Flags: needinfo?(cpearce)
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2dfacbfaeda5
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Our Coverity analysis pointed out a small problem in this assertion: http://searchfox.org/mozilla-central/rev/38fdbbcaa4be3a3f5b0d207dc5d53fb687c42d6a/dom/media/gmp/ChromiumCDMChild.cpp#606 Seems like it should be MOZ_ASSERT_IF(buffer, buffer->AsShmemBuffer());
Flags: needinfo?(cpearce)
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #20) > Our Coverity analysis pointed out a small problem in this assertion: > http://searchfox.org/mozilla-central/rev/ > 38fdbbcaa4be3a3f5b0d207dc5d53fb687c42d6a/dom/media/gmp/ChromiumCDMChild. > cpp#606 > > Seems like it should be MOZ_ASSERT_IF(buffer, buffer->AsShmemBuffer()); I've filed bug 1360959 to address this. Thanks Bill!
Flags: needinfo?(cpearce)
You need to log in
before you can comment on or make changes to this bug.
Description
•