Adapt dumpstate to perfetto-based SurfaceFlinger tracing

- Include system trace (perfetto) into the pre-dumped data.
- Snapshot the system trace (perfetto) first to avoid
  that dumpsys critical's activity pushes out interesting
  data from the trace ring buffer.

Bug: b/293429094
Bug: b/296650898
Test: atest dumpstate_test && \
    atest com.android.os.bugreports.tests.BugreportManagerTest
Ignore-AOSP-First: depends on changes (surfaceflinger) that cannot go into AOSP
Change-Id: Iccf9f92989c317a29ffa4c806afd6eab1da7976a

3 files changed, 31 insertions, 61 deletions

diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 33f6d38cc6..ea3090349e 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -1088,8 +1088,14 @@ static void MaybeAddSystemTraceToZip() {
     // This function copies into the .zip the system trace that was snapshotted
     // by the early call to MaybeSnapshotSystemTrace(), if any background
     // tracing was happening.
-    if (!ds.has_system_trace_) {
-        // No background trace was happening at the time dumpstate was invoked.
+    bool system_trace_exists = access(SYSTEM_TRACE_SNAPSHOT, F_OK) == 0;
+    if (!system_trace_exists) {
+        // No background trace was happening at the time MaybeSnapshotSystemTrace() was invoked.
+        if (!PropertiesHelper::IsUserBuild()) {
+            MYLOGI(
+                "No system traces found. Check for previously uploaded traces by looking for "
+                "go/trace-uuid in logcat")
+        }
         return;
     }
     ds.AddZipEntry(
@@ -1649,8 +1655,6 @@ Dumpstate::RunStatus Dumpstate::dumpstate() {
         dump_board = ds.dump_pool_->enqueueTaskWithFd(
             DUMP_BOARD_TASK, &Dumpstate::DumpstateBoard, &ds, _1);
         dump_checkins = ds.dump_pool_->enqueueTaskWithFd(DUMP_CHECKINS_TASK, &DumpCheckins, _1);
-        post_process_ui_traces = ds.dump_pool_->enqueueTask(
-            POST_PROCESS_UI_TRACES_TASK, &Dumpstate::MaybePostProcessUiTraces, &ds);
     }
 
     // Dump various things. Note that anything that takes "long" (i.e. several seconds) should
@@ -1860,12 +1864,6 @@ Dumpstate::RunStatus Dumpstate::dumpstate() {
                 DumpIncidentReport);
     }
 
-    if (ds.dump_pool_) {
-        WaitForTask(std::move(post_process_ui_traces));
-    } else {
-        RUN_SLOW_FUNCTION_AND_LOG(POST_PROCESS_UI_TRACES_TASK, MaybePostProcessUiTraces);
-    }
-
     MaybeAddUiTracesToZip();
 
     return Dumpstate::RunStatus::OK;
@@ -3071,6 +3069,7 @@ void Dumpstate::Cancel() {
 }
 
 void Dumpstate::PreDumpUiData() {
+    MaybeSnapshotSystemTrace();
     MaybeSnapshotUiTraces();
 }
 
@@ -3257,25 +3256,23 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid,
     // duration is logged into MYLOG instead.
     PrintHeader();
 
-    bool is_dumpstate_restricted = options_->telephony_only
-                                   || options_->wifi_only
-                                   || options_->limited_only;
-    if (!is_dumpstate_restricted) {
-        // Invoke critical dumpsys first to preserve system state, before doing anything else.
-        RunDumpsysCritical();
-    }
-    MaybeTakeEarlyScreenshot();
-
+    bool is_dumpstate_restricted =
+        options_->telephony_only || options_->wifi_only || options_->limited_only;
     if (!is_dumpstate_restricted) {
         // Snapshot the system trace now (if running) to avoid that dumpstate's
         // own activity pushes out interesting data from the trace ring buffer.
         // The trace file is added to the zip by MaybeAddSystemTraceToZip().
         MaybeSnapshotSystemTrace();
 
+        // Invoke critical dumpsys to preserve system state, before doing anything else.
+        RunDumpsysCritical();
+
         // Snapshot the UI traces now (if running).
         // The trace files will be added to bugreport later.
         MaybeSnapshotUiTraces();
     }
+
+    MaybeTakeEarlyScreenshot();
     onUiIntensiveBugreportDumpsFinished(calling_uid);
     MaybeCheckUserConsent(calling_uid, calling_package);
     if (options_->telephony_only) {
@@ -3372,6 +3369,19 @@ void Dumpstate::MaybeTakeEarlyScreenshot() {
 }
 
 void Dumpstate::MaybeSnapshotSystemTrace() {
+    // When capturing traces via bugreport handler (BH), this function will be invoked twice:
+    // 1) When BH invokes IDumpstate::PreDumpUiData()
+    // 2) When BH invokes IDumpstate::startBugreport(flags = BUGREPORT_USE_PREDUMPED_UI_DATA)
+    // In this case we don't want to re-invoke perfetto in step 2.
+    // In all other standard invocation states, this function is invoked once
+    // without the flag BUGREPORT_USE_PREDUMPED_UI_DATA.
+    if (options_->use_predumped_ui_data) {
+        return;
+    }
+
+    // If a stale file exists already, remove it.
+    unlink(SYSTEM_TRACE_SNAPSHOT);
+
     // If a background system trace is happening and is marked as "suitable for
     // bugreport" (i.e. bugreport_score > 0 in the trace config), this command
     // will stop it and serialize into SYSTEM_TRACE_SNAPSHOT. In the (likely)
@@ -3379,14 +3389,8 @@ void Dumpstate::MaybeSnapshotSystemTrace() {
     // Note: this should not be enqueued as we need to freeze the trace before
     // dumpstate starts. Otherwise the trace ring buffers will contain mostly
     // the dumpstate's own activity which is irrelevant.
-    int res = RunCommand(
-        "SERIALIZE PERFETTO TRACE",
-        {"perfetto", "--save-for-bugreport"},
-        CommandOptions::WithTimeout(10)
-            .DropRoot()
-            .CloseAllFileDescriptorsOnExec()
-            .Build());
-    has_system_trace_ = res == 0;
+    RunCommand("SERIALIZE PERFETTO TRACE", {"perfetto", "--save-for-bugreport"},
+               CommandOptions::WithTimeout(10).DropRoot().CloseAllFileDescriptorsOnExec().Build());
     // MaybeAddSystemTraceToZip() will take care of copying the trace in the zip
     // file in the later stages.
 }
@@ -3413,33 +3417,6 @@ void Dumpstate::MaybeSnapshotUiTraces() {
             "", command,
             CommandOptions::WithTimeout(10).Always().DropRoot().RedirectStderr().Build());
     }
-
-    // This command needs to be run as root
-    static const auto SURFACEFLINGER_COMMAND_SAVE_ALL_TRACES = std::vector<std::string> {
-        "service", "call", "SurfaceFlinger", "1042"
-    };
-    // Empty name because it's not intended to be classified as a bugreport section.
-    // Actual tracing files can be found in "/data/misc/wmtrace/" in the bugreport.
-    RunCommand(
-        "", SURFACEFLINGER_COMMAND_SAVE_ALL_TRACES,
-        CommandOptions::WithTimeout(10).Always().AsRoot().RedirectStderr().Build());
-}
-
-void Dumpstate::MaybePostProcessUiTraces() {
-    if (PropertiesHelper::IsUserBuild()) {
-        return;
-    }
-
-    RunCommand(
-        // Empty name because it's not intended to be classified as a bugreport section.
-        // Actual tracing files can be found in "/data/misc/wmtrace/" in the bugreport.
-        "", {
-            "/system/xbin/su", "system",
-            "/system/bin/layertracegenerator",
-            "/data/misc/wmtrace/transactions_trace.winscope",
-            "/data/misc/wmtrace/layers_trace_from_transactions.winscope"
-        },
-        CommandOptions::WithTimeout(120).Always().RedirectStderr().Build());
 }
 
 void Dumpstate::MaybeAddUiTracesToZip() {
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index 0a032ecfc3..5913f6b1a7 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -473,11 +473,6 @@ class Dumpstate {
     // Whether it should take an screenshot earlier in the process.
     bool do_early_screenshot_ = false;
 
-    // This is set to true when the trace snapshot request in the early call to
-    // MaybeSnapshotSystemTrace(). When this is true, the later stages of
-    // dumpstate will append the trace to the zip archive.
-    bool has_system_trace_ = false;
-
     std::unique_ptr<Progress> progress_;
 
     // When set, defines a socket file-descriptor use to report progress to bugreportz
@@ -570,7 +565,6 @@ class Dumpstate {
     void MaybeTakeEarlyScreenshot();
     void MaybeSnapshotSystemTrace();
     void MaybeSnapshotUiTraces();
-    void MaybePostProcessUiTraces();
     void MaybeAddUiTracesToZip();
 
     void onUiIntensiveBugreportDumpsFinished(int32_t calling_uid);
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index a417837ef9..fc828864d5 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -1000,7 +1000,6 @@ TEST_F(DumpstateTest, DumpPool_withParallelRunDisabled_isNull) {
 TEST_F(DumpstateTest, PreDumpUiData) {
     // These traces are always enabled, i.e. they are always pre-dumped
     const std::vector<std::filesystem::path> uiTraces = {
-        std::filesystem::path{"/data/misc/wmtrace/transactions_trace.winscope"},
         std::filesystem::path{"/data/misc/wmtrace/wm_transition_trace.winscope"},
         std::filesystem::path{"/data/misc/wmtrace/shell_transition_trace.winscope"},
     };