Attempt to resolve some dangling pointer issues in Telemetry (#1439)

b/298057575
b/298050585

Change-Id: I2f64ad74c667d258eb84105ed15b0d5f8f35105c

cobalt/browser/metrics/cobalt_metrics_log_uploader.cc

@@ -67,8 +67,13 @@ void CobaltMetricsLogUploader::UploadLog(
       base::Base64UrlEncode(cobalt_uma_event.SerializeAsString(),
                             base::Base64UrlEncodePolicy::INCLUDE_PADDING,
                             &base64_encoded_proto);
-      upload_handler_->Run(h5vcc::H5vccMetricType::kH5vccMetricTypeCobaltUma,
-                           base64_encoded_proto);
+      // Check again that the upload handler is still valid. Was seeing race
+      // conditions where it was being destroyed while the proto encoding was
+      // happening above.
+      if (upload_handler_ != nullptr) {
+        upload_handler_->Run(h5vcc::H5vccMetricType::kH5vccMetricTypeCobaltUma,
+                             base64_encoded_proto);
+      }
     }
   }
 

cobalt/browser/metrics/cobalt_metrics_service_client.cc

@@ -58,6 +58,17 @@ void CobaltMetricsServiceClient::SetOnUploadHandler(
   }
 }
 
+void CobaltMetricsServiceClient::RemoveOnUploadHandler(
+    const CobaltMetricsUploaderCallback* uploader_callback) {
+  // Only remove the upload handler if our current reference matches that which
+  // is passed in. Avoids issues with race conditions with two threads trying to
+  // override the handler.
+  if (upload_handler_ == uploader_callback) {
+    LOG(INFO) << "Upload handler removed.";
+    upload_handler_ = nullptr;
+  }
+}
+
 CobaltMetricsServiceClient::CobaltMetricsServiceClient(
     ::metrics::MetricsStateManager* state_manager, PrefService* local_state)
     : metrics_state_manager_(state_manager) {

cobalt/browser/metrics/cobalt_metrics_service_client.h

@@ -53,6 +53,11 @@ class CobaltMetricsServiceClient : public ::metrics::MetricsServiceClient {
   void SetOnUploadHandler(
       const CobaltMetricsUploaderCallback* uploader_callback);
 
+  // Remove reference to the passed uploader callback, if it's the current
+  // reference. Otherwise, does nothing.
+  void RemoveOnUploadHandler(
+      const CobaltMetricsUploaderCallback* uploader_callback);
+
   // Returns the MetricsService instance that this client is associated with.
   // With the exception of testing contexts, the returned instance must be valid
   // for the lifetime of this object (typically, the embedder's client

cobalt/browser/metrics/cobalt_metrics_services_manager.cc

@@ -41,14 +41,40 @@ CobaltMetricsServicesManager* CobaltMetricsServicesManager::GetInstance() {
   return instance_;
 }
 
-void CobaltMetricsServicesManager::DeleteInstance() { delete instance_; }
+void CobaltMetricsServicesManager::DeleteInstance() {
+  delete instance_;
+  instance_ = nullptr;
+}
+
+void CobaltMetricsServicesManager::RemoveOnUploadHandler(
+    const CobaltMetricsUploaderCallback* uploader_callback) {
+  if (instance_ != nullptr) {
+    instance_->task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&CobaltMetricsServicesManager::RemoveOnUploadHandlerInternal,
+                   base::Unretained(instance_), uploader_callback));
+  }
+}
+
+void CobaltMetricsServicesManager::RemoveOnUploadHandlerInternal(
+    const CobaltMetricsUploaderCallback* uploader_callback) {
+  CobaltMetricsServiceClient* client =
+      static_cast<CobaltMetricsServiceClient*>(GetMetricsServiceClient());
+  DCHECK(client);
+  client->RemoveOnUploadHandler(uploader_callback);
+}
 
 void CobaltMetricsServicesManager::SetOnUploadHandler(
     const CobaltMetricsUploaderCallback* uploader_callback) {
-  instance_->task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&CobaltMetricsServicesManager::SetOnUploadHandlerInternal,
-                 base::Unretained(instance_), uploader_callback));
+  // H5vccMetrics calls this on destruction when the WebModule is torn down. On
+  // shutdown, CobaltMetricsServicesManager can be destructed before
+  // H5vccMetrics, so we make sure we have a valid instance here.
+  if (instance_ != nullptr) {
+    instance_->task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&CobaltMetricsServicesManager::SetOnUploadHandlerInternal,
+                   base::Unretained(instance_), uploader_callback));
+  }
 }
 
 void CobaltMetricsServicesManager::SetOnUploadHandlerInternal(

cobalt/browser/metrics/cobalt_metrics_services_manager.h

@@ -59,6 +59,14 @@ class CobaltMetricsServicesManager
   static void SetOnUploadHandler(
       const CobaltMetricsUploaderCallback* uploader_callback);
 
+  // Attempts to clean up the passed reference to CobaltMetricsUploaderCallback,
+  // IFF it matches the current callback reference in
+  // CobaltMetricsServiceManager. This is to avoid situations where two clients
+  // are competing to override the upload handler and prevent one from
+  // inadvertently clobbering another.
+  static void RemoveOnUploadHandler(
+      const CobaltMetricsUploaderCallback* uploader_callback);
+
   // Toggles whether metric reporting is enabled via
   // CobaltMetricsServicesManager.
   static void ToggleMetricsEnabled(bool is_enabled);
@@ -71,6 +79,9 @@ class CobaltMetricsServicesManager
   void SetOnUploadHandlerInternal(
       const CobaltMetricsUploaderCallback* uploader_callback);
 
+  void RemoveOnUploadHandlerInternal(
+      const CobaltMetricsUploaderCallback* uploader_callback);
+
   void ToggleMetricsEnabledInternal(bool is_enabled);
 
   void SetUploadIntervalInternal(uint32_t interval_seconds);

cobalt/h5vcc/h5vcc_metrics.cc

@@ -25,6 +25,17 @@
 namespace cobalt {
 namespace h5vcc {
 
+H5vccMetrics::~H5vccMetrics() {
+  if (browser::metrics::CobaltMetricsServicesManager::GetInstance() !=
+          nullptr &&
+      run_event_handler_callback_) {
+    // We need to let the metrics manager know not to call the upload callback
+    // any longer, otherwise it could crash.
+    browser::metrics::CobaltMetricsServicesManager::GetInstance()
+        ->RemoveOnUploadHandler(run_event_handler_callback_.get());
+  }
+}
+
 void H5vccMetrics::OnMetricEvent(
     const h5vcc::MetricEventHandlerWrapper::ScriptValue& event_handler) {
   if (!uploader_callback_) {
@@ -43,16 +54,20 @@ void H5vccMetrics::OnMetricEvent(
 void H5vccMetrics::RunEventHandler(
     const cobalt::h5vcc::H5vccMetricType& metric_type,
     const std::string& serialized_proto) {
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&H5vccMetrics::RunEventHandlerInternal, base::Unretained(this),
-                 metric_type, serialized_proto));
+  if (task_runner_ && task_runner_->HasAtLeastOneRef()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&H5vccMetrics::RunEventHandlerInternal,
+                   base::Unretained(this), metric_type, serialized_proto));
+  }
 }
 
 void H5vccMetrics::RunEventHandlerInternal(
     const cobalt::h5vcc::H5vccMetricType& metric_type,
     const std::string& serialized_proto) {
-  uploader_callback_->callback.value().Run(metric_type, serialized_proto);
+  if (uploader_callback_ != nullptr && uploader_callback_->HasAtLeastOneRef()) {
+    uploader_callback_->callback.value().Run(metric_type, serialized_proto);
+  }
 }
 
 void H5vccMetrics::Enable() { ToggleMetricsEnabled(true); }

cobalt/h5vcc/h5vcc_metrics.h

@@ -47,6 +47,8 @@ class H5vccMetrics : public script::Wrappable {
       : task_runner_(base::ThreadTaskRunnerHandle::Get()),
         persistent_settings_(persistent_settings) {}
 
+  ~H5vccMetrics();
+
   H5vccMetrics(const H5vccMetrics&) = delete;
   H5vccMetrics& operator=(const H5vccMetrics&) = delete;