From 05599927b9d276ff0151ef758385031b94d7cefc Mon Sep 17 00:00:00 2001 From: Adam Stone Date: Tue, 29 Jan 2019 11:27:21 -0800 Subject: [PATCH] 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 | 7 ++++--- libwvdrmengine/cdm/core/src/cdm_engine.cpp | 4 ++-- libwvdrmengine/cdm/core/src/cdm_session.cpp | 8 ++++---- libwvdrmengine/cdm/core/test/cdm_session_unittest.cpp | 5 +++-- .../cdm/metrics/include/metrics_collections.h | 10 +++++++--- libwvdrmengine/cdm/metrics/include/timer_metric.h | 6 +++--- libwvdrmengine/cdm/metrics/src/metrics_collections.cpp | 6 +++--- .../cdm/metrics/test/metrics_collections_unittest.cpp | 10 ++++++---- 8 files changed, 32 insertions(+), 24 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/cdm_session.h b/libwvdrmengine/cdm/core/include/cdm_session.h index 98032520..c518c582 100644 --- a/libwvdrmengine/cdm/core/include/cdm_session.h +++ b/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); 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_; metrics::CryptoMetrics* crypto_metrics_; metrics::TimerMetric life_span_; metrics::TimerMetric license_request_latency_; diff --git a/libwvdrmengine/cdm/core/src/cdm_engine.cpp b/libwvdrmengine/cdm/core/src/cdm_engine.cpp index be832457..feff5702 100644 --- a/libwvdrmengine/cdm/core/src/cdm_engine.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_engine.cpp @@ -133,8 +133,8 @@ CdmResponseType CdmEngine::OpenSession( CloseExpiredReleaseSessions(); - std::unique_ptr new_session(new CdmSession(file_system_, - metrics_->AddSession())); + std::unique_ptr new_session( + new CdmSession(file_system_, metrics_->AddSession())); CdmResponseType sts = new_session->Init(property_set, forced_session_id, event_listener); if (sts != NO_ERROR) { diff --git a/libwvdrmengine/cdm/core/src/cdm_session.cpp b/libwvdrmengine/cdm/core/src/cdm_session.cpp index 5a6e668b..90260800 100644 --- a/libwvdrmengine/cdm/core/src/cdm_session.cpp +++ b/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) : 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(); } } diff --git a/libwvdrmengine/cdm/core/test/cdm_session_unittest.cpp b/libwvdrmengine/cdm/core/test/cdm_session_unittest.cpp index 1b825a05..f7cb1827 100644 --- a/libwvdrmengine/cdm/core/test/cdm_session_unittest.cpp +++ b/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_; std::unique_ptr cdm_session_; MockCdmLicense* license_parser_; metrics::CryptoMetrics crypto_metrics_; diff --git a/libwvdrmengine/cdm/metrics/include/metrics_collections.h b/libwvdrmengine/cdm/metrics/include/metrics_collections.h index b9e70b46..122320d6 100644 --- a/libwvdrmengine/cdm/metrics/include/metrics_collections.h +++ b/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 AddSession(); // Removes the metrics object for the given session id. This should only // be called when the SessionMetrics instance is no longer in use. diff --git a/libwvdrmengine/cdm/metrics/include/timer_metric.h b/libwvdrmengine/cdm/metrics/include/timer_metric.h index ace35370..cc5dbe10 100644 --- a/libwvdrmengine/cdm/metrics/include/timer_metric.h +++ b/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 start_; bool is_started_; - }; } // namespace metrics diff --git a/libwvdrmengine/cdm/metrics/src/metrics_collections.cpp b/libwvdrmengine/cdm/metrics/src/metrics_collections.cpp index 601debb7..afa301af 100644 --- a/libwvdrmengine/cdm/metrics/src/metrics_collections.cpp +++ b/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 EngineMetrics::AddSession() { std::unique_lock lock(session_metrics_lock_); active_session_metrics_list_.push_back(std::make_shared()); - return active_session_metrics_list_.back().get(); + return active_session_metrics_list_.back(); } void EngineMetrics::RemoveSession(wvcdm::CdmSessionId session_id) { diff --git a/libwvdrmengine/cdm/metrics/test/metrics_collections_unittest.cpp b/libwvdrmengine/cdm/metrics/test/metrics_collections_unittest.cpp index 84ad930b..2c29907e 100644 --- a/libwvdrmengine/cdm/metrics/test/metrics_collections_unittest.cpp +++ b/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 session_metrics_1 = + engine_metrics.AddSession(); session_metrics_1->SetSessionId(kSessionId1); - SessionMetrics* session_metrics_2 = engine_metrics.AddSession(); + std::shared_ptr 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 session_metrics; + std::vector> 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 session_metrics; + std::vector> 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));