Lock session list in CdmEngine OnTimerEvent

This is a copy of the widevine CL:
https://widevine-internal-review.googlesource.com/#/c/12742/

If a session is closed at the same time as an OnTimerEvent is
processing an event, there could be a race condition between the two
threads.  This CL adds a lock that prevents a session from being
removed from the list while the timer is currently processing an
event.

If CloseSession is called while the OnTimerEvent method is active, the
session will be added to a dead list, and deleted when the timer event
has finished.

This CL does not address the main problem in bug 19252886, but
one bugreport, netflix_log_3.txt, indicates there may have been
a problem with the CDM timer.
bug: 19252886

Change-Id: I17190edaeb3eef1295d4d204232cc4262cb5fa9b

libwvdrmengine/cdm/core/include/cdm_engine.h

@@ -9,6 +9,7 @@
 #include "certificate_provisioning.h"
 #include "crypto_session.h"
 #include "initialization_data.h"
+#include "lock.h"
 #include "oemcrypto_adapter.h"
 #include "scoped_ptr.h"
 #include "wv_cdm_types.h"
@@ -154,6 +155,12 @@ class CdmEngine {
   scoped_ptr<UsagePropertySet> usage_property_set_;
   int64_t last_usage_information_update_time_;
 
+  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<CdmSession*> dead_sessions_;
+
   CORE_DISALLOW_COPY_AND_ASSIGN(CdmEngine);
 };
 

libwvdrmengine/cdm/core/src/cdm_engine.cpp

@@ -58,7 +58,8 @@ CdmEngine::CdmEngine()
     : cert_provisioning_(NULL),
       cert_provisioning_requested_security_level_(kLevelDefault),
       usage_session_(NULL),
-      last_usage_information_update_time_(0) {
+      last_usage_information_update_time_(0),
+      timer_event_active_(false) {
   Properties::Init();
   if (!seeded_) {
     Clock clock;
@@ -68,6 +69,7 @@ CdmEngine::CdmEngine()
 }
 
 CdmEngine::~CdmEngine() {
+  AutoLock lock(session_list_lock_);
   CdmSessionMap::iterator i(sessions_.begin());
   for (; i != sessions_.end(); ++i) {
     delete i->second;
@@ -108,6 +110,7 @@ CdmResponseType CdmEngine::OpenSession(
     return sts;
   }
   *session_id = new_session->session_id();
+  AutoLock lock(session_list_lock_);
   sessions_[*session_id] = new_session.release();
   return NO_ERROR;
 }
@@ -134,7 +137,7 @@ CdmResponseType CdmEngine::OpenKeySetSession(
 
 CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) {
   LOGI("CdmEngine::CloseSession");
-
+  AutoLock lock(session_list_lock_);
   CdmSessionMap::iterator iter = sessions_.find(session_id);
   if (iter == sessions_.end()) {
     LOGE("CdmEngine::CloseSession: session not found = %s", session_id.c_str());
@@ -143,7 +146,9 @@ CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) {
 
   CdmSession* session = iter->second;
   sessions_.erase(session_id);
-  delete session;
+  // 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;
   return NO_ERROR;
 }
 
@@ -905,26 +910,38 @@ 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;
   for (CdmSessionMap::iterator iter = sessions_.begin();
-      iter != sessions_.end(); ++iter) {
+       iter != sessions_.end() && index < session_count; ++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();
 
-    iter->second->OnTimerEvent(usage_update_period_expired);
+
+  for (size_t i=0; i < session_count; i++) {
+    event_sessions[i]->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 (CdmSessionMap::iterator iter = sessions_.begin();
-        iter != sessions_.end(); ++iter) {
-      iter->second->reset_usage_flags();
+    for (size_t i=0; i < session_count; i++) {
+      event_sessions[i]->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 = iter->second->UpdateUsageInformation();
+        CdmResponseType status = event_sessions[i]->UpdateUsageInformation();
         if (NO_ERROR != status) {
           LOGW("Update usage information failed: %d", status);
         } else {
@@ -933,6 +950,16 @@ 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) {