libchrome_tools/patches/backward-compatibility-0200-Revert-Return-nullopt-on-error-from-ProcessMetrics-C.patch

From 1793b7e0837efdf44ba08e9289ac0d24727a1947 Mon Sep 17 00:00:00 2001
From: Nathan Muggli <nmuggli@google.com>
Date: Wed, 13 Mar 2024 16:07:37 -0600
Subject: [PATCH] Revert "Return nullopt on error from ProcessMetrics CPU
 measurements"

This reverts commit 94912573bfad4a687bc86cbbcbafad7ec0dad19e.
---
 base/process/internal_linux.cc           | 15 ++---
 base/process/internal_linux.h            |  9 ---
 base/process/process_metrics.cc          |  8 +--
 base/process/process_metrics.h           | 15 ++---
 base/process/process_metrics_apple.cc    | 14 ++---
 base/process/process_metrics_linux.cc    | 56 ++++++++----------
 base/process/process_metrics_unittest.cc | 72 ++++++++++++++++--------
 ipc/ipc_cpu_perftest.cc                  |  6 +-
 8 files changed, 90 insertions(+), 105 deletions(-)

diff --git a/base/process/internal_linux.cc b/base/process/internal_linux.cc
index c8f38dc28c..b9d3f0f851 100644
--- a/base/process/internal_linux.cc
+++ b/base/process/internal_linux.cc
@@ -14,7 +14,6 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/notreached.h"
-#include "base/numerics/safe_conversions.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ -207,17 +206,10 @@ void ParseProcStat(const std::string& contents, ProcStatMap* output) {
 int64_t GetProcStatsFieldAsInt64(const std::vector<std::string>& proc_stats,
                                  ProcStatsFields field_num) {
   DCHECK_GE(field_num, VM_PPID);
-  return GetProcStatsFieldAsOptionalInt64(proc_stats, field_num).value_or(0);
-}
+  CHECK_LT(static_cast<size_t>(field_num), proc_stats.size());
 
