Support stopping a perf session in PerfOutputCall.

This CL adds support of stopping a perf session in PerfOutputCall
before the profiler duration elapses. This also changes make
PerfOutputCall to move the profiler output in the callback so that the
user class doesn't need to hold hold the PerfOuptutCall instance alive
when using the profiler output.

Bug: 904785
Test: locally build.
Change-Id: I7ea63962c234a9c6b55d8948e3b98fa496b3784c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1637024
Commit-Queue: Chinglin Yu <chinglinyu@chromium.org>
Reviewed-by: Gabriel Marin <gmx@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671940}

chrome/browser/metrics/perf/perf_events_collector.cc

@@ -392,6 +392,22 @@ MetricCollector::PerfProtoType PerfCollector::GetPerfProtoType(
   return PerfProtoType::PERF_TYPE_UNSUPPORTED;
 }
 
+void PerfCollector::OnPerfOutputComplete(
+    std::unique_ptr<WindowedIncognitoObserver> incognito_observer,
+    std::unique_ptr<SampledProfile> sampled_profile,
+    PerfProtoType type,
+    bool has_cycles,
+    std::string perf_stdout) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // We are done using |perf_output_call_| and may destroy it.
+  perf_output_call_ = nullptr;
+
+  ParseOutputProtoIfValid(std::move(incognito_observer),
+                          std::move(sampled_profile), type, has_cycles,
+                          perf_stdout);
+}
+
 void PerfCollector::ParseOutputProtoIfValid(
     std::unique_ptr<WindowedIncognitoObserver> incognito_observer,
     std::unique_ptr<SampledProfile> sampled_profile,
@@ -400,10 +416,6 @@ void PerfCollector::ParseOutputProtoIfValid(
     const std::string& perf_stdout) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // |perf_output_call_| called us, and owns |perf_stdout|. We must delete it,
-  // but not before parsing |perf_stdout|, and we may return early.
-  std::unique_ptr<PerfOutputCall> call_deleter(std::move(perf_output_call_));
-
   if (incognito_observer->incognito_launched()) {
     AddToUmaHistogram(CollectionAttemptStatus::INCOGNITO_LAUNCHED);
     return;
@@ -481,7 +493,7 @@ void PerfCollector::CollectProfile(
 
   perf_output_call_ = std::make_unique<PerfOutputCall>(
       collection_params_.collection_duration, command,
-      base::BindOnce(&PerfCollector::ParseOutputProtoIfValid,
+      base::BindOnce(&PerfCollector::OnPerfOutputComplete,
                      weak_factory_.GetWeakPtr(), std::move(incognito_observer),
                      std::move(sampled_profile), type, has_cycles));
 }

chrome/browser/metrics/perf/perf_events_collector.h

@@ -35,6 +35,13 @@ class PerfCollector : public MetricCollector {
   // arguments, starting with "perf" itself in |args[0]|.
   static PerfProtoType GetPerfProtoType(const std::vector<std::string>& args);
 
+  void OnPerfOutputComplete(
+      std::unique_ptr<WindowedIncognitoObserver> incognito_observer,
+      std::unique_ptr<SampledProfile> sampled_profile,
+      PerfProtoType type,
+      bool has_cycles,
+      std::string perf_stdout);
+
   // Parses a PerfDataProto or PerfStatProto from serialized data |perf_stdout|,
   // if non-empty. Which proto to use depends on |subcommand|. If |perf_stdout|
   // is empty, it is counted as an error. |incognito_observer| indicates

chrome/browser/metrics/perf/perf_output.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/metrics/perf/perf_output.h"
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/task/post_task.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
@@ -17,6 +18,7 @@ PerfOutputCall::PerfOutputCall(base::TimeDelta duration,
     : duration_(duration),
       perf_args_(perf_args),
       done_callback_(std::move(callback)),
+      pending_stop_(false),
       weak_factory_(this) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
@@ -36,10 +38,27 @@ PerfOutputCall::PerfOutputCall(base::TimeDelta duration,
 
 PerfOutputCall::~PerfOutputCall() {}
 
+void PerfOutputCall::Stop() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  if (!perf_session_id_) {
+    // GetPerfOutputFd hasn't returned the session ID yet. Mark that Stop() has
+    // been called. StopImpl() will be delayed until we receive the session ID.
+    pending_stop_ = true;
+    return;
+  }
+
+  StopImpl();
+}
+
 void PerfOutputCall::OnIOComplete(base::Optional<std::string> result) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   perf_data_pipe_reader_.reset();
-  std::move(done_callback_).Run(result.value_or(std::string()));
+  // Use the r-value variant of base::Optional::value_or() to move |result| to
+  // the callback argument. Callback can safely use |result| after |this| is
+  // deleted.
+  std::move(done_callback_).Run(std::move(result).value_or(std::string()));
   // The callback may delete us, so it's hammertime: Can't touch |this|.
 }
 
@@ -51,6 +70,22 @@ void PerfOutputCall::OnGetPerfOutput(base::Optional<uint64_t> result) {
     perf_data_pipe_reader_.reset();
     std::move(done_callback_).Run(std::string());
   }
+
+  // DBus method GetPerfOutputFd returns a generated session ID back to the
+  // caller. The session ID will be used in stopping the existing perf session.
+  perf_session_id_ = result;
+  if (pending_stop_) {
+    // Stop() is called before GetPerfOutputFd returns the session ID. We can
+    // invoke the StopPerf DBus method now.
+    StopImpl();
+  }
+}
+
+void PerfOutputCall::StopImpl() {
+  DCHECK(perf_session_id_);
+  chromeos::DebugDaemonClient* client =
+      chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
+  client->StopPerf(*perf_session_id_, base::DoNothing());
 }
 
 }  // namespace metrics

chrome/browser/metrics/perf/perf_output.h

@@ -28,18 +28,24 @@ class PerfOutputCall {
   // - Output from "perf record", in PerfDataProto format, OR
   // - Output from "perf stat", in PerfStatProto format, OR
   // - The empty string if there was an error.
-  using DoneCallback = base::OnceCallback<void(const std::string& perf_stdout)>;
+  // The output is transferred to |perf_stdout|.
+  using DoneCallback = base::OnceCallback<void(std::string perf_stdout)>;
 
   PerfOutputCall(base::TimeDelta duration,
                  const std::vector<std::string>& perf_args,
                  DoneCallback callback);
-  ~PerfOutputCall();
+  virtual ~PerfOutputCall();
+
+  // Stop() is made virtual for mocks in testing.
+  virtual void Stop();
 
  private:
   // Internal callbacks.
   void OnIOComplete(base::Optional<std::string> data);
   void OnGetPerfOutput(base::Optional<uint64_t> result);
 
+  void StopImpl();
+
   // Used to capture perf data written to a pipe.
   std::unique_ptr<chromeos::PipeReader> perf_data_pipe_reader_;
 
@@ -48,6 +54,13 @@ class PerfOutputCall {
   std::vector<std::string> perf_args_;
   DoneCallback done_callback_;
 
+  // Whether Stop() is called before OnGetPerfOutput() has returned the session
+  // ID. If true (meaning Stop() is called very soon after we request perf
+  // output), the stop request will be sent out after we have the session ID to
+  // stop the perf session.
+  bool pending_stop_;
+  base::Optional<uint64_t> perf_session_id_;
+
   THREAD_CHECKER(thread_checker_);
 
   // To pass around the "this" pointer across threads safely.

chromeos/dbus/debug_daemon_client.cc

@@ -219,6 +219,18 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
+  void StopPerf(uint64_t session_id, VoidDBusMethodCallback callback) override {
+    DCHECK(session_id);
+    dbus::MethodCall method_call(debugd::kDebugdInterface, debugd::kStopPerf);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendUint64(session_id);
+
+    debugdaemon_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&DebugDaemonClientImpl::OnVoidMethod,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  }
+
   void GetScrubbedBigLogs(GetLogsCallback callback) override {
     // The PipeReaderWrapper is a self-deleting object; we don't have to worry
     // about ownership or lifetime. We need to create a new one for each Big

chromeos/dbus/debug_daemon_client.h

@@ -82,6 +82,13 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) DebugDaemonClient
                              int file_descriptor,
                              DBusMethodCallback<uint64_t> callback) = 0;
 
+  // Stops the perf session identified with |session_id| that was started by a
+  // prior call to GetPerfOutput(), and let the caller of GetPerfOutput() gather
+  // profiling data right away. If the profiler session as identified by
+  // |session_id| has ended, this method will silently succeed.
+  virtual void StopPerf(uint64_t session_id,
+                        VoidDBusMethodCallback callback) = 0;
+
   // Callback type for GetAllLogs()
   using GetLogsCallback =
       base::OnceCallback<void(bool succeeded,

chromeos/dbus/fake_debug_daemon_client.cc

@@ -102,6 +102,9 @@ void FakeDebugDaemonClient::GetPerfOutput(
     int file_descriptor,
     DBusMethodCallback<uint64_t> error_callback) {}
 
+void FakeDebugDaemonClient::StopPerf(uint64_t session_id,
+                                     VoidDBusMethodCallback callback) {}
+
 void FakeDebugDaemonClient::GetScrubbedBigLogs(GetLogsCallback callback) {
   std::map<std::string, std::string> sample;
   sample["Sample Scrubbed Big Log"] = "Your email address is xxxxxxxx";

chromeos/dbus/fake_debug_daemon_client.h

@@ -51,6 +51,7 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeDebugDaemonClient
                      const std::vector<std::string>& perf_args,
                      int file_descriptor,
                      DBusMethodCallback<uint64_t> callback) override;
+  void StopPerf(uint64_t session_id, VoidDBusMethodCallback callback) override;
   void GetScrubbedBigLogs(GetLogsCallback callback) override;
   void GetAllLogs(GetLogsCallback callback) override;
   void GetLog(const std::string& log_name,