Switch to using shared_ptr for Session Metrics

[ Merge from http://go/wvgerrit/71443 ]

The assumption that the metrics will always outlive the CdmSession
instance appears not to always hold (at least in a non-android
multi-threaded solution). The shared_ptr ensures that the metrics
are available even in these rare race conditions.

BUG: http://b/123321465
Test: CDM unit tests. Also http://go/wvgerrit/71264 parallel tests.
Change-Id: Iaa6a8f6c0fdc46a911789759d6e1228d849aa237

libwvdrmengine/cdm/core/include/cdm_session.h

@@ -35,7 +35,8 @@ class CdmSession {
   // and |metrics| parameters. Both parameters are owned by the caller and
   // must remain in scope througout the scope of the new instance. |metrics|
   // must not be null.
-  CdmSession(FileSystem* file_system, metrics::SessionMetrics* metrics);
+  CdmSession(FileSystem* file_system,
+             std::shared_ptr<metrics::SessionMetrics> metrics);
   virtual ~CdmSession();
 
   void Close() { closed_ = true; }
@@ -198,7 +199,7 @@ class CdmSession {
 
   virtual CdmResponseType GetDecryptHashError(std::string* hash_error_string);
 
-  virtual metrics::SessionMetrics* GetMetrics() { return metrics_; }
+  virtual metrics::SessionMetrics* GetMetrics() { return metrics_.get(); }
 
  private:
   friend class CdmSessionTest;
@@ -223,7 +224,7 @@ class CdmSession {
   void set_file_handle(DeviceFiles* file_handle);
 
   // instance variables
-  metrics::SessionMetrics* metrics_;
+  std::shared_ptr<metrics::SessionMetrics> metrics_;
   metrics::CryptoMetrics* crypto_metrics_;
   metrics::TimerMetric life_span_;
   metrics::TimerMetric license_request_latency_;

libwvdrmengine/cdm/core/src/cdm_engine.cpp

@@ -133,8 +133,8 @@ CdmResponseType CdmEngine::OpenSession(
 
   CloseExpiredReleaseSessions();
 
-  std::unique_ptr<CdmSession> new_session(new CdmSession(file_system_,
-                                                    metrics_->AddSession()));
+  std::unique_ptr<CdmSession> new_session(
+      new CdmSession(file_system_, metrics_->AddSession()));
   CdmResponseType sts = new_session->Init(property_set, forced_session_id,
                                           event_listener);
   if (sts != NO_ERROR) {

libwvdrmengine/cdm/core/src/cdm_session.cpp

@@ -27,7 +27,7 @@ const size_t kKeySetIdLength = 14;
 namespace wvcdm {
 
 CdmSession::CdmSession(FileSystem* file_system,
-                       metrics::SessionMetrics* metrics)
+                       std::shared_ptr<metrics::SessionMetrics> metrics)
     : metrics_(metrics),
       initialized_(false),
       closed_(true),
@@ -47,7 +47,7 @@ CdmSession::CdmSession(FileSystem* file_system,
       usage_entry_number_(0),
       mock_license_parser_in_use_(false),
       mock_policy_engine_in_use_(false) {
-  assert(metrics_);  // metrics_ must not be null.
+  assert(metrics_.get());  // metrics_ must not be null.
   crypto_metrics_ = metrics_->GetCryptoMetrics();
   crypto_session_.reset(CryptoSession::MakeCryptoSession(crypto_metrics_));
   life_span_.Start();
@@ -67,8 +67,8 @@ CdmSession::~CdmSession() {
   }
   Properties::RemoveSessionPropertySet(session_id_);
 
-  if (metrics_) {
-    M_RECORD(metrics_, cdm_session_life_span_, life_span_.AsMs());
+  if (metrics_.get()) {
+    M_RECORD(metrics_.get(), cdm_session_life_span_, life_span_.AsMs());
     metrics_->SetCompleted();
   }
 }

libwvdrmengine/cdm/core/test/cdm_session_unittest.cpp

@@ -174,7 +174,8 @@ class CdmSessionTest : public WvCdmTestBase {
  protected:
   void SetUp() override {
     WvCdmTestBase::SetUp();
-    cdm_session_.reset(new CdmSession(NULL, &metrics_));
+    metrics_.reset(new metrics::SessionMetrics);
+    cdm_session_.reset(new CdmSession(NULL, metrics_));
     // Inject testing mocks.
     license_parser_ = new MockCdmLicense(cdm_session_->session_id());
     cdm_session_->set_license_parser(license_parser_);
@@ -192,7 +193,7 @@ class CdmSessionTest : public WvCdmTestBase {
     cdm_session_.reset();
   }
 
-  metrics::SessionMetrics metrics_;
+  std::shared_ptr<metrics::SessionMetrics> metrics_;
   std::unique_ptr<CdmSession> cdm_session_;
   MockCdmLicense* license_parser_;
   metrics::CryptoMetrics crypto_metrics_;

libwvdrmengine/cdm/metrics/include/metrics_collections.h

@@ -353,9 +353,13 @@ class EngineMetrics {
   ~EngineMetrics();
 
   // Add a new SessionMetrics instance and return a pointer to the caller.
-  // The new SessionMetrics instance is owned by this EngineMetrics instance
-  // and will exist until RemoveSession is called or this object is deleted.
-  SessionMetrics *AddSession();
+  // A shared_ptr is used since it's possible that the SessionMetrics instance
+  // may be in use after the EngineMetrics instance is closed. The EngineMetrics
+  // instance will hold a shared_ptr reference to the SessionMetrics instance
+  // until RemoveSession is called, the EngineMetrics instance is deleted, or
+  // the SessionMetrics instance is marked as completed and ConsolidateSessions
+  // removes it.
+  std::shared_ptr<SessionMetrics> AddSession();
 
   // Removes the metrics object for the given session id. This should only
   // be called when the SessionMetrics instance is no longer in use.

libwvdrmengine/cdm/metrics/include/timer_metric.h

@@ -7,13 +7,14 @@ namespace wvcdm {
 namespace metrics {
 
 class TimerMetric {
-
  public:
+  // Constructs a new TimerMetric.
+  explicit TimerMetric() : is_started_(false) {}
   // Starts the clock running. If the clock was previously set, this resets it.
   // IsStarted will return true after this call.
   void Start();
   // Returns whether or not the timer has started.
-  bool IsStarted() const { return is_started_; };
+  bool IsStarted() const { return is_started_; }
   // Stops the clock and clears the current value.  IsStarted will return false
   // after this call.
   void Clear();
@@ -26,7 +27,6 @@ class TimerMetric {
   std::chrono::steady_clock clock_;
   std::chrono::time_point<std::chrono::steady_clock> start_;
   bool is_started_;
-
 };
 
 }  // namespace metrics

libwvdrmengine/cdm/metrics/src/metrics_collections.cpp

@@ -154,7 +154,7 @@ void CryptoMetrics::Serialize(WvCdmMetrics::CryptoMetrics *crypto_metrics)
             crypto_metrics->mutable_oemcrypto_update_usage_entry());
 }
 
-SessionMetrics::SessionMetrics() {}
+SessionMetrics::SessionMetrics() : session_id_(""), completed_(false) {}
 
 void SessionMetrics::Serialize(WvCdmMetrics::SessionMetrics *session_metrics)
     const {
@@ -268,10 +268,10 @@ EngineMetrics::~EngineMetrics() {
   }
 }
 
-SessionMetrics *EngineMetrics::AddSession() {
+std::shared_ptr<SessionMetrics> EngineMetrics::AddSession() {
   std::unique_lock<std::mutex> lock(session_metrics_lock_);
   active_session_metrics_list_.push_back(std::make_shared<SessionMetrics>());
-  return active_session_metrics_list_.back().get();
+  return active_session_metrics_list_.back();
 }
 
 void EngineMetrics::RemoveSession(wvcdm::CdmSessionId session_id) {

libwvdrmengine/cdm/metrics/test/metrics_collections_unittest.cpp

@@ -156,9 +156,11 @@ TEST_F(EngineMetricsTest, EngineMetricsWithSessions) {
       ->crypto_session_load_certificate_private_key_.Record(2.0, NO_ERROR);
 
   // Create two sessions and record some metrics.
-  SessionMetrics* session_metrics_1 = engine_metrics.AddSession();
+  std::shared_ptr<SessionMetrics> session_metrics_1 =
+      engine_metrics.AddSession();
   session_metrics_1->SetSessionId(kSessionId1);
-  SessionMetrics* session_metrics_2 = engine_metrics.AddSession();
+  std::shared_ptr<SessionMetrics> session_metrics_2 =
+      engine_metrics.AddSession();
   session_metrics_2->SetSessionId(kSessionId2);
   // Record a CryptoMetrics metric in the session.
   session_metrics_2->GetCryptoMetrics()->crypto_session_generic_decrypt_
@@ -184,7 +186,7 @@ TEST_F(EngineMetricsTest, EngineMetricsConsolidateSessions) {
   EngineMetrics engine_metrics;
 
   // Create one more session than the max allowed closed sessions.
-  std::vector<SessionMetrics*> session_metrics;
+  std::vector<std::shared_ptr<SessionMetrics>> session_metrics;
   for (int i = 0; i < kMaxCompletedSessions + 1; i++) {
     session_metrics.push_back(engine_metrics.AddSession());
     session_metrics.back()->SetSessionId(std::to_string(i));
@@ -208,7 +210,7 @@ TEST_F(EngineMetricsTest, EngineMetricsConsolidateSessionsNoClosedSessions) {
   EngineMetrics engine_metrics;
 
   // Create one more session than the max allowed closed sessions.
-  std::vector<SessionMetrics*> session_metrics;
+  std::vector<std::shared_ptr<SessionMetrics>> session_metrics;
   for (int i = 0; i < kMaxCompletedSessions + 1; i++) {
     session_metrics.push_back(engine_metrics.AddSession());
     session_metrics.back()->SetSessionId(std::to_string(i));