Fix dangling pointer in IndependentFlattener

IndependentFlattener keeps a raw_ptr to MetricsLog. For it not to
become dangling, the MetricsLog needs to outlive the flattener, which
is currently not the case. There are a few different problems, as
explained below. In all cases, the IndependentFlattener is indirectly
owned by a MetricsLogHistogramWriter.

- SnapshotUnloggedSamplesAndFinalizeLog(
    MetricsLogHistogramWriter* log_histogram_writer,
    std::unique_ptr<MetricsLog> log, ...)

  This is problematic from an interface standpoint already, since the
  log is destroyed at the end of the function's scope, while the unowned
  log_histogram_writer is kept alive (by whoever owns it on the caller
  side).

- MetricsService::SnapshotDeltasAndFinalizeLog(
    std::unique_ptr<MetricsLogHistogramWriter> log_histogram_writer,
    std::unique_ptr<MetricsLog> log, ...) {
    ...
    return FinalizeLog(std::move(log), ...);
  }

  This is broken for a similar reason: As ownership is transferred to
  FinalizeLog(), log is destroyed when FinalizeLog() returns, but
  log_histogram_writer is only destroyed when
  SnapshotDeltasAndFinalizeLog() returns slightly afterwards.

Both cases are fixed by resetting the MetricsLogHistogramWriter's
flattener before FinalizeLog().

Fixed: 41485234
Change-Id: I60d13e8237d90a09b3cc1e82d43d6c3587610993
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5808942
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1348776}

base/metrics/histogram_snapshot_manager.h

@@ -67,6 +67,10 @@ class BASE_EXPORT HistogramSnapshotManager final {
   void PrepareDelta(HistogramBase* histogram);
   void PrepareFinalDelta(const HistogramBase* histogram);
 
+  // Used to avoid a dangling pointer `histogram_flattener_` if the referenced
+  // `HistogramFlattener` is destroyed first.
+  void ResetFlattener() { histogram_flattener_ = nullptr; }
+
  private:
   FRIEND_TEST_ALL_PREFIXES(HistogramSnapshotManagerTest, CheckMerge);
 
@@ -91,7 +95,7 @@ class BASE_EXPORT HistogramSnapshotManager final {
 
   // |histogram_flattener_| handles the logistics of recording the histogram
   // deltas.
-  const raw_ptr<HistogramFlattener> histogram_flattener_;  // Weak.
+  raw_ptr<HistogramFlattener> histogram_flattener_;  // Weak.
 };
 
 }  // namespace base

components/metrics/metrics_service.cc

@@ -196,7 +196,7 @@ class IndependentFlattener : public base::HistogramFlattener {
   }
 
  private:
-  const raw_ptr<MetricsLog, AcrossTasksDanglingUntriaged> log_;
+  const raw_ptr<MetricsLog> log_;
 };
 
 // Used to mark histogram samples as reported so that they are not included in
@@ -985,13 +985,21 @@ void MetricsService::MetricsLogHistogramWriter::
       required_flags_, histogram_snapshot_manager_.get());
 }
 
+void MetricsService::MetricsLogHistogramWriter::NotifyLogBeingFinalized() {
+  // Since the `flattener_` references the `log`, make sure it is destroyed so
+  // the pointer doesn't become dangling.
+  histogram_snapshot_manager()->ResetFlattener();
+  flattener_.reset();
+}
+
 MetricsService::IndependentMetricsLoader::IndependentMetricsLoader(
     std::unique_ptr<MetricsLog> log,
     std::string app_version,
     std::string signing_key)
     : log_(std::move(log)),
-      flattener_(new IndependentFlattener(log_.get())),
-      snapshot_manager_(new base::HistogramSnapshotManager(flattener_.get())),
+      flattener_(std::make_unique<IndependentFlattener>(log_.get())),
+      snapshot_manager_(
+          std::make_unique<base::HistogramSnapshotManager>(flattener_.get())),
       app_version_(std::move(app_version)),
       signing_key_(std::move(signing_key)) {
   CHECK(log_);
@@ -1654,6 +1662,7 @@ MetricsService::FinalizedLog MetricsService::SnapshotDeltasAndFinalizeLog(
     std::string&& current_app_version,
     std::string&& signing_key) {
   log_histogram_writer->SnapshotStatisticsRecorderDeltas();
+  log_histogram_writer->NotifyLogBeingFinalized();
   return FinalizeLog(std::move(log), truncate_events, std::move(close_time),
                      current_app_version, signing_key);
 }
@@ -1668,6 +1677,7 @@ MetricsService::SnapshotUnloggedSamplesAndFinalizeLog(
     std::string&& current_app_version,
     std::string&& signing_key) {
   log_histogram_writer->SnapshotStatisticsRecorderUnloggedSamples();
+  log_histogram_writer->NotifyLogBeingFinalized();
   return FinalizeLog(std::move(log), truncate_events, std::move(close_time),
                      current_app_version, signing_key);
 }

components/metrics/metrics_service.h

@@ -383,13 +383,18 @@ class MetricsService {
       return snapshot_transaction_id_;
     }
 
+    // Notifies the histogram writer that the `log` passed in through the
+    // constructor is about to be destroyed.
+    void NotifyLogBeingFinalized();
+
    private:
     // Used to select which histograms to record when calling
     // SnapshotStatisticsRecorderHistograms() or
     // SnapshotStatisticsRecorderUnloggedSamples().
     const base::HistogramBase::Flags required_flags_;
 
-    // Used to write histograms to the log passed in the constructor.
+    // Used to write histograms to the log passed in the constructor. Null after
+    // `NotifyLogBeingFinalized()`.
     std::unique_ptr<base::HistogramFlattener> flattener_;
 
     // Used to snapshot histograms.