-std::optional<int64_t> GetProcStatsFieldAsOptionalInt64(
-    base::span<const std::string> proc_stats,
-    ProcStatsFields field_num) {
   int64_t value;
-  if (StringToInt64(proc_stats[size_t{field_num}], &value)) {
-    return value;
-  }
-  return std::nullopt;
+  return StringToInt64(proc_stats[field_num], &value) ? value : 0;
 }
 
 size_t GetProcStatsFieldAsSizeT(const std::vector<std::string>& proc_stats,
@@ -250,7 +242,8 @@ int64_t ReadProcSelfStatsAndGetFieldAsInt64(ProcStatsFields field_num) {
   return ReadStatFileAndGetFieldAsInt64(stat_file, field_num);
 }
 
-size_t ReadProcStatsAndGetFieldAsSizeT(pid_t pid, ProcStatsFields field_num) {
+size_t ReadProcStatsAndGetFieldAsSizeT(pid_t pid,
+                                       ProcStatsFields field_num) {
   std::string stats_data;
   if (!ReadProcStats(pid, &stats_data))
     return 0;
diff --git a/base/process/internal_linux.h b/base/process/internal_linux.h
index e7b70601bc..a2657a7bc3 100644
--- a/base/process/internal_linux.h
+++ b/base/process/internal_linux.h
@@ -11,11 +11,9 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <unistd.h>
-#include <optional>
 #include <string>
 #include <vector>
 
-#include "base/containers/span.h"
 #include "base/files/dir_reader_posix.h"
 #include "base/files/file_path.h"
 #include "base/process/process_handle.h"
@@ -102,13 +100,6 @@ enum ProcStatsFields {
 int64_t GetProcStatsFieldAsInt64(const std::vector<std::string>& proc_stats,
                                  ProcStatsFields field_num);
 
-// Reads the `field_num`th field from `proc_stats`. Asserts that `field_num` is
-// a valid index into `proc_stats`. Returns nullopt if the field doesn't contain
-// a valid integer.
-std::optional<int64_t> GetProcStatsFieldAsOptionalInt64(
-    base::span<const std::string> proc_stats,
-    ProcStatsFields field_num);
-
 // Same as GetProcStatsFieldAsInt64(), but for size_t values.
 size_t GetProcStatsFieldAsSizeT(const std::vector<std::string>& proc_stats,
                                 ProcStatsFields field_num);
diff --git a/base/process/process_metrics.cc b/base/process/process_metrics.cc
index e9d90dcad8..e59252c6e3 100644
--- a/base/process/process_metrics.cc
+++ b/base/process/process_metrics.cc
@@ -123,12 +123,8 @@ double ProcessMetrics::GetPlatformIndependentCPUUsage(
   return 100.0 * cpu_time_delta / time_delta;
 }
 
-std::optional<double> ProcessMetrics::GetPlatformIndependentCPUUsage() {
-  const std::optional<TimeDelta> cpu_usage = GetCumulativeCPUUsage();
-  if (!cpu_usage.has_value()) {
-    return std::nullopt;
-  }
-  return GetPlatformIndependentCPUUsage(cpu_usage.value());
+double ProcessMetrics::GetPlatformIndependentCPUUsage() {
+  return GetPlatformIndependentCPUUsage(GetCumulativeCPUUsage());
 }
 #endif
 
diff --git a/base/process/process_metrics.h b/base/process/process_metrics.h
index db4ea9d024..b9688fe6ed 100644
--- a/base/process/process_metrics.h
+++ b/base/process/process_metrics.h
@@ -12,7 +12,6 @@
 #include <stdint.h>
 
 #include <memory>
-#include <optional>
 
 #include "base/base_export.h"
 #include "base/gtest_prod_util.h"
@@ -125,16 +124,14 @@ class BASE_EXPORT ProcessMetrics {
   [[nodiscard]] double GetPlatformIndependentCPUUsage(TimeDelta cumulative_cpu);
 
   // Same as the above, but automatically calls GetCumulativeCPUUsage() to
-  // determine the current cumulative CPU. Returns nullopt if
-  // GetCumulativeCPUUsage() fails.
-  [[nodiscard]] std::optional<double> GetPlatformIndependentCPUUsage();
+  // determine the current cumulative CPU.
+  [[nodiscard]] double GetPlatformIndependentCPUUsage();
 
   // Returns the cumulative CPU usage across all threads of the process since
-  // process start, or nullopt on error. In case of multi-core processors, a
-  // process can consume CPU at a rate higher than wall-clock time, e.g. two
-  // cores at full utilization will result in a time delta of 2 seconds/per 1
-  // wall-clock second.
-  [[nodiscard]] std::optional<TimeDelta> GetCumulativeCPUUsage();
+  // process start. In case of multi-core processors, a process can consume CPU
+  // at a rate higher than wall-clock time, e.g. two cores at full utilization
+  // will result in a time delta of 2 seconds/per 1 wall-clock second.
+  [[nodiscard]] TimeDelta GetCumulativeCPUUsage();
 
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_ANDROID) || \
     BUILDFLAG(IS_AIX)
diff --git a/base/process/process_metrics_apple.cc b/base/process/process_metrics_apple.cc
index 685e0c4360..1a23c8a083 100644
--- a/base/process/process_metrics_apple.cc
+++ b/base/process/process_metrics_apple.cc
@@ -11,8 +11,6 @@
 #include <stdint.h>
 #include <sys/sysctl.h>
 
-#include <optional>
-
 #include "base/apple/mach_logging.h"
 #include "base/apple/scoped_mach_port.h"
 #include "base/logging.h"
@@ -95,10 +93,10 @@ mach_port_t ProcessMetrics::TaskForHandle(ProcessHandle process_handle) const {
   return task;
 }
 
-std::optional<TimeDelta> ProcessMetrics::GetCumulativeCPUUsage() {
+TimeDelta ProcessMetrics::GetCumulativeCPUUsage() {
   mach_port_t task = TaskForHandle(process_);
   if (task == MACH_PORT_NULL) {
-    return std::nullopt;
+    return TimeDelta();
   }
 
   // Libtop explicitly loops over the threads (libtop_pinfo_update_cpu_usage()
@@ -110,12 +108,12 @@ std::optional<TimeDelta> ProcessMetrics::GetCumulativeCPUUsage() {
                                &thread_info_count);
   if (kr != KERN_SUCCESS) {
     // Most likely cause: |task| is a zombie.
-    return std::nullopt;
+    return TimeDelta();
   }
 
   task_basic_info_64 task_info_data;
   if (!GetTaskInfo(task, &task_info_data)) {
-    return std::nullopt;
+    return TimeDelta();
   }
 
   /* Set total_time. */
@@ -139,10 +137,10 @@ std::optional<TimeDelta> ProcessMetrics::GetCumulativeCPUUsage() {
     // a lag before it shows up in the terminated thread times returned by
     // GetTaskInfo(). Make sure CPU usage doesn't appear to go backwards if
     // GetCumulativeCPUUsage() is called in the interval.
-    return std::optional(last_measured_cpu_);
+    return last_measured_cpu_;
   }
   last_measured_cpu_ = measured_cpu;
-  return std::optional(measured_cpu);
+  return measured_cpu;
 }
 
 int ProcessMetrics::GetPackageIdleWakeupsPerSecond() {
diff --git a/base/process/process_metrics_linux.cc b/base/process/process_metrics_linux.cc
index 0ed005ff09..5d11ac6638 100644
--- a/base/process/process_metrics_linux.cc
+++ b/base/process/process_metrics_linux.cc
@@ -17,7 +17,6 @@
 #include <optional>
 #include <utility>
 
-#include "base/containers/span.h"
 #include "base/cpu.h"
 #include "base/files/dir_reader_posix.h"
 #include "base/files/file_util.h"
@@ -58,26 +57,24 @@ uint64_t ReadFileToUint64(const FilePath& file) {
 }
 #endif
 
-// Get the total CPU from a proc stat buffer.  Return value is a TimeDelta
-// converted from a number of jiffies on success or nullopt if parsing failed.
-std::optional<TimeDelta> ParseTotalCPUTimeFromStats(
-    base::span<const std::string> proc_stats) {
-  const std::optional<int64_t> utime =
-      internal::GetProcStatsFieldAsOptionalInt64(proc_stats,
-                                                 internal::VM_UTIME);
-  if (utime.value_or(-1) < 0) {
-    return std::nullopt;
-  }
-  const std::optional<int64_t> stime =
-      internal::GetProcStatsFieldAsOptionalInt64(proc_stats,
-                                                 internal::VM_UTIME);
-  if (stime.value_or(-1) < 0) {
-    return std::nullopt;
+// Get the total CPU from a proc stat buffer.  Return value is number of jiffies
+// on success or 0 if parsing failed.
+int64_t ParseTotalCPUTimeFromStats(const std::vector<std::string>& proc_stats) {
+  return internal::GetProcStatsFieldAsInt64(proc_stats, internal::VM_UTIME) +
+         internal::GetProcStatsFieldAsInt64(proc_stats, internal::VM_STIME);
+}
+
+// Get the total CPU of a single process.  Return value is number of jiffies
+// on success or -1 on error.
+int64_t GetProcessCPU(pid_t pid) {
+  std::string buffer;
+  std::vector<std::string> proc_stats;
+  if (!internal::ReadProcStats(pid, &buffer) ||
+      !internal::ParseProcStats(buffer, &proc_stats)) {
+    return -1;
   }
-  const TimeDelta cpu_time = internal::ClockTicksToTimeDelta(
-      base::ClampAdd(utime.value(), stime.value()));
-  CHECK(!cpu_time.is_negative());
-  return std::optional(cpu_time);
+
+  return ParseTotalCPUTimeFromStats(proc_stats);
 }
 
 }  // namespace
@@ -93,15 +90,8 @@ size_t ProcessMetrics::GetResidentSetSize() const {
          checked_cast<size_t>(getpagesize());
 }
 
-std::optional<TimeDelta> ProcessMetrics::GetCumulativeCPUUsage() {
-  std::string buffer;
-  std::vector<std::string> proc_stats;
-  if (!internal::ReadProcStats(process_, &buffer) ||
-      !internal::ParseProcStats(buffer, &proc_stats)) {
-    return std::nullopt;
-  }
-
-  return ParseTotalCPUTimeFromStats(proc_stats);
+TimeDelta ProcessMetrics::GetCumulativeCPUUsage() {
+  return internal::ClockTicksToTimeDelta(GetProcessCPU(process_));
 }
 
 bool ProcessMetrics::GetCumulativeCPUUsagePerThread(
@@ -120,11 +110,9 @@ bool ProcessMetrics::GetCumulativeCPUUsagePerThread(
           return;
         }
 
-        const std::optional<TimeDelta> thread_time =
-            ParseTotalCPUTimeFromStats(proc_stats);
-        if (thread_time.has_value()) {
-          cpu_per_thread.emplace_back(tid, thread_time.value());
-        }
+        TimeDelta thread_time = internal::ClockTicksToTimeDelta(
+            ParseTotalCPUTimeFromStats(proc_stats));
+        cpu_per_thread.emplace_back(tid, thread_time);
       });
 
   return !cpu_per_thread.empty();
diff --git a/base/process/process_metrics_unittest.cc b/base/process/process_metrics_unittest.cc
index 230d22499e..84baef0f11 100644
--- a/base/process/process_metrics_unittest.cc
+++ b/base/process/process_metrics_unittest.cc
@@ -8,7 +8,6 @@
 #include <stdint.h>
 
 #include <memory>
-#include <optional>
 #include <sstream>
 #include <string>
 #include <utility>
@@ -37,7 +36,6 @@
 #include "build/blink_buildflags.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/multiprocess_func_list.h"
 
@@ -66,12 +64,6 @@ namespace base::debug {
 
 namespace {
 
-using ::testing::AssertionFailure;
-using ::testing::AssertionResult;
-using ::testing::AssertionSuccess;
-using ::testing::Ge;
-using ::testing::Optional;
-
 #if ENABLE_CPU_TESTS
 
 void BusyWork(std::vector<std::string>* vec) {
@@ -82,21 +74,19 @@ void BusyWork(std::vector<std::string>* vec) {
   }
 }
 
-// Tests that GetCumulativeCPUUsage() returns a valid result that's equal to or
-// greater than `prev_cpu_usage`, and returns the result. If
-// GetCumulativeCPUUsage() returns an error, records a failed expectation and
-// returns an empty TimeDelta so that each test doesn't need to check for
-// nullopt repeatedly.
 TimeDelta TestCumulativeCPU(ProcessMetrics* metrics, TimeDelta prev_cpu_usage) {
-  const std::optional<TimeDelta> current_cpu_usage =
-      metrics->GetCumulativeCPUUsage();
-  EXPECT_THAT(current_cpu_usage, Optional(Ge(prev_cpu_usage)));
-  EXPECT_THAT(metrics->GetPlatformIndependentCPUUsage(), Optional(Ge(0.0)));
-  return current_cpu_usage.value_or(TimeDelta());
+  const TimeDelta current_cpu_usage = metrics->GetCumulativeCPUUsage();
+  EXPECT_GE(current_cpu_usage, prev_cpu_usage);
+  EXPECT_GE(metrics->GetPlatformIndependentCPUUsage(), 0.0);
+  return current_cpu_usage;
 }
 
 #endif  // ENABLE_CPU_TESTS
 
+using ::testing::AssertionFailure;
+using ::testing::AssertionResult;
+using ::testing::AssertionSuccess;
+
 // Helper to deal with Mac process launching complexity. On other platforms this
 // is just a thin wrapper around SpawnMultiProcessTestChild.
 class TestChildLauncher {
@@ -634,7 +624,7 @@ TEST_F(SystemMetricsTest, TestNoNegativeCpuUsage) {
   std::unique_ptr<ProcessMetrics> metrics =
       ProcessMetrics::CreateCurrentProcessMetrics();
 
-  EXPECT_THAT(metrics->GetPlatformIndependentCPUUsage(), Optional(Ge(0.0)));
+  EXPECT_GE(metrics->GetPlatformIndependentCPUUsage(), 0.0);
 
   Thread thread1("thread1");
   Thread thread2("thread2");
@@ -697,15 +687,38 @@ TEST_F(SystemMetricsTest, MeasureChildCpuUsage) {
 #if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_FUCHSIA)
   // Windows and Fuchsia return final measurements of a process after it exits.
   TestCumulativeCPU(metrics.get(), cpu_usage2);
+#elif BUILDFLAG(IS_APPLE)
+  // Mac and iOS return 0 on error. GetPlatformIndependentCPUUsage subtracts
+  // from this to get a negative.
+  EXPECT_EQ(metrics->GetCumulativeCPUUsage(), TimeDelta());
+  EXPECT_LT(metrics->GetPlatformIndependentCPUUsage(), 0.0);
 #else
-  // All other platforms return an error.
-  EXPECT_EQ(metrics->GetCumulativeCPUUsage(), std::nullopt);
-  EXPECT_EQ(metrics->GetPlatformIndependentCPUUsage(), std::nullopt);
+  // All other platforms return a negative time value to indicate an error.
+  EXPECT_LT(metrics->GetCumulativeCPUUsage(), TimeDelta());
+  EXPECT_LT(metrics->GetPlatformIndependentCPUUsage(), 0.0);
 #endif
 }
 
 #endif  // !BUILDFLAG(IS_APPLE) || BUILDFLAG(USE_BLINK)
 
+#if BUILDFLAG(IS_FUCHSIA)
+
+// Fuchsia CHECK's when measuring an invalid procses.
+using SystemMetricsDeathTest = SystemMetricsTest;
+TEST_F(SystemMetricsDeathTest, InvalidProcessCpuUsage) {
+  std::unique_ptr<ProcessMetrics> metrics =
+      ProcessMetrics::CreateProcessMetrics(kNullProcessHandle);
+  EXPECT_CHECK_DEATH(
+      { [[maybe_unused]] TimeDelta usage = metrics->GetCumulativeCPUUsage(); });
+  EXPECT_CHECK_DEATH({
+    [[maybe_unused]] double usage = metrics->GetPlatformIndependentCPUUsage();
+  });
+}
+
+#else  // !BUILDFLAG(IS_FUCHSIA)
+
+// Windows, Mac and iOS also return 0 on error. All other platforms return a
+// negative value to indicate an error.
 TEST_F(SystemMetricsTest, InvalidProcessCpuUsage) {
 #if BUILDFLAG(IS_MAC)
   std::unique_ptr<ProcessMetrics> metrics =
@@ -714,9 +727,20 @@ TEST_F(SystemMetricsTest, InvalidProcessCpuUsage) {
   std::unique_ptr<ProcessMetrics> metrics =
       ProcessMetrics::CreateProcessMetrics(kNullProcessHandle);
 #endif
-  EXPECT_EQ(metrics->GetCumulativeCPUUsage(), std::nullopt);
-  EXPECT_EQ(metrics->GetPlatformIndependentCPUUsage(), std::nullopt);
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_APPLE)
+  EXPECT_EQ(metrics->GetCumulativeCPUUsage(), TimeDelta());
+#else
+  EXPECT_LT(metrics->GetCumulativeCPUUsage(), TimeDelta());
+#endif
+
+  // The first call always caches the result of GetCumulativeCPUUsage() and
+  // returns 0, even if it's an error value. The second call returns the delta
+  // with the cached result, which is 0 when the second GetCumulativeCPUUsage()
+  // call returns the same error value.
+  EXPECT_EQ(metrics->GetPlatformIndependentCPUUsage(), 0.0);
+  EXPECT_EQ(metrics->GetPlatformIndependentCPUUsage(), 0.0);
 }
+#endif  // !BUILDFLAG(IS_FUCHSIA)
 
 #endif  // ENABLE_CPU_TESTS
 
diff --git a/ipc/ipc_cpu_perftest.cc b/ipc/ipc_cpu_perftest.cc
index dacbdc987b..a58d6b401a 100644
--- a/ipc/ipc_cpu_perftest.cc
+++ b/ipc/ipc_cpu_perftest.cc
@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #include <memory>
-#include <optional>
 #include <string_view>
 #include <tuple>
 
@@ -74,7 +73,7 @@ class PerfCpuLogger {
         process_metrics_(base::ProcessMetrics::CreateCurrentProcessMetrics()) {
     // Query the CPU usage once to start the recording interval.
     const double inital_cpu_usage =
-        process_metrics_->GetPlatformIndependentCPUUsage().value_or(-1.0);
+        process_metrics_->GetPlatformIndependentCPUUsage();
     // This should have been the first call so the reported cpu usage should be
     // exactly zero.
     DCHECK_EQ(inital_cpu_usage, 0.0);
@@ -84,8 +83,7 @@ class PerfCpuLogger {
   PerfCpuLogger& operator=(const PerfCpuLogger&) = delete;
 
   ~PerfCpuLogger() {
-    const double result =
-        process_metrics_->GetPlatformIndependentCPUUsage().value_or(-1.0);
+    double result = process_metrics_->GetPlatformIndependentCPUUsage();
     base::LogPerfResult(test_name_.c_str(), result, "%");
   }
 
-- 
2.44.0.278.ge034bb2e1d-goog