Merge changes I30a3ede3,I6008ddba,I869c22a2 into pi-dev

* changes:
  Protect sessions from concurrent access.
  Address concurrency failures between calls to decrypt and periodic timer
  Revert of "Prevent race conditions between decrypt and close session"

libwvdrmengine/cdm/core/include/cdm_engine.h

@@ -330,6 +330,17 @@ class CdmEngine {
   // Protect release_key_sets_ from non-thread-safe operations.
   Lock release_key_sets_lock_;
 
+  // TODO(rfrias): Replace with two sets of locks, one to protect
+  // the CdmSessionMap and a per-session lock to control access to
+  // session usage/destruction.
+  // Locks the session map |session_map_| and session usage/destruction
+  // between session management calls (OpenSession, CloseSession, etc),
+  // periodic timer calls (OnTimerEvent), and calls to Decrypt.
+  // 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_map_lock_;
+
   CORE_DISALLOW_COPY_AND_ASSIGN(CdmEngine);
 };
 

libwvdrmengine/cdm/core/include/cdm_session.h

@@ -37,7 +37,7 @@ class CdmSession {
   CdmSession(FileSystem* file_system, metrics::SessionMetrics* metrics);
   virtual ~CdmSession();
 
-  void Close();
+  void Close() { closed_ = true; }
   bool IsClosed() { return closed_; }
 
   // Initializes this instance of CdmSession with the given property set.
@@ -263,9 +263,6 @@ class CdmSession {
   bool mock_license_parser_in_use_;
   bool mock_policy_engine_in_use_;
 
-  // Lock to avoid race conditions between Close() and Decrypt()
-  Lock close_lock_;
-
   CORE_DISALLOW_COPY_AND_ASSIGN(CdmSession);
 };
 

libwvdrmengine/cdm/core/include/cdm_session_map.h

@@ -10,7 +10,6 @@
 #include <string>
 
 #include "cdm_session.h"
-#include "lock.h"
 #include "shared_ptr.h"
 #include "wv_cdm_types.h"
 
@@ -18,11 +17,18 @@ namespace wvcdm {
 
 typedef std::list<shared_ptr<CdmSession> > CdmSessionList;
 
+// TODO(rfrias): Concurrency protection for this class has moved to CdmEngine.
+// Add it back when locks to control access to session usage and destruction
+// are introduced.
 class CdmSessionMap {
  public:
   CdmSessionMap() {}
   virtual ~CdmSessionMap();
 
+  // Use |Terminate| rather than relying on the destructor to release
+  // resources, as it can be protected by locks.
+  void Terminate();
+
   void Add(const std::string& id, CdmSession* session);
 
   bool CloseSession(const std::string& id);
@@ -43,7 +49,6 @@ class CdmSessionMap {
   bool FindSessionNoLock(const CdmSessionId& session_id,
                          shared_ptr<CdmSession>* session);
 
-  Lock lock_;
   CdmIdToSessionMap sessions_;
 
   CORE_DISALLOW_COPY_AND_ASSIGN(CdmSessionMap);

libwvdrmengine/cdm/core/include/shared_ptr.h

@@ -12,9 +12,17 @@
 #include <stddef.h>
 #include <memory>
 
+#include "lock.h"
 #include "wv_cdm_types.h"
 
+namespace wvcdm {
+
+extern Lock shared_ptr_ref_count_lock_;
+
+}  // namespace wvcdm
+
 namespace {
+
 bool Barrier_AtomicIncrement(volatile uint32_t* ptr, uint32_t value) {
   *ptr += value;
   return *ptr;
@@ -25,10 +33,12 @@ bool NoBarrier_AtomicIncrement(volatile uint32_t* ptr, uint32_t value) {
 }
 
 inline bool RefCountDec(volatile uint32_t *ptr) {
+  wvcdm::AutoLock auto_lock(wvcdm::shared_ptr_ref_count_lock_);
   return Barrier_AtomicIncrement(ptr, -1) != 0;
 }
 
 inline void RefCountInc(volatile uint32_t *ptr) {
+  wvcdm::AutoLock auto_lock(wvcdm::shared_ptr_ref_count_lock_);
   NoBarrier_AtomicIncrement(ptr, 1);
 }
 

libwvdrmengine/cdm/core/include/wv_cdm_types.h

@@ -333,7 +333,6 @@ enum CdmResponseType {
   GET_PROVISIONING_METHOD_ERROR = 289,
   SESSION_NOT_FOUND_17 = 290,
   SESSION_NOT_FOUND_18 = 291,
-  SESSION_CLOSED_1 = 292,
 };
 
 enum CdmKeyStatus {

libwvdrmengine/cdm/core/src/cdm_engine.cpp

@@ -30,6 +30,8 @@ const size_t kUsageReportsPerRequest = 1;
 
 namespace wvcdm {
 
+Lock shared_ptr_ref_count_lock_;
+
 class UsagePropertySet : public CdmClientPropertySet {
  public:
   UsagePropertySet() {}
@@ -83,7 +85,10 @@ CdmEngine::CdmEngine(FileSystem* file_system, const std::string& spoid)
   }
 }
 
-CdmEngine::~CdmEngine() {}
+CdmEngine::~CdmEngine() {
+  AutoLock lock(session_map_lock_);
+  session_map_.Terminate();
+}
 
 CdmResponseType CdmEngine::OpenSession(
     const CdmKeySystem& key_system, CdmClientPropertySet* property_set,
@@ -141,6 +146,7 @@ CdmResponseType CdmEngine::OpenSession(
   CdmSessionId id = new_session->session_id();
   LOGI("CdmEngine::OpenSession: %s", id.c_str());
 
+  AutoLock lock(session_map_lock_);
   session_map_.Add(id, new_session.release());
   if (session_id) *session_id = id;
   return NO_ERROR;
@@ -182,6 +188,7 @@ CdmResponseType CdmEngine::OpenKeySetSession(
 
 CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) {
   LOGI("CdmEngine::CloseSession: %s", session_id.c_str());
+  AutoLock lock(session_map_lock_);
   if (!session_map_.CloseSession(session_id)) {
     LOGE("CdmEngine::CloseSession: session not found = %s", session_id.c_str());
     return SESSION_NOT_FOUND_1;
@@ -215,6 +222,7 @@ CdmResponseType CdmEngine::CloseKeySetSession(const CdmKeySetId& key_set_id) {
 }
 
 bool CdmEngine::IsOpenSession(const CdmSessionId& session_id) {
+  AutoLock lock(session_map_lock_);
   return session_map_.Exists(session_id);
 }
 
@@ -1470,6 +1478,7 @@ CdmResponseType CdmEngine::Decrypt(const CdmSessionId& session_id,
     // else we must be level 1 direct and we don't need to return a buffer.
   }
 
+  AutoLock lock(session_map_lock_);
   shared_ptr<CdmSession> session;
   if (session_id.empty()) {
     CdmSessionList sessions;
@@ -1590,6 +1599,7 @@ bool CdmEngine::FindSessionForKey(const KeyId& key_id,
 
   uint32_t session_sharing_id = Properties::GetSessionSharingId(*session_id);
 
+  AutoLock lock(session_map_lock_);
   CdmSessionList sessions;
   session_map_.GetSessionList(sessions);
 
@@ -1635,9 +1645,6 @@ void CdmEngine::OnTimerEvent() {
   Clock clock;
   uint64_t current_time = clock.GetCurrentTime();
 
-  CdmSessionList sessions;
-  session_map_.GetSessionList(sessions);
-
   bool usage_update_period_expired = false;
   if (current_time - last_usage_information_update_time_ >
       kUpdateUsageInformationPeriod) {
@@ -1648,6 +1655,10 @@ void CdmEngine::OnTimerEvent() {
   bool is_initial_usage_update = false;
   bool is_usage_update_needed = false;
 
+  AutoLock lock(session_map_lock_);
+  CdmSessionList sessions;
+  session_map_.GetSessionList(sessions);
+
   while (!sessions.empty()) {
     is_initial_usage_update =
         is_initial_usage_update || sessions.front()->is_initial_usage_update();

libwvdrmengine/cdm/core/src/cdm_session.cpp

@@ -72,11 +72,6 @@ CdmSession::~CdmSession() {
   }
 }
 
-void CdmSession::Close() {
-  AutoLock lock(close_lock_);
-  closed_ = true;
-}
-
 CdmResponseType CdmSession::Init(
     CdmClientPropertySet* cdm_client_property_set) {
   return Init(cdm_client_property_set, NULL, NULL);
@@ -581,11 +576,6 @@ CdmResponseType CdmSession::Decrypt(const CdmDecryptionParameters& params) {
     return NOT_INITIALIZED_ERROR;
   }
 
-  AutoLock lock(close_lock_);
-  if (IsClosed()) {
-    return SESSION_CLOSED_1;
-  }
-
   // Playback may not begin until either the start time passes or the license
   // is updated, so we treat this Decrypt call as invalid.
   if (params.is_encrypted) {

libwvdrmengine/cdm/core/src/cdm_session_map.cpp

@@ -12,7 +12,10 @@
 namespace wvcdm {
 
 CdmSessionMap::~CdmSessionMap() {
-  AutoLock lock(lock_);
+  Terminate();
+}
+
+void CdmSessionMap::Terminate() {
   for (CdmIdToSessionMap::iterator i = sessions_.begin();
        i != sessions_.end(); ++i) {
     i->second->Close();
@@ -22,12 +25,10 @@ CdmSessionMap::~CdmSessionMap() {
 }
 
 void CdmSessionMap::Add(const std::string& id, CdmSession* session) {
-  AutoLock lock(lock_);
   sessions_[id].reset(session);
 }
 
 bool CdmSessionMap::CloseSession(const std::string& id) {
-  AutoLock lock(lock_);
   shared_ptr<CdmSession> session;
   if (!FindSessionNoLock(id, &session)) {
     return false;
@@ -38,13 +39,11 @@ bool CdmSessionMap::CloseSession(const std::string& id) {
 }
 
 bool CdmSessionMap::Exists(const std::string& id) {
-  AutoLock lock(lock_);
   return sessions_.find(id) != sessions_.end();
 }
 
 bool CdmSessionMap::FindSession(const CdmSessionId& id,
                                 shared_ptr<CdmSession>* session) {
-  AutoLock lock(lock_);
   return FindSessionNoLock(id, session);
 }
 
@@ -61,7 +60,6 @@ bool CdmSessionMap::FindSessionNoLock(const CdmSessionId& session_id,
 
 void CdmSessionMap::GetSessionList(CdmSessionList& sessions) {
   sessions.clear();
-  AutoLock lock(lock_);
   for (CdmIdToSessionMap::iterator iter = sessions_.begin();
        iter != sessions_.end(); ++iter) {
     if (!(iter->second)->IsClosed()) {

libwvdrmengine/cdm/core/test/test_printers.cpp

@@ -599,8 +599,6 @@ void PrintTo(const enum CdmResponseType& value, ::std::ostream* os) {
       break;
     case SESSION_NOT_FOUND_18: *os << "SESSION_NOT_FOUND_18";
       break;
-    case SESSION_CLOSED_1: *os << "SESSION_CLOSED_1";
-      break;
     default:
       *os                                        << "Unknown CdmResponseType";
       break;

libwvdrmengine/include/mapErrors-inl.h

@@ -230,7 +230,6 @@ static android::status_t mapCdmResponseType(wvcdm::CdmResponseType res) {
     case wvcdm::SESSION_NOT_FOUND_10:
     case wvcdm::SESSION_NOT_FOUND_17:
     case wvcdm::SESSION_NOT_FOUND_18:
-    case wvcdm::SESSION_CLOSED_1:
         return android::ERROR_DRM_SESSION_NOT_OPENED;
     case wvcdm::SESSION_KEYS_NOT_FOUND:
       return kSessionKeysNotFound;

libwvdrmengine/include_hidl/mapErrors-inl.h

@@ -58,7 +58,6 @@ static Status mapCdmResponseType(wvcdm::CdmResponseType res) {
     case wvcdm::SESSION_NOT_FOUND_10:
     case wvcdm::SESSION_NOT_FOUND_17:
     case wvcdm::SESSION_NOT_FOUND_18:
-    case wvcdm::SESSION_CLOSED_1:
       return Status::ERROR_DRM_SESSION_NOT_OPENED;
 
     case wvcdm::DECRYPT_ERROR: