1109457 - [EME] Implement persistent sessions in gmp-clearkey

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We need to implement persistent sessions for our gmp-clearkey plugin so that we can ensure that we don't regress the infrastructure that makes it work, as it's an important usecase for DRM systems.

Comment 1

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

(Note: I accidentally landed https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddb36faf9ee labelled as bug 1109457 instead of bug 1101328)

Comment 2

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

Attachment: WIP Patch

Work in progress patch...

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ddb36faf9ee

Comment 4

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

Attachment: Patch v1: Add persistent sessions to gmp-clearkey

Add persistent session to gmp-clearkey. I tried to separate out the stuff related to persistent sessions into other files where it made sense, to reduce bitrotting bug 1075199 where possible.

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

Comment 5

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

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

Comment 6

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

Comment on attachment 8535381 [details] [diff] [review]
Patch v1: Add persistent sessions to gmp-clearkey

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

::: media/gmp-clearkey/0.1/ClearKeyPersistence.cpp
@@ +41,5 @@
> +    // the persistent session ids.
> +    const char* name = nullptr;
> +    uint32_t len = 0;
> +    while (GMP_SUCCEEDED(aRecordIterator->GetName(&name, &len))) {
> +      if (!ClearKeyUtils::IsValidSessionId(name, len)) {

Err, this needs to be:

if (ClearKeyUtils::IsValidSessionId(name, len)) {
  MOZ_ASSERT(name[len] == 0);
  sPersistentSessionIds.insert(atoi(name));
}
aRecordIterator->NextRecord();

Else if there are non-valid session Ids we won't increment the iterator, and we'll get stuck in an infinite loop... d'oh!

Comment 7

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

Comment on attachment 8535381 [details] [diff] [review]
Patch v1: Add persistent sessions to gmp-clearkey

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

Overall looks fine with some nits.

::: dom/media/test/eme.js
@@ +9,3 @@
>      ok(false, message);
>      if (err) {
> +      info(String(err));

Do we need to stringify |err|? info should also accept integers.

@@ +219,5 @@
>          Log(token, "created MediaKeys object ok");
>          mediaKeys.sessions = [];
>          return v.setMediaKeys(mediaKeys);
>        }, bail("failed to create MediaKeys object"))
>        

space

@@ +232,3 @@
>          return session.generateRequest(ev.initDataType, ev.initData);
>        }, onSetKeysFail)
>        

space

