Closed Bug 1109457 Opened 5 years ago Closed 5 years ago

[EME] Implement persistent sessions in gmp-clearkey

Categories

(Core :: Audio/Video, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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
Attached patch WIP Patch (obsolete) — Splinter Review
Work in progress patch...
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)
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.
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.
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...
It was greener at least. One more potential fix:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dd19eb0f2e2a
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)
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.