1130932 - [EME] Be more lenient in handling mIsOpen in GMPDecryptorParent::RecvXXX

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

When running test_eme_canvas_blocked.html, chances are the test case finish and call GMPDecryptorParent::Close() before GMPDecryptorParent::RecvKeyStatusChanged() arrives. Since this is a valid case, we should return true in GMPDecryptorParent::RecvXXX on such conditions to avoid IPDL errors.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1130932_allow_recv_calls_after_close-v1.patch

Don't return false in GMPDecryptorParent::RecvXXX functions.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8561213 [details] [diff] [review]
1130932_allow_recv_calls_after_close-v1.patch

Review of attachment 8561213 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should treat this as valid. The CDM should not emit any messages about a session after it has sent SessionClosed. (yes, this needs to be fixed in our ClearKey CDM; on my todo list).

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8561213 [details] [diff] [review]
1130932_allow_recv_calls_after_close-v1.patch

Review of attachment 8561213 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, looking again at the parent-side code, looks like we close immediately after sending DecryptionComplete. Lame.

r+, but reduce to only the messages we're likely to receive at shutdown.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

It is kinda hard to identify the messages we could receive during shutdown since you can close the page to trigger GMPDecryptorParent::Close() at any time.

For now I will leave GMPDecryptorParent::RecvKeyStatusChanged() only which should fix most problems we are facing in the test cases. In the long term, I think GMPDecryptorParent should wait for some response from the child after sending SendDecryptingComplete() to ensure shutdown sequence is done in a controlled way.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1130932_allow_recv_calls_after_close-v2.patch

address comment 3.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

try: https://tbpl.mozilla.org/?tree=Try&rev=6242cf850a2f

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=854eae361f01

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f2ea7b6061f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f2ea7b6061f

Comment 11

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

https://hg.mozilla.org/releases/mozilla-beta/rev/5ee41a13b1ee

Comment 12

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

Attachment: Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 13

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

Comment on attachment 8572364 [details] [diff] [review]
Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8572364 [details] [diff] [review]
Beta patch

Approved for Beta as part of EME platform uplift.