From 6444332cd7bf136faadb43f7efb5c97706b1624d Mon Sep 17 00:00:00 2001 From: Fred Gylys-Colwell Date: Fri, 6 Mar 2015 13:29:21 -0800 Subject: [PATCH] Lock session list in CdmEngine OnTimerEvent (master) Cherry pick of https://widevine-internal-review.googlesource.com/#/c/12935/ Change-Id: I029d36b2b6d092ae938fca2a7f6d893814c25a8a --- libwvdrmengine/cdm/core/include/cdm_engine.h | 22 +++++++--- libwvdrmengine/cdm/core/include/cdm_session.h | 1 + libwvdrmengine/cdm/core/src/cdm_engine.cpp | 44 ++++--------------- 3 files changed, 26 insertions(+), 41 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/cdm_engine.h b/libwvdrmengine/cdm/core/include/cdm_engine.h index c9d0ea61..aaf2e24a 100644 --- a/libwvdrmengine/cdm/core/include/cdm_engine.h +++ b/libwvdrmengine/cdm/core/include/cdm_engine.h @@ -138,9 +138,16 @@ class CdmEngine { virtual bool IsKeyLoaded(const KeyId& key_id); virtual bool FindSessionForKey(const KeyId& key_id, CdmSessionId* sessionId); - // Event listener related methods + // Event listener related methods. + + // We assume that the WvCdmEventListener is asynchronous -- i.e. an event + // should be dispatched to another thread which actually does the work. In + // particular, if a syncrhonous listener calls OpenSession or CloseSession, + // the thread will dead lock. + // Returns false if listener already attached. virtual bool AttachEventListener(const CdmSessionId& session_id, WvCdmEventListener* listener); + // Returns true if listener was detached. virtual bool DetachEventListener(const CdmSessionId& session_id, WvCdmEventListener* listener); @@ -148,7 +155,8 @@ class CdmEngine { virtual void NotifyResolution(const CdmSessionId& session_id, uint32_t width, uint32_t height); - // Timer expiration method + // Timer expiration method. This method is not re-entrant -- there can be + // only one timer. virtual void OnTimerEvent(); private: @@ -175,11 +183,13 @@ class CdmEngine { scoped_ptr usage_property_set_; int64_t last_usage_information_update_time_; + // Locks the list of sessions, |sessions_|, for the event timer. It will be + // locked in OpenSession, CloseSession. It is also locked in OnTimerEvent and + // OnKeyReleaseEvent while the list of event listeners is being generated. + // The layer above the CDM implementation is expected to handle thread + // synchronization to make sure other functions that access sessions do not + // occur simultaneously with OpenSession or CloseSession. Lock session_list_lock_; - // If the timer is currently active -- do not delete any sessions. - bool timer_event_active_; - // Sessions to be deleted after the timer has finished. - std::vector dead_sessions_; CORE_DISALLOW_COPY_AND_ASSIGN(CdmEngine); }; diff --git a/libwvdrmengine/cdm/core/include/cdm_session.h b/libwvdrmengine/cdm/core/include/cdm_session.h index dd1079af..15e72716 100644 --- a/libwvdrmengine/cdm/core/include/cdm_session.h +++ b/libwvdrmengine/cdm/core/include/cdm_session.h @@ -74,6 +74,7 @@ class CdmSession { virtual bool IsKeyLoaded(const KeyId& key_id); + // See comments for CdmEngine::AttachEventListener/DetachEventListener. virtual bool AttachEventListener(WvCdmEventListener* listener); virtual bool DetachEventListener(WvCdmEventListener* listener); diff --git a/libwvdrmengine/cdm/core/src/cdm_engine.cpp b/libwvdrmengine/cdm/core/src/cdm_engine.cpp index 8b497110..8f36beb0 100644 --- a/libwvdrmengine/cdm/core/src/cdm_engine.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_engine.cpp @@ -140,15 +140,9 @@ CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) { LOGE("CdmEngine::CloseSession: session not found = %s", session_id.c_str()); return KEY_ERROR; } - CdmSession* session = iter->second; sessions_.erase(session_id); - // If the event timer is active, it will delete this session when it's done. - if (timer_event_active_) { - dead_sessions_.push_back(session); - } else { - delete session; - } + delete session; return NO_ERROR; } @@ -894,38 +888,27 @@ void CdmEngine::OnTimerEvent() { bool is_initial_usage_update = false; bool is_usage_update_needed = false; - // Make a copy of all currently open sessions. While this event handler is - // active, after we release session_list_lock_, CloseSession and OpenSession - // might modify sessions_, but they will not delete sessions. - session_list_lock_.Acquire(); - const size_t session_count = sessions_.size(); - CdmSession* event_sessions[session_count]; - timer_event_active_ = true; - size_t index = 0; + AutoLock lock(session_list_lock_); for (CdmSessionMap::iterator iter = sessions_.begin(); - iter != sessions_.end() && index < session_count; ++iter) { + iter != sessions_.end(); ++iter) { is_initial_usage_update = is_initial_usage_update || iter->second->is_initial_usage_update(); is_usage_update_needed = is_usage_update_needed || iter->second->is_usage_update_needed(); - event_sessions[index++] = iter->second; - } - session_list_lock_.Release(); - - for (size_t i=0; i < session_count; i++) { - event_sessions[i]->OnTimerEvent(usage_update_period_expired); + iter->second->OnTimerEvent(usage_update_period_expired); } if (is_usage_update_needed && (usage_update_period_expired || is_initial_usage_update)) { bool has_usage_been_updated = false; - for (size_t i=0; i < session_count; i++) { - event_sessions[i]->reset_usage_flags(); + for (CdmSessionMap::iterator iter = sessions_.begin(); + iter != sessions_.end(); ++iter) { + iter->second->reset_usage_flags(); if (!has_usage_been_updated) { // usage is updated for all sessions so this needs to be // called only once per update usage information period - CdmResponseType status = event_sessions[i]->UpdateUsageInformation(); + CdmResponseType status = iter->second->UpdateUsageInformation(); if (NO_ERROR != status) { LOGW("Update usage information failed: %d", status); } else { @@ -934,19 +917,10 @@ void CdmEngine::OnTimerEvent() { } } } - { - // We are done with the event handler. Now we look for any sessions that - // were closed. - AutoLock lock(session_list_lock_); - for (size_t i=0; i < dead_sessions_.size(); i++) { - delete dead_sessions_[i]; - } - dead_sessions_.clear(); - timer_event_active_ = false; - } } void CdmEngine::OnKeyReleaseEvent(const CdmKeySetId& key_set_id) { + AutoLock lock(session_list_lock_); for (CdmSessionMap::iterator iter = sessions_.begin(); iter != sessions_.end(); ++iter) { iter->second->OnKeyReleaseEvent(key_set_id);