Trace Redaction - Assign empty comm for cleared values

Before, we removed the next_comm and prev_comm values by not copying
the fields over. This caused issues in the perfetto ui as it would
use "a value" causing names to appear from "somewhere".

Now, we replace strings with an empty string, fixing the issue. We've
seen this issue elsewhere and should be applied to future
modifications.

Bug: 336807771
Change-Id: Ia7e6cab076e0ec0de7f2cdedb1903f1da2040172

3 files changed, 29 insertions, 24 deletions

diff --git a/src/trace_redaction/redact_sched_switch.cc b/src/trace_redaction/redact_sched_switch.cc
index 2f85efe38..55777f1df 100644
--- a/src/trace_redaction/redact_sched_switch.cc
+++ b/src/trace_redaction/redact_sched_switch.cc
@@ -88,13 +88,17 @@ base::Status RedactSchedSwitch::Redact(
     switch (field.id()) {
       case protos::pbzero::SchedSwitchFtraceEvent::kNextCommFieldNumber:
         if (next_slice.uid == context.package_uid) {
-          proto_util::AppendField(field, sched_switch_message);
+          sched_switch_message->set_next_comm(field.as_string());
+        } else {
+          sched_switch_message->set_next_comm("");
         }
         break;
 
       case protos::pbzero::SchedSwitchFtraceEvent::kPrevCommFieldNumber:
         if (prev_slice.uid == context.package_uid) {
-          proto_util::AppendField(field, sched_switch_message);
+          sched_switch_message->set_prev_comm(field.as_string());
+        } else {
+          sched_switch_message->set_prev_comm("");
         }
         break;
 
diff --git a/src/trace_redaction/redact_sched_switch_integrationtest.cc b/src/trace_redaction/redact_sched_switch_integrationtest.cc
index 3b3523f3c..67948f2fe 100644
--- a/src/trace_redaction/redact_sched_switch_integrationtest.cc
+++ b/src/trace_redaction/redact_sched_switch_integrationtest.cc
@@ -175,18 +175,19 @@ TEST_F(RedactSchedSwitchIntegrationTest, ClearsNonTargetSwitchComms) {
       const auto* next_comm = expected_names.Find(next_pid);
       const auto* prev_comm = expected_names.Find(prev_pid);
 
+      EXPECT_TRUE(sched_decoder.has_next_comm());
+      EXPECT_TRUE(sched_decoder.has_prev_comm());
+
       if (next_comm) {
-        EXPECT_TRUE(sched_decoder.has_next_comm());
         EXPECT_EQ(sched_decoder.next_comm().ToStdString(), *next_comm);
       } else {
-        EXPECT_FALSE(sched_decoder.has_next_comm());
+        EXPECT_EQ(sched_decoder.next_comm().size, 0u);
       }
 
       if (prev_comm) {
-        EXPECT_TRUE(sched_decoder.has_prev_comm());
         EXPECT_EQ(sched_decoder.prev_comm().ToStdString(), *prev_comm);
       } else {
-        EXPECT_FALSE(sched_decoder.has_prev_comm());
+        EXPECT_EQ(sched_decoder.prev_comm().size, 0u);
       }
     }
   }
diff --git a/src/trace_redaction/redact_sched_switch_unittest.cc b/src/trace_redaction/redact_sched_switch_unittest.cc
index 174a35f1b..f6a4c1357 100644
--- a/src/trace_redaction/redact_sched_switch_unittest.cc
+++ b/src/trace_redaction/redact_sched_switch_unittest.cc
@@ -108,7 +108,7 @@ TEST_F(RedactSchedSwitchTest, RejectMissingTimeline) {
   ASSERT_FALSE(result.ok());
 }
 
-TEST_F(RedactSchedSwitchTest, ClearsPrevAndNext) {
+TEST_F(RedactSchedSwitchTest, ReplacePrevAndNextWithEmptyStrings) {
   RedactSchedSwitch redact;
 
   Context context;
@@ -131,15 +131,15 @@ TEST_F(RedactSchedSwitchTest, ClearsPrevAndNext) {
 
   ASSERT_TRUE(event.has_sched_switch());
 
-  // Pid should always carry over; only the comm value should get removed.
-  ASSERT_TRUE(event.sched_switch().has_next_pid());
-  ASSERT_FALSE(event.sched_switch().has_next_comm());
+  // Cleared prev and next comm.
+  ASSERT_TRUE(event.sched_switch().has_prev_comm());
+  ASSERT_TRUE(event.sched_switch().prev_comm().empty());
 
-  ASSERT_TRUE(event.sched_switch().has_prev_pid());
-  ASSERT_FALSE(event.sched_switch().has_prev_comm());
+  ASSERT_TRUE(event.sched_switch().has_next_comm());
+  ASSERT_TRUE(event.sched_switch().next_comm().empty());
 }
 
-TEST_F(RedactSchedSwitchTest, ClearsPrev) {
+TEST_F(RedactSchedSwitchTest, ReplacePrevWithEmptyStrings) {
   RedactSchedSwitch redact;
 
   Context context;
@@ -162,15 +162,15 @@ TEST_F(RedactSchedSwitchTest, ClearsPrev) {
 
   ASSERT_TRUE(event.has_sched_switch());
 
-  // Pid should always carry over; only the comm value should get removed.
-  ASSERT_TRUE(event.sched_switch().has_next_pid());
-  ASSERT_TRUE(event.sched_switch().has_next_comm());
+  // Only cleared the prev comm.
+  ASSERT_TRUE(event.sched_switch().has_prev_comm());
+  ASSERT_TRUE(event.sched_switch().prev_comm().empty());
 
-  ASSERT_TRUE(event.sched_switch().has_prev_pid());
-  ASSERT_FALSE(event.sched_switch().has_prev_comm());
+  ASSERT_TRUE(event.sched_switch().has_next_comm());
+  ASSERT_FALSE(event.sched_switch().next_comm().empty());
 }
 
-TEST_F(RedactSchedSwitchTest, ClearNext) {
+TEST_F(RedactSchedSwitchTest, ReplaceNextWithEmptyStrings) {
   RedactSchedSwitch redact;
 
   Context context;
@@ -193,12 +193,12 @@ TEST_F(RedactSchedSwitchTest, ClearNext) {
 
   ASSERT_TRUE(event.has_sched_switch());
 
-  // Pid should always carry over; only the comm value should get removed.
-  ASSERT_TRUE(event.sched_switch().has_next_pid());
-  ASSERT_FALSE(event.sched_switch().has_next_comm());
-
-  ASSERT_TRUE(event.sched_switch().has_prev_pid());
   ASSERT_TRUE(event.sched_switch().has_prev_comm());
+  ASSERT_FALSE(event.sched_switch().prev_comm().empty());
+
+  // Only cleared the next comm.
+  ASSERT_TRUE(event.sched_switch().has_next_comm());
+  ASSERT_TRUE(event.sched_switch().next_comm().empty());
 }
 
 }  // namespace perfetto::trace_redaction