1165163 - [EME] Firefox crash in MediaKeySession.remove() if CDM crashes

Status: RESOLVED FIXED

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

Description

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

If the CDM crashes, and MediaKeySession.remove() is called, we can crash.

For example:
https://crash-stats.mozilla.com/report/index/57febf58-278e-4706-8926-0a8682150515

We don't null check MediaKeySession::mKeys in MediaKeySession.remove(). mKeys is nulled out if the CDM crashes.

Comment 1

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

Attachment: Patch: Don't rely on mediaKeys to create MediaKeySession's promises

In MediaKeySession, instead of relying MediaKeySession::mKeys to create our promise objects, create them directly. This means we can still create promises after our pointer to the MediaKeys has been cleared, so we can always reject our promises.

Comment 2

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

We have identical code in MediaKeys::MakePromise(). Someday we will have MediaKeysUtils to collect common functions.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d148bd91115b

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c63327cac6

Comment 5

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

Attachment: Patch v2: Don't rely on mediaKeys to create MediaKeySession's promises

Update commit message. carry forward r=jwwang

Comment 6

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

Must remember to uplift.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44f38195b432

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44f38195b432

Comment 9

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

Comment on attachment 8606228 [details] [diff] [review]
Patch v2: Don't rely on mediaKeys to create MediaKeySession's promises

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: If the CDM crashes during EME session startup, the browser can crash. Adobe are fixing a particular crash in their CDM that causes us to crash, but we should still not crash on our side.
[Describe test coverage new/current, TreeHerder]: Lots of EME mochitests, though our test CDM doesn't crash in when we exercise the case where Adobe's crashes.
[Risks and why]: Low; basically we just change the parent object that we use to create some promises; pretty safe.
[String/UUID change made/needed]: None.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8606228 [details] [diff] [review]
Patch v2: Don't rely on mediaKeys to create MediaKeySession's promises

Approved for uplift to aurora (40) and beta (39). May help with EME crashes.

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a810a56634b1

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/549b8e7903f5