Changeset 279363 in webkit


Ignore:
Timestamp:
Jun 28, 2021, 11:11:52 PM (4 years ago)
Author:
calvaris@igalia.com
Message:

[GStreamer][EME] Fix resources release when a MediaKeySession is closed
​https://bugs.webkit.org/show_bug.cgi?id=227403

Reviewed by Philippe Normand.

Thunder sessions should be a BoxPtr, already when stored at the
CDMInstanceSessionThunder, it does not make sense to store then in
a unique_ptr. This way the same session lives in the
MediaKeySession wrapper (CDMInstanceSessionThunder) and inside the
KeyStores.

Removed the CDMInstanceProxy key store. It is not needed.

When a session is closed in Thunder, there should be a cascade to
remove it from the other synced stores, that's why we introduce
the removeAllKeysFrom logic.

Regular key stores do not manage key session references
anymore. They are only needed in the CDMProxy class, which is
where the keys are shared among different sessions. We were
managing key session references in other stores and they were
messing up with the key references in the CDMProxy class. In
those cases, a key kept in a local store could have a reference
that would prevent the CDMProxy key store from removing it when
asked from it. There were also cases of removing keys from local
stores that were creating negative reference numbers, which
created the opposite effect, this is, leaving in place keys in the
CDMProxy store that should be released.

  • platform/encryptedmedia/CDMProxy.cpp:

(WebCore::KeyStore::merge): Simplified to just add keys.
(WebCore::KeyStore::add): Adds references (if needed) and merges
if needed.
(WebCore::KeyStore::unrefAllKeys): Renamed. Unrefs all keys from a
store by copying itself and calling unrefAllKeysFrom that copy.
(WebCore::KeyStore::unref): Renamed. Uses proper refefencing.
(WebCore::CDMProxy::unrefAllKeysFrom): Method to unref all keys
that are contained in some other store.
(WebCore::CDMInstanceProxy::mergeKeysFrom): There is no more key
store in this class.
(WebCore::CDMInstanceProxy::unrefAllKeysFrom): Renamed. Calls the
CDMproxy to unref the keys from there.

  • platform/encryptedmedia/CDMProxy.h:

(WebCore::KeyHandle::mergeKeyInto): Merges one key into self by
copying status, data and summing reference count.
(WebCore::KeyHandle::numSessionReferences const): Turn int.
(WebCore::KeyHandle::hasReferences const): Added.
(WebCore::KeyStore::addSessionReferenceTo const):
(WebCore::KeyStore::removeSessionReferenceFrom const): Helpers to
check if the store is reference counted or not. Default is do
nothing and but the ReferenceAwareKeyStore redefines them to do
reference counting.
(WebCore::KeyStore::unrefAllKeys): Deleted.

  • platform/encryptedmedia/clearkey/CDMClearKey.cpp:

(WebCore::CDMInstanceSessionClearKey::updateLicense):
(WebCore::CDMInstanceSessionClearKey::removeSessionData): Use
renamed methods.

  • platform/graphics/gstreamer/eme/CDMThunder.h:
  • platform/graphics/gstreamer/eme/CDMThunder.cpp:

