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

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

As Bug 1404230 comment 31, we should reject the promise for avoiding the permanent waiting promise.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Review commit: https://reviewboard.mozilla.org/r/196116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196116/

Comment 2

[James Cheng[:JamesCheng]]

(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.

Comment 3

[James Cheng[:JamesCheng]]

Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196116/diff/1-2/

Comment 4

[James Cheng[:JamesCheng]]

Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196116/diff/2-3/

Comment 5

[James Cheng[:JamesCheng]]

Bug 1404230 is postponed so I removed the related code.

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 8

[James Cheng[:JamesCheng]]

Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196116/diff/3-4/

Comment 9

[James Cheng[:JamesCheng]]

Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196116/diff/4-5/

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

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

Thanks.

Comment 11

[James Cheng[:JamesCheng]]

Comment on attachment 8924863 [details]
Bug 1413480 - Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196116/diff/5-6/

Comment 12

[James Cheng[:JamesCheng]]

Remove dependency from Bug 1404230 .

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b0b68c54b54
Check mCDM in ChromiumCDMChild::RecvXXX before dereferencing it. r=cpearce,kikuo

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4b0b68c54b54