Closed Bug 1358373 Opened 3 years ago Closed 3 years ago

Assertion in ChromiumCDMChild::Allocate due lack of appropriately-sized shmen

Categories

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

55 Branch
x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gerald, 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);
Severity: normal → major
Rank: 15
Priority: -- → P1
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1359621
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
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+
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.
(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.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: DUPLICATE → ---
Duplicate of this bug: 1359621
Duplicate of this bug: 1356378
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.
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
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68532b7b6ca5
Backed out changeset 0b8bf5cb743f for build bustage. a=backout
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
(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)
https://hg.mozilla.org/mozilla-central/rev/2dfacbfaeda5
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago3 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)
Depends on: 1360959
(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)
Depends on: 1366639
You need to log in before you can comment on or make changes to this bug.