::: dom/media/test/manifest.js
@@ +649,5 @@
>      },
>      sessionType:"temporary",
>      duration:0.47
>    },
> +/*  {

Can you explain why this file is removed from the test?

::: dom/media/test/test_eme_persistent_sessions.html
@@ +16,5 @@
> +function AllKeyIdsUsable(usableKeyIds, expectedKeyIds) {
> +  for (var i = 0; i < usableKeyIds.length; i++) {
> +    var keyId = usableKeyIds[i];
> +    var kid = Base64ToHex(window.btoa(ArrayBufferToString(keyId)));
> +    if (!(kid in expectedKeyIds)) {

It looks like we should return true when usableKeyIds is a super set of expectedKeyIds (ie, all keys of expectedKeyIds are in usableKeyIds).

@@ +60,5 @@
> +
> +  var recreatedSession; // will have remove() called on it.
> +
> +  var keySystemAccess;
> +  

space

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +85,4 @@
>  ClearKeyDecryptionManager::ClearKeyDecryptionManager()
>  {
>    CK_LOGD("ClearKeyDecryptionManager ctor");
> +  AddRef();

Why does the ref count start with 1 before it is hold by any RefPtr? Shouldn't it start with 0 and become 1 when passed to a RefPtr?

@@ +285,5 @@
> +  // and simply append each keyId followed by its key.
> +  vector<uint8_t> keydata;
> +  Serialize(session, keydata);
> +  GMPTask* resolve = WrapTask(mCallback, &GMPDecryptorCallback::ResolvePromise, aPromiseId);
> +  static const char message[] = "Couldn't store cenc key init data";

Why not making message a const char*?

@@ +322,5 @@
>    CK_LOGD("ClearKeyDecryptionManager::CloseSession");
>  
>    string sessionId(aSessionId, aSessionId + aSessionIdLength);
> +  auto itr = mSessions.find(sessionId);
> +  if (itr == mSessions.end()) {

Should we reject the promise when session not found?

@@ +382,5 @@
> +  ClearKeyPersistence::PersistentSessionRemoved(sid);
> +
> +  // Overwrite the record storing the sessionId's key data with a zero
> +  // length record to delete it.
> +  vector<uint8_t> keydata;

emptyKeyData should better suggest the purpose.

@@ +384,5 @@
> +  // Overwrite the record storing the sessionId's key data with a zero
> +  // length record to delete it.
> +  vector<uint8_t> keydata;
> +  GMPTask* resolve = WrapTask(mCallback, &GMPDecryptorCallback::ResolvePromise, aPromiseId);
> +  static const char message[] = "Could not remove session";

Why not a const char*?

::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp
@@ +13,5 @@
> +
> +#include <vector>
> +
> +GMPErr
> +RunOnMainThread(GMPTask* aTask)

Add the static modifier since this function is local to this file only.

@@ +86,5 @@
> +{
> +  GMPRecord* record;
> +  WriteRecordClient* client = new WriteRecordClient();
> +  if (GMP_FAILED(OpenRecord(aRecordName.c_str(),
> +                               aRecordName.size(),

indentation

@@ +134,5 @@
> +  }
> +
> +private:
> +  GMPRecord* mRecord;
> +  ReadContinuation* mContinuation;

mContinuation is not deleted here. Is the user responsible for deleting it? It is inconsistent with StoreData which is responsible for deleting aOnSuccess passed by the user.

@@ +145,5 @@
> +  MOZ_ASSERT(aContinuation);
> +  GMPRecord* record;
> +  ReadRecordClient* client = new ReadRecordClient();
> +  auto err = OpenRecord(aRecordName.c_str(),
> +                           aRecordName.size(),

indentation

::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp
@@ +568,5 @@
> +
> +/* static */ bool
> +ClearKeyUtils::IsValidSessionId(const char* aBuff, uint32_t aLength)
> +{
> +  if (aLength > sizeof(uint32_t)) {

sizeof(uint32_t) is 4. So a valid session id is at most 4 character long?

::: media/gmp-clearkey/0.1/ClearKeyUtils.h
@@ +57,5 @@
>  };
>  
> +template<class Container, class Element>
> +inline bool
> +Contains(const Container& aContainer, Element aElement)

Why don't we pass a const reference of aElement?

Comment 8

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

Comment on attachment 8535381 [details] [diff] [review]
Patch v1: Add persistent sessions to gmp-clearkey

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

lgtm after jw's and my comments.

::: dom/media/test/manifest.js
@@ +649,5 @@
>      },
>      sessionType:"temporary",
>      duration:0.47
>    },
> +/*  {

Yeah -- nice try ;)

::: dom/media/test/test_eme_persistent_sessions.html
@@ +8,5 @@
> +  <script type="text/javascript" src="eme.js"></script>
> +</head>
> +<body>
> +<pre id="test">
> +<div id="cpearce_log"></div>

Remove

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +363,5 @@
> +  string sessionId(aSessionId, aSessionId + aSessionIdLength);
> +  auto itr = mSessions.find(sessionId);
> +  if (itr == mSessions.end()) {
> +    // Do we have to reject promise?
> +    // Can't we assume the session exists?

Eh, better to be defensive. We should do *something* here -- resolve, reject, and/or assert.

::: media/gmp-clearkey/0.1/ClearKeyPersistence.h
@@ +32,5 @@
> +  static bool LoadSessionData(ClearKeyDecryptionManager* aInstance,
> +                              const std::string& aSid,
> +                              uint32_t aPromiseId);
> +
> +  static void PersistentSessionRemoved(const std::string aSid);

Should be string ref?

::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp
@@ +42,5 @@
> +  }
> +
> +  virtual void ReadComplete(GMPErr aStatus,
> +                            const uint8_t* aData,
> +                            uint32_t aDataSize) MOZ_OVERRIDE {}

Assert not reached?

@@ +129,5 @@
> +    mContinuation->ReadComplete(GMPNoErr, aData, aDataSize);
> +    delete this;
> +  }
> +
> +  virtual void WriteComplete(GMPErr aStatus) MOZ_OVERRIDE {

Assert that this shouldn't be reached?

::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp
@@ +561,5 @@
> +{
> +  switch (aSessionType) {
> +    case kGMPTemporySession: return "temporary";
> +    case kGMPPersistentSession: return "persistent";
> +    default: return "invalid";

Maybe assert not reached here?

Comment 9

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

Comment on attachment 8535381 [details] [diff] [review]
Patch v1: Add persistent sessions to gmp-clearkey

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

Thanks for your comments JW!

::: dom/media/test/eme.js
@@ +9,3 @@
>      ok(false, message);
>      if (err) {
> +      info(String(err));

Yeah, I discovered while helping a partner debug that we could get a DOMError here, and that doesn't implicitly stringify, it has to be explicitly stringified like this.

::: dom/media/test/test_eme_persistent_sessions.html
@@ +16,5 @@
> +function AllKeyIdsUsable(usableKeyIds, expectedKeyIds) {
> +  for (var i = 0; i < usableKeyIds.length; i++) {
> +    var keyId = usableKeyIds[i];
> +    var kid = Base64ToHex(window.btoa(ArrayBufferToString(keyId)));
> +    if (!(kid in expectedKeyIds)) {

We don't want usableKeyIds to be a superset of expectedKeyIds, as then usableKeyIds would contain unexpected keyIds... We really want usableKeyIds to be exactly the expectedKeyIds. We can do that like so:

function UsableKeyIdsMatch(usableKeyIds, expectedKeyIds) {
  var hexKeyIds = usableKeyIds.map(function(keyId) {
    return Base64ToHex(window.btoa(ArrayBufferToString(keyId)));
  }).sort();

  var expected = Object.keys(expectedKeyIds).sort();
  
  if (expected.length != hexKeyIds.length) {
    return false;
  }
  
  for (var i = 0; i < hexKeyIds.length; i++) {
    if (hexKeyIds[i] != expected[i]){
      return false;
    }
  }
  return true;
}

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +85,4 @@
>  ClearKeyDecryptionManager::ClearKeyDecryptionManager()
>  {
>    CK_LOGD("ClearKeyDecryptionManager ctor");
> +  AddRef();

The ClearKeyDecryptionManager maintains a self reference which is removed when the host is finished with the interface and calls DecryptingComplete(). We make ClearKeyDecryptionManager refcounted so that the tasks to that we dispatch to call functions on it won't end up derefing a null reference. Previously we deleted the ClearKeyDecryptionManager in DecryptingComplete(), which could cause a crash.

@@ +322,5 @@
>    CK_LOGD("ClearKeyDecryptionManager::CloseSession");
>  
>    string sessionId(aSessionId, aSessionId + aSessionIdLength);
> +  auto itr = mSessions.find(sessionId);
> +  if (itr == mSessions.end()) {

Won't hurt. Gecko is not supposed to call the GMP to close an already-closed session, but it doesn't hurt to be careful.

::: media/gmp-clearkey/0.1/ClearKeyStorage.cpp
@@ +134,5 @@
> +  }
> +
> +private:
> +  GMPRecord* mRecord;
> +  ReadContinuation* mContinuation;

Good catch. ReadData() should be responsible for deleting it. I will make it always call the continuation to report error, and delete the continuation.

::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp
@@ +568,5 @@
> +
> +/* static */ bool
> +ClearKeyUtils::IsValidSessionId(const char* aBuff, uint32_t aLength)
> +{
> +  if (aLength > sizeof(uint32_t)) {

Oh oops, Good catch. I was going to store the session id as uint32_t. Got my wires crossed.

A sensible value for this is 10, which is the maximum number of characters possible that a stringified uint32_t can have. Will make that change, with a comment.

Comment 10

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

Review comments addressed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9ba96e7f107

Comment 11

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

Static analysis and double-free fixed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=addbd635ea4e

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6994acf4e3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for EME failures and crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0d52c8f679

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4749744&repo=mozilla-inbound

Comment 14

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

Looks like the problem is in the loop in ClearKeyDecryptionManager::UpdateSession():


  for (auto it = keyPairs.begin(); it != keyPairs.end(); it++) {
    KeyId& keyId = it->mKeyId;

    if (mDecryptors.find(keyId) != mDecryptors.end()) {
      mDecryptors[keyId] = new ClearKeyDecryptor(mCallback, it->mKey);
      mCallback->KeyIdUsable(aSessionId, aSessionIdLength,
                             &keyId[0], keyId.size());
    }

    mDecryptors[keyId]->AddRef();
  }


The problem is the "if (mDecryptors.find(keyId) != mDecryptors.end())" condition; that returns true if mDecryptors does contain the keyId, and then in the body of the if statement, we override mDecryptors[keyId] with a new instance. This mostly works since in ClearKeyDecryptionManager::CreateSession we insert a null pointer into mDecryptors[keyId]. However I think that since we insert nullptr into mDecryptors, we can hit the nullptr when iterating over the decryptors in ClearKeyDecryptionManager::ClearInMemorySessionData, which I think causes the crash we're seeing.

Comment 15

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

Attachment: Patch: Fix crash...

Applies on top of patch 1, correct insertions, deletions and checks whether a decryptor is in ClearKeyDecryptionManager::mDecryptors.

Comment 16

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

I can still crash gmp-clearkey when looping over the EME mochitests, but it takes a lot longer to crash... So the worst problem is fixed by this patch at least...

Comment 17

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a64ee59a883

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ba2f134695
https://hg.mozilla.org/integration/mozilla-inbound/rev/70428bb355a4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out (again) for OSX 10.6 failures/crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d01e271d8a

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4754982&repo=mozilla-inbound

Comment 20

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

It was greener at least. One more potential fix:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dd19eb0f2e2a

Comment 21

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

WAE makes the fox sad:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=00c6d0dc7eca

Comment 22

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

Attachment: Patch: fix ClearKeyDecryptor's shutdown of thread

The problem we're hitting is here:

uint32_t
ClearKeyDecryptor::Release()
{
  uint32_t newCount = --mRefCnt;
  if (!newCount) {
    if (mThread) {
      mThread->Post(new DestroyTask(this));
      mThread->Join();
    } else {
      delete this;
    }
  }

  return newCount;
}

When we Post(new DestroyTask(this)), the task may run before Post() returns, deleting the ClearKeyDecryptor which means, ClearKeyDecryptor::mThread lies in freed memory. On MacOSX the free() implementation must be clear or poison memory when it frees a block, so that's why we're failing on MacOSX.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce51003483e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7811bf654f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8c14c2cc13

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce51003483e4
https://hg.mozilla.org/mozilla-central/rev/5f7811bf654f
https://hg.mozilla.org/mozilla-central/rev/0f8c14c2cc13

Comment 25

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

Mass update firefox-status to track EME uplift.