From 182f3c805800ff6bf170c145ad311831a60fd0c2 Mon Sep 17 00:00:00 2001
From: Rahul Frias <rfrias@google.com>
Date: Fri, 6 Jan 2017 16:40:04 -0800
Subject: [PATCH] Avoid race condition on closing CDM session.

[ Merge from go/wvgerrit/22920 ]

Automated tests reveal a race condition between closing
a session and the periodic policy timer event. If a close
session was in progress (WVDrmPlugin::closeSession) and
CdmEngine::CloseSession caused the CdmEngine::session_list_lock_
to be held, a call into CdmEngine::OnTimerEvent would pend on the
release of the lock.

The close session would continue to deallocate the session
and disable (stop) the policy timer leaving the CdmEngine::OnTimerEvent
call in an undefined state. This would result in an ANR.

This subtle race-condition was introduced when changes were made
to add in per-origin storage [ http://go/wvgerrit/17971 ]. This seems
to happen at a low frequency (~ < 0.5%).

To address a lock has been introduced to protect the map
WvContentDecryptionModule::cdms_.

Test: Unit tests + 200 aupt test iterations

b/33343891

Change-Id: I9788db8a7d1df84f0df82cdbadb9d0f0fbe21e4e
---
 .../include/wv_content_decryption_module.h    |  2 ++
 .../cdm/src/wv_content_decryption_module.cpp  | 21 ++++++++++++-------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/libwvdrmengine/cdm/include/wv_content_decryption_module.h b/libwvdrmengine/cdm/include/wv_content_decryption_module.h
index d576d398..6c3d28b4 100644
--- a/libwvdrmengine/cdm/include/wv_content_decryption_module.h
+++ b/libwvdrmengine/cdm/include/wv_content_decryption_module.h
@@ -148,6 +148,8 @@ class WvContentDecryptionModule : public android::RefBase, public TimerHandler {
   // instance variables
   // This manages the lifetime of the CDM instances.
   std::map<std::string, CdmInfo> cdms_;
+  Lock cdms_lock_;
+
   // This contains weak pointers to the CDM instances contained in |cdms_|.
   std::map<std::string, CdmEngine*> cdm_by_session_id_;
 
diff --git a/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp b/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp
index 86799978..83f98fb5 100644
--- a/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp
+++ b/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp
@@ -265,6 +265,7 @@ WvContentDecryptionModule::CdmInfo::CdmInfo()
 
 CdmEngine* WvContentDecryptionModule::EnsureCdmForOrigin(
     const std::string& origin) {
+  AutoLock auto_lock(cdms_lock_);
   if (cdms_.find(origin) == cdms_.end()) {
     // Will create a new instance using the default constructor.
     cdms_[origin].file_system.SetOrigin(origin);
@@ -288,23 +289,27 @@ void WvContentDecryptionModule::EnablePolicyTimer() {
 }
 
 void WvContentDecryptionModule::DisablePolicyTimer(bool force) {
-  AutoLock auto_lock(policy_timer_lock_);
   bool has_sessions = false;
-  for (auto it = cdms_.begin(); it != cdms_.end();) {
-    if (it->second.cdm_engine->SessionSize() != 0) {
-      has_sessions = true;
-      ++it;
-    } else {
-      // The CDM is no longer used for this origin, delete it.
-      it = cdms_.erase(it);
+  {
+    AutoLock auto_lock(cdms_lock_);
+    for (auto it = cdms_.begin(); it != cdms_.end();) {
+      if (it->second.cdm_engine->SessionSize() != 0) {
+        has_sessions = true;
+        ++it;
+      } else {
+        // The CDM is no longer used for this origin, delete it.
+        it = cdms_.erase(it);
+      }
     }
   }
 
+  AutoLock auto_lock(policy_timer_lock_);
   if ((!has_sessions || force) && policy_timer_.IsRunning())
     policy_timer_.Stop();
 }
 
 void WvContentDecryptionModule::OnTimerEvent() {
+  AutoLock auto_lock(cdms_lock_);
   for (auto it = cdms_.begin(); it != cdms_.end(); ++it) {
     it->second.cdm_engine->OnTimerEvent();
   }