Closed Bug 1413480 Opened 2 years ago Closed 2 years ago

[EME] Should check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Categories

(Core :: Audio/Video: GMP, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(1 file)

As Bug 1404230 comment 31, we should reject the promise for avoiding the permanent waiting promise.
Assignee: nobody → jacheng
Attachment #8924863 - Flags: review?(kikuo)
(In reply to James Cheng[:JamesCheng] from comment #0)
> As Bug 1404230 comment 31, we should reject the promise for avoiding the
> permanent waiting promise.

http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/media/gmp/ChromiumCDMChild.cpp#912

I'm not sure that we will receive any ipc RecvXXX after we called Send__delete__(this);.

If it really did call the RecvXXX after that, we should reject the promise as this patch did.
Bug 1404230 is postponed so I removed the related code.
Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

https://reviewboard.mozilla.org/r/196116/#review201786

ChromiumCDMChild sets mCDM to nullptr only when ChromiumCDMChild::RecvDestroy is called.
And, correct me if I'm wrong, that would be triggered only after ChromiumCDMParent::Shutdown is called.

For me, I think adding MOZ_ASSERT(mCDM) in every ChromiumCDMChild::RecvXXX and rejecting those promises in ChromiumCDMParent::Shutdown would be a better solution as I could think of a case that there are still in-processing requests after mCDM is set to null.
Attachment #8924863 - Flags: review?(kikuo) → review+
Attachment #8924863 - Flags: review?(cpearce)
Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

https://reviewboard.mozilla.org/r/196116/#review202486

::: dom/media/gmp/ChromiumCDMChild.cpp:626
(Diff revision 3)
>  ChromiumCDMChild::RecvCloseSession(const uint32_t& aPromiseId,
>                                     const nsCString& aSessionId)
>  {
>    MOZ_ASSERT(IsOnMessageLoopThread());
> +  if (!mCDM) {
> +    SendOnRejectPromise(aPromiseId,

Rejecting the promises here shouldn't be necessary; when the MediaKeys shuts down, it rejects any pending promises:

https://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/eme/MediaKeys.cpp#105

Please just ensure we null check mCDM before dereferencing it.
Attachment #8924863 - Flags: review?(cpearce) → review-
Summary: [EME] Should reject the promise when mCDM is null in ChromiumCDMChild::RecvXXX → [EME] Should check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.
Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

https://reviewboard.mozilla.org/r/196116/#review202980

Thanks.
Attachment #8924863 - Flags: review?(cpearce) → review+
Remove dependency from Bug 1404230 .
No longer depends on: 1404230
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b0b68c54b54
Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it. r=cpearce,kikuo
https://hg.mozilla.org/mozilla-central/rev/4b0b68c54b54
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.