[EME] Implement persistent sessions in gmp-clearkey

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 2 bugs)

unspecified
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Depends on: 1109861
(Note: I accidentally landed https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddb36faf9ee labelled as bug 1109457 instead of bug 1101328)
Keywords: leave-open
Created attachment 8534866 [details] [diff] [review]
WIP Patch

Work in progress patch...
https://hg.mozilla.org/mozilla-central/rev/5ddb36faf9ee
Created attachment 8535381 [details] [diff] [review]
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
Attachment #8534866 - Attachment is obsolete: true
Attachment #8535381 - Flags: review?(edwin)
Attachment #8535381 - Flags: feedback?(jwwang)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bebe9720e24e
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 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?
Attachment #8535381 - Flags: feedback?(jwwang) → feedback+
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?
Attachment #8535381 - Flags: review?(edwin) → review+
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.
Review comments addressed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9ba96e7f107
Static analysis and double-free fixed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=addbd635ea4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6994acf4e3
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
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.
Created attachment 8538236 [details] [diff] [review]
Patch: Fix crash...

Applies on top of patch 1, correct insertions, deletions and checks whether a decryptor is in ClearKeyDecryptionManager::mDecryptors.
Attachment #8538236 - Flags: review?(edwin)
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...
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a64ee59a883
Attachment #8538236 - Flags: review?(edwin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ba2f134695
https://hg.mozilla.org/integration/mozilla-inbound/rev/70428bb355a4
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
It was greener at least. One more potential fix:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dd19eb0f2e2a
WAE makes the fox sad:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=00c6d0dc7eca
Created attachment 8538678 [details] [diff] [review]
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.
Attachment #8538678 - Flags: review?(edwin)
Attachment #8538678 - Flags: review?(edwin) → review+
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
https://hg.mozilla.org/mozilla-central/rev/ce51003483e4
https://hg.mozilla.org/mozilla-central/rev/5f7811bf654f
https://hg.mozilla.org/mozilla-central/rev/0f8c14c2cc13
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Mass update firefox-status to track EME uplift.
status-firefox36: --- → fixed
status-firefox37: --- → fixed
status-firefox38: --- → fixed
You need to log in before you can comment on or make changes to this bug.