(WebCore::CDMInstanceSessionThunder::status const):
(WebCore::CDMInstanceSessionThunder::keyUpdatedCallback):
(WebCore::CDMInstanceSessionThunder::requestLicense):
(WebCore::CDMInstanceSessionThunder::updateLicense):
(WebCore::CDMInstanceSessionThunder::removeSessionData):
(WebCore::CDMInstanceSessionThunder::loadSession): Use BoxPtr in
the session.
(WebCore::CDMInstanceSessionThunder::closeSession): Close the
current session, release the BoxPtr, notify the instance and
therefore the proxy to unref all they key IDs in this session and
empty the local key store.

Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r279358 r279363  
     12021-06-28  Xabier Rodriguez Calvar  <calvaris@igalia.com>
     2
     3        [GStreamer][EME] Fix resources release when a MediaKeySession is closed
     4        https://bugs.webkit.org/show_bug.cgi?id=227403
     5
     6        Reviewed by Philippe Normand.
     7
     8        Thunder sessions should be a BoxPtr, already when stored at the
     9        CDMInstanceSessionThunder, it does not make sense to store then in
     10        a unique_ptr. This way the same session lives in the
     11        MediaKeySession wrapper (CDMInstanceSessionThunder) and inside the
     12        KeyStores.
     13
     14        Removed the CDMInstanceProxy key store. It is not needed.
     15
     16        When a session is closed in Thunder, there should be a cascade to
     17        remove it from the other synced stores, that's why we introduce
     18        the removeAllKeysFrom logic.
     19
     20        Regular key stores do not manage key session references
     21        anymore. They are only needed in the CDMProxy class, which is
     22        where the keys are shared among different sessions. We were
     23        managing key session references in other stores and they were
     24        messing up with the key references in the CDMProxy class. In
     25        those cases, a key kept in a local store could have a reference
     26        that would prevent the CDMProxy key store from removing it when
     27        asked from it. There were also cases of removing keys from local
     28        stores that were creating negative reference numbers, which
     29        created the opposite effect, this is, leaving in place keys in the
     30        CDMProxy store that should be released.
     31
     32        * platform/encryptedmedia/CDMProxy.cpp:
     33        (WebCore::KeyStore::merge): Simplified to just add keys.
     34        (WebCore::KeyStore::add): Adds references (if needed) and merges
     35        if needed.
     36        (WebCore::KeyStore::unrefAllKeys): Renamed. Unrefs all keys from a
     37        store by copying itself and calling unrefAllKeysFrom that copy.
     38        (WebCore::KeyStore::unref): Renamed. Uses proper refefencing.
     39        (WebCore::CDMProxy::unrefAllKeysFrom): Method to unref all keys
     40        that are contained in some other store.
     41        (WebCore::CDMInstanceProxy::mergeKeysFrom): There is no more key
     42        store in this class.
     43        (WebCore::CDMInstanceProxy::unrefAllKeysFrom): Renamed. Calls the
     44        CDMproxy to unref the keys from there.
     45        * platform/encryptedmedia/CDMProxy.h:
     46        (WebCore::KeyHandle::mergeKeyInto): Merges one key into self by
     47        copying status, data and summing reference count.
     48        (WebCore::KeyHandle::numSessionReferences const): Turn int.
     49        (WebCore::KeyHandle::hasReferences const): Added.
     50        (WebCore::KeyStore::addSessionReferenceTo const):
     51        (WebCore::KeyStore::removeSessionReferenceFrom const): Helpers to
     52        check if the store is reference counted or not. Default is do
     53        nothing and but the ReferenceAwareKeyStore redefines them to do
     54        reference counting.
     55        (WebCore::KeyStore::unrefAllKeys): Deleted.
     56        * platform/encryptedmedia/clearkey/CDMClearKey.cpp:
     57        (WebCore::CDMInstanceSessionClearKey::updateLicense):
     58        (WebCore::CDMInstanceSessionClearKey::removeSessionData): Use
     59        renamed methods.
     60        * platform/graphics/gstreamer/eme/CDMThunder.h:
     61        * platform/graphics/gstreamer/eme/CDMThunder.cpp:
     62        (WebCore::CDMInstanceSessionThunder::status const):
     63        (WebCore::CDMInstanceSessionThunder::keyUpdatedCallback):
     64        (WebCore::CDMInstanceSessionThunder::requestLicense):
     65        (WebCore::CDMInstanceSessionThunder::updateLicense):
     66        (WebCore::CDMInstanceSessionThunder::removeSessionData):
     67        (WebCore::CDMInstanceSessionThunder::loadSession): Use BoxPtr in
     68        the session.
     69        (WebCore::CDMInstanceSessionThunder::closeSession): Close the
     70        current session, release the BoxPtr, notify the instance and
     71        therefore the proxy to unref all they key IDs in this session and
     72        empty the local key store.
     73
    1742021-06-28  Darin Adler  <darin@apple.com>
    275

  • trunk/Source/WebCore/platform/encryptedmedia/CDMProxy.cpp

    r278253 r279363  
    118118    ASSERT(isMainThread());
    119119    LOG(EME, "EME - CDMProxy - merging %u new keys into a key store of %u keys", other.numKeys(), numKeys());
    120     for (const auto& key : other) {
    121         // NOTE: Do we care that we will not append a key if it matches a key ID
    122         // in the keystore and has different data. Should we overwrite? Which is "newer"?
    123         // Don't think we need this extra complexity.
    124         size_t keyWithMatchingKeyIDIndex = m_keys.findMatching([&] (const RefPtr<KeyHandle>& storedKey) { return *key == *storedKey; });
    125         if (keyWithMatchingKeyIDIndex == WTF::notFound)
    126             m_keys.append(key);
    127         else {
    128             LOG(EME, "EME - CDMProxy - keys with the same ID!");
    129             ASSERT(key->value() == m_keys[keyWithMatchingKeyIDIndex]->value());
    130         }
    131     }
     120    for (const auto& key : other)
     121        add(key.copyRef());
    132122
    133123#if !LOG_DISABLED
    … …  
    163153    });
    164154
     155    addSessionReferenceTo(key);
    165156    if (keyWithMatchingKeyIDIndex != WTF::notFound) {
    166157        auto& keyWithMatchingKeyID = m_keys[keyWithMatchingKeyIDIndex];
    167         didStoreChange = keyWithMatchingKeyID == key;
     158        didStoreChange = keyWithMatchingKeyID != key;
    168159        if (didStoreChange)
    169             keyWithMatchingKeyID = key;
     160            keyWithMatchingKeyID->mergeKeyInto(WTFMove(key));
    170161    } else {
    171162        LOG(EME, "EME - ClearKey - New key with ID %s getting added to key store", key->idAsString().ascii().data());     
    172         m_keys.append(key);
     163        m_keys.append(WTFMove(key));
    173164        didStoreChange = true;
    174165    }
    … …  
    184175    }
    185176
    186     key->addSessionReference();
    187177    return didStoreChange;
    188178}
    189179
    190 void KeyStore::removeAllKeysFrom(const KeyStore& other)
     180void KeyStore::unrefAllKeysFrom(const KeyStore& other)
    191181{
    192182    for (const auto& key : other)
    193         remove(key);
    194 }
    195 
    196 bool KeyStore::remove(const RefPtr<KeyHandle>& key)
     183        unref(key);
     184}
     185
     186void KeyStore::unrefAllKeys()
     187{
     188    KeyStore store(*this);
     189    unrefAllKeysFrom(store);
     190}
     191
     192bool KeyStore::unref(const RefPtr<KeyHandle>& key)
    197193{
    198194    bool storeChanged = false;
    199195
    200196    size_t keyWithMatchingKeyIDIndex = m_keys.find(key);
    201     LOG(EME, "EME - ClearKey - requested to remove key with ID %s and %u session references", key->idAsString().ascii().data(), key->numSessionReferences());
     197    LOG(EME, "EME - ClearKey - requested to unref key with ID %s and %d session references", key->idAsString().ascii().data(), key->numSessionReferences());
    202198
    203199    if (keyWithMatchingKeyIDIndex != WTF::notFound) {
    204200        auto& keyWithMatchingKeyID = m_keys[keyWithMatchingKeyIDIndex];
    205         keyWithMatchingKeyID->removeSessionReference();
    206         if (!keyWithMatchingKeyID->numSessionReferences()) {
    207             LOG(EME, "EME - ClearKey - remove key with ID %s", keyWithMatchingKeyID->idAsString().ascii().data());
     201        removeSessionReferenceFrom(keyWithMatchingKeyID);
     202        if (!keyWithMatchingKeyID->hasReferences()) {
     203            LOG(EME, "EME - ClearKey - unref key with ID %s", keyWithMatchingKeyID->idAsString().ascii().data());
    208204            m_keys.remove(keyWithMatchingKeyIDIndex);
    209205            storeChanged = true;
    210206        }
    211207    } else
    212         LOG(EME, "EME - ClearKey - attempt to remove key with ID %s ignored, does not exist", key->idAsString().ascii().data());
     208        LOG(EME, "EME - ClearKey - attempt to unref key with ID %s ignored, does not exist", key->idAsString().ascii().data());
    213209
    214210    return storeChanged;
    … …  
    247243    Locker locker { m_instanceLock };
    248244    return m_instance;
     245}
     246
     247void CDMProxy::unrefAllKeysFrom(const KeyStore& keyStore)
     248{
     249    Locker locker { m_keysLock };
     250    m_keyStore.unrefAllKeysFrom(keyStore);
     251    LOG(EME, "EME - CDMProxy - removing from key store from a session closure");
     252    m_keysCondition.notifyAll();
    249253}
    250254
    … …  
    378382    // FIXME: Notify JS when appropriate.
    379383    ASSERT(isMainThread());
    380     m_keyStore.merge(keyStore);
    381384    if (m_cdmProxy) {
    382385        LOG(EME, "EME - CDMInstanceProxy - merging keys into proxy instance and notifying CDMProxy of changes");
    … …  
    385388}
    386389
    387 void CDMInstanceProxy::removeAllKeysFrom(const KeyStore& keyStore)
     390void CDMInstanceProxy::unrefAllKeysFrom(const KeyStore& keyStore)
    388391{
    389392    ASSERT(isMainThread());
    390     m_keyStore.removeAllKeysFrom(keyStore);
     393    if (m_cdmProxy) {
     394        LOG(EME, "EME - CDMInstanceProxy - removing keys from proxy instance and notifying CDMProxy of changes");
     395        m_cdmProxy->unrefAllKeysFrom(keyStore);
     396    }
    391397}
    392398

  • trunk/Source/WebCore/platform/encryptedmedia/CDMProxy.h

    r278253 r279363  
    6969    KeyHandleValueVariant& value() { return m_value; }
    7070    KeyStatus status() const { return m_status; }
     71    void mergeKeyInto(RefPtr<KeyHandle>&& other)
     72    {
     73        m_status = other->m_status;
     74        m_value = other->m_value;
     75        m_numSessionReferences += other->m_numSessionReferences;
     76    }
    7177    bool isStatusCurrentlyValid()
    7278    {
    … …  
    100106    void addSessionReference() { ASSERT(isMainThread()); m_numSessionReferences++; }
    101107    void removeSessionReference() { ASSERT(isMainThread()); m_numSessionReferences--; }
    102     unsigned numSessionReferences() const { ASSERT(isMainThread()); return m_numSessionReferences; }
     108    int numSessionReferences() const { ASSERT(isMainThread()); return m_numSessionReferences; }
     109    bool hasReferences() const { ASSERT(isMainThread()); return m_numSessionReferences > 0; }
    103110    friend class KeyStore;
     111    friend class ReferenceAwareKeyStore;
    104112
    105113    KeyHandle(KeyStatus status, KeyIDType&& keyID, KeyHandleValueVariant&& keyHandleValue)
    … …  
    109117    KeyIDType m_id;
    110118    KeyHandleValueVariant m_value;
    111     unsigned m_numSessionReferences { 0 };
     119    int m_numSessionReferences { 0 };
    112120};
    113121
    … …  
    117125
    118126    KeyStore() = default;
     127    virtual ~KeyStore() = default;
    119128
    120129    bool containsKeyID(const KeyIDType&) const;
    121130    void merge(const KeyStore&);
    122     void removeAllKeysFrom(const KeyStore&);
    123     void removeAllKeys() { m_keys.clear(); }
     131    void unrefAllKeysFrom(const KeyStore&);
     132    void unrefAllKeys();
    124133    bool addKeys(Vector<RefPtr<KeyHandle>>&&);
    125134    bool add(RefPtr<KeyHandle>&&);
    126     bool remove(const RefPtr<KeyHandle>&);
     135    bool unref(const RefPtr<KeyHandle>&);
    127136    bool hasKeys() const { return m_keys.size(); }
    128137    unsigned numKeys() const { return m_keys.size(); }
    … …  
    131140    KeyStatusVector convertToJSKeyStatusVector() const;
    132141    bool isEmpty() const { return m_keys.isEmpty(); }
     142    virtual void addSessionReferenceTo(const RefPtr<KeyHandle>&) const { }
     143    virtual void removeSessionReferenceFrom(const RefPtr<KeyHandle>&) const { };
    133144
    134145    auto begin() { return m_keys.begin(); }
    … …  
    145156};
    146157
     158class ReferenceAwareKeyStore : public KeyStore {
     159public:
     160    virtual ~ReferenceAwareKeyStore() = default;
     161    void addSessionReferenceTo(const RefPtr<KeyHandle>& key) const final { key->addSessionReference(); }
     162    void removeSessionReferenceFrom(const RefPtr<KeyHandle>& key) const final { key->removeSessionReference(); }
     163};
     164
    147165class CDMInstanceProxy;
    148166class CDMProxyDecryptionClient;
    … …  
    156174    virtual ~CDMProxy() = default;
    157175
    158     void updateKeyStore(const KeyStore& newKeyStore);
     176    void updateKeyStore(const KeyStore&);
     177    void unrefAllKeysFrom(const KeyStore&);
    159178    void setInstance(CDMInstanceProxy*);
    160179    void abortWaitingForKey() const;
    … …  
    179198    // FIXME: Duplicated key stores in the instance and the proxy are probably not needed, but simplified
    180199    // the initial implementation in terms of threading invariants.
    181     KeyStore m_keyStore WTF_GUARDED_BY_LOCK(m_keysLock);
     200    ReferenceAwareKeyStore m_keyStore WTF_GUARDED_BY_LOCK(m_keysLock);
    182201};
    183202
    … …  
    230249    // Main-thread only.
    231250    void mergeKeysFrom(const KeyStore&);
    232     void removeAllKeysFrom(const KeyStore&);
     251    void unrefAllKeysFrom(const KeyStore&);
    233252
    234253    // Media player query methods - main thread only.
    … …  
    249268
    250269    std::atomic<int> m_numDecryptorsWaitingForKey { 0 };
    251 
    252     KeyStore m_keyStore;
    253270};
    254271

  • trunk/Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp

    r278516 r279363  
    521521    if (parseLicenseReleaseAcknowledgementFormat(*root)) {
    522522        LOG(EME, "EME - ClearKey - session %s release acknowledged, clearing all known keys", sessionId.utf8().data());
    523         parentInstance().removeAllKeysFrom(m_keyStore);
    524         m_keyStore.removeAllKeys();
     523        parentInstance().unrefAllKeysFrom(m_keyStore);
     524        m_keyStore.unrefAllKeys();
    525525        dispatchCallback(true, std::nullopt, SuccessValue::Succeeded);
    526526        return;
    … …  
    600600    }
    601601
    602     m_keyStore.removeAllKeys();
     602    m_keyStore.unrefAllKeys();
    603603    dispatchCallback(WTFMove(keyStatusVector), Ref<SharedBuffer>(*message), SuccessValue::Succeeded);
    604604}

  • trunk/Source/WebCore/platform/graphics/gstreamer/eme/CDMThunder.cpp

    r278516 r279363  
    433433CDMInstanceSession::KeyStatus CDMInstanceSessionThunder::status(const KeyIDType& keyID) const
    434434{
    435     ThunderKeyStatus status = m_session && !m_sessionID.isEmpty() ? opencdm_session_status(m_session.get(), keyID.data(), keyID.size()) : StatusPending;
     435    ThunderKeyStatus status = m_session && !m_sessionID.isEmpty() ? opencdm_session_status(m_session->get(), keyID.data(), keyID.size()) : StatusPending;
    436436
    437437    switch (status) {
    … …  
    462462    auto keyStatus = status(keyID);
    463463    GST_DEBUG("updated with with key status %s", toString(keyStatus));
    464     m_doesKeyStoreNeedMerging |= m_keyStore.add(KeyHandle::create(keyStatus, WTFMove(keyID), BoxPtr<OpenCDMSession>()));
     464    m_doesKeyStoreNeedMerging |= m_keyStore.add(KeyHandle::create(keyStatus, WTFMove(keyID), BoxPtr<OpenCDMSession>(m_session)));
    465465}
    466466
    … …  
    519519        return;
    520520    }
    521     m_session.reset(session);
    522     m_sessionID = String::fromUTF8(opencdm_session_id(m_session.get()));
     521    m_session = adoptInBoxPtr(session);
     522    m_sessionID = String::fromUTF8(opencdm_session_id(m_session->get()));
    523523
    524524    auto generateChallenge = [this, callback = WTFMove(callback)]() mutable {
    … …  
    587587        }
    588588    });
    589     if (!m_session || m_sessionID.isEmpty() || opencdm_session_update(m_session.get(), response->data(), response->size()))
     589    if (!m_session || m_sessionID.isEmpty() || opencdm_session_update(m_session->get(), response->data(), response->size()))
    590590        sessionFailure();
    591591}
    … …  
    624624        }
    625625    });
    626     if (!m_session || m_sessionID.isEmpty() || opencdm_session_load(m_session.get()))
     626    if (!m_session || m_sessionID.isEmpty() || opencdm_session_load(m_session->get()))
    627627        sessionFailure();
    628628}
    … …  
    632632    ASSERT_UNUSED(sessionID, m_sessionID == sessionID);
    633633
    634     if (m_session && !m_sessionID.isEmpty())
    635         opencdm_session_close(m_session.get());
     634    if (m_session && !m_sessionID.isEmpty()) {
     635        opencdm_session_close(m_session->get());
     636        m_session = BoxPtr<OpenCDMSession>();
     637        auto instance = cdmInstanceThunder();
     638        if (instance) {
     639            instance->unrefAllKeysFrom(m_keyStore);
     640            m_keyStore.unrefAllKeys();
     641        }
     642    }
    636643
    637644    callback();
    … …  
    665672        }
    666673    });
    667     if (!m_session || m_sessionID.isEmpty() || opencdm_session_remove(m_session.get()))
     674    if (!m_session || m_sessionID.isEmpty() || opencdm_session_remove(m_session->get()))
    668675        sessionFailure();
    669676}

  • trunk/Source/WebCore/platform/graphics/gstreamer/eme/CDMThunder.h

    r278244 r279363  
    5050
    5151using UniqueThunderSystem = std::unique_ptr<OpenCDMSystem, ThunderSystemDeleter>;
    52 
    53 using UniqueThunderSession = std::unique_ptr<OpenCDMSession, WTF::BoxPtrDeleter<OpenCDMSession>>;
    5452
    5553} // namespace Thunder
    … …  
    162160    InitData m_initData;
    163161    OpenCDMSessionCallbacks m_thunderSessionCallbacks { };
    164     Thunder::UniqueThunderSession m_session;
     162    BoxPtr<OpenCDMSession> m_session;
    165163    RefPtr<SharedBuffer> m_message;
    166164    bool m_needsIndividualization { false };
Note: See TracChangeset for help on using the changeset viewer.