Enable System tracing for all Android debug builds as well as finch.

This is safe since a rooted (debug build) android phone would be able to
set the command line flags to enable this finch experiment anyway. In
addition this makes developer's lives a lot easier because they can just
know system tracing will work on their local devices (if debug build)
without fiddling with flags.

We also check the enable guardrails and enforce that in guardrails mode
privacy filter is enabled.

Bug: 1007299
Change-Id: Ic01858c353a195dd7492fb12408aa3241895b334
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1803919
Commit-Queue: Stephen Nusko <nuskos@chromium.org>
Reviewed-by: Stephen Nusko <nuskos@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Auto-Submit: Stephen Nusko <nuskos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700223}

services/tracing/perfetto/system_perfetto_unittest.cc

@@ -49,17 +49,38 @@ std::string RandomASCII(size_t length) {
   return tmp;
 }
 
+class SaveSystemProducerAndScopedRestore {
+ public:
+  SaveSystemProducerAndScopedRestore()
+      : saved_producer_(
+            PerfettoTracedProcess::Get()->SetSystemProducerForTesting(
+                std::make_unique<DummyProducer>(
+                    PerfettoTracedProcess::GetTaskRunner()))) {}
+
+  ~SaveSystemProducerAndScopedRestore() {
+    base::RunLoop destroy;
+    PerfettoTracedProcess::GetTaskRunner()
+        ->GetOrCreateTaskRunner()
+        ->PostTaskAndReply(
+            FROM_HERE, base::BindLambdaForTesting([this]() {
+              PerfettoTracedProcess::Get()
+                  ->SetSystemProducerForTesting(std::move(saved_producer_))
+                  .reset();
+            }),
+            destroy.QuitClosure());
+    destroy.Run();
+  }
+
+ private:
+  std::unique_ptr<SystemProducer> saved_producer_;
+};
+
 class SystemPerfettoTest : public testing::Test {
  public:
   SystemPerfettoTest()
       : task_environment_(base::test::TaskEnvironment::MainThreadType::IO) {
-    // Ensure system tracing is enabled for all tests.
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitAndEnableFeature(features::kEnablePerfettoSystemTracing);
     PerfettoTracedProcess::ResetTaskRunnerForTesting();
-    // To ensure we have a fully clean PerfettoTracedProcess reconstruct it at
-    // the beginning of each test.
-    PerfettoTracedProcess::ReconstructForTesting(perfetto::GetProducerSocket());
+    PerfettoTracedProcess::Get()->ClearDataSourcesForTesting();
 
     EXPECT_TRUE(tmp_dir_.CreateUniqueTempDir());
     // We need to set TMPDIR environment variable because when a new producer
@@ -134,17 +155,6 @@ class SystemPerfettoTest : public testing::Test {
   }
 
   ~SystemPerfettoTest() override {
-    // The real "AndroidSystemProducer" must be destroyed on the correct
-    // sequence however that sequence is tied to the |task_environment_|
-    // which is being deleted now. Therefore to prevent it crashing a future
-    // test we destroy it now.
-    PerfettoTracedProcess::GetTaskRunner()->GetOrCreateTaskRunner()->PostTask(
-        FROM_HERE, base::BindOnce([]() {
-          PerfettoTracedProcess::Get()
-              ->SetSystemProducerForTesting(std::make_unique<DummyProducer>(
-                  PerfettoTracedProcess::GetTaskRunner()))
-              .reset();
-        }));
     RunUntilIdle();
     // The producer client will be destroyed in the next iteration of the test,
     // but the sequence it was used on disappears with the
@@ -790,7 +800,33 @@ TEST_F(SystemPerfettoTest, SystemToLowAPILevel) {
   EXPECT_EQ(0u, run_test(/* check_sdk_level = */ true));
 }
 
+TEST_F(SystemPerfettoTest, EnabledOnDebugBuilds) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(features::kEnablePerfettoSystemTracing);
+  // We have to prevent destroying the system producer because we might have
+  // created it on a different task environment (wrong sequence).
+  SaveSystemProducerAndScopedRestore saved_system_producer;
+  PerfettoTracedProcess::ReconstructForTesting(producer_socket_.c_str());
+  if (base::android::BuildInfo::GetInstance()->is_debug_android()) {
+    EXPECT_FALSE(PerfettoTracedProcess::Get()
+                     ->SystemProducerForTesting()
+                     ->IsDummySystemProducerForTesting());
+  } else {
+    EXPECT_TRUE(PerfettoTracedProcess::Get()
+                    ->SystemProducerForTesting()
+                    ->IsDummySystemProducerForTesting());
+  }
+}
+
 TEST_F(SystemPerfettoTest, RespectsFeatureList) {
+  if (base::android::BuildInfo::GetInstance()->is_debug_android()) {
+    // The feature list is ignored on debug android builds so we should have a
+    // real system producer so just bail out of this test.
+    EXPECT_FALSE(PerfettoTracedProcess::Get()
+                     ->SystemProducerForTesting()
+                     ->IsDummySystemProducerForTesting());
+    return;
+  }
   {
     base::test::ScopedFeatureList feature_list;
     feature_list.InitAndEnableFeature(features::kEnablePerfettoSystemTracing);

services/tracing/public/cpp/perfetto/android_system_producer.cc

@@ -26,6 +26,19 @@ namespace {
 constexpr uint32_t kInitialConnectionBackoffMs = 100;
 constexpr uint32_t kMaxConnectionBackoffMs = 30 * 1000;
 
+perfetto::DataSourceConfig EnsureGuardRailsAreFollowed(
+    const perfetto::DataSourceConfig& data_source_config) {
+  if (!data_source_config.enable_extra_guardrails() ||
+      data_source_config.chrome_config().privacy_filtering_enabled()) {
+    return data_source_config;
+  }
+  // If extra_guardrails is enabled then we have to ensure we have privacy
+  // filtering enabled.
+  perfetto::DataSourceConfig config = data_source_config;
+  config.mutable_chrome_config()->set_privacy_filtering_enabled(true);
+  return config;
+}
+
 uint32_t IncreaseBackoff(uint32_t current, uint32_t max) {
   return std::min(current * 2, max);
 }
@@ -44,7 +57,6 @@ AndroidSystemProducer::~AndroidSystemProducer() {
 }
 
 void AndroidSystemProducer::SetDisallowPreAndroidPieForTesting(bool disallow) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   disallow_pre_android_pie = disallow;
   if (!disallow && state_ == State::kUninitialized) {
     // If previously we would not have connected, we now attempt to connect
@@ -54,7 +66,6 @@ void AndroidSystemProducer::SetDisallowPreAndroidPieForTesting(bool disallow) {
 }
 
 void AndroidSystemProducer::SetNewSocketForTesting(const char* socket) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   socket_name_ = socket;
   if (state_ == State::kConnected) {
     // If we are fully connected we need to reset the service before we
@@ -69,6 +80,15 @@ void AndroidSystemProducer::SetNewSocketForTesting(const char* socket) {
   }
 }
 
+void AndroidSystemProducer::ResetSequenceForTesting() {
+  // DETACH the sequence and then immediately attach it. This is needed in tests
+  // because we might be executing in a TaskEnvironment, but the global
+  // PerfettoTracedProcess (which contains a pointer to AndroidSystemProducer)
+  // will leak between tests, but the sequence will no longer be valid.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
 bool AndroidSystemProducer::IsTracingActive() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return data_sources_tracing_ > 0;
@@ -217,8 +237,9 @@ void AndroidSystemProducer::StartDataSource(
                 }
                 DCHECK_CALLED_ON_VALID_SEQUENCE(weak_ptr->sequence_checker_);
                 ++weak_ptr->data_sources_tracing_;
-                data_source->StartTracingWithID(id, weak_ptr.get(),
-                                                data_source_config);
+                data_source->StartTracingWithID(
+                    id, weak_ptr.get(),
+                    EnsureGuardRailsAreFollowed(data_source_config));
                 weak_ptr->service_->NotifyDataSourceStarted(id);
               },
               weak_ptr_factory_.GetWeakPtr(), data_source, id, config));

services/tracing/public/cpp/perfetto/android_system_producer.h

@@ -68,6 +68,11 @@ class COMPONENT_EXPORT(TRACING_CPP) AndroidSystemProducer
   // traces.
   void DisconnectWithReply(
       base::OnceClosure on_disconnect_complete = base::OnceClosure()) override;
+  // IMPORTANT!! this only resets |sequence_checker_| owned by this class, if
+  // this SystemProducer is connected to a service through the unix socket there
+  // will be additional sequence checkers which will fail if the socket
+  // disconnects.
+  void ResetSequenceForTesting() override;
 
   // perfetto::Producer implementation.
   // Used by the service to start and stop traces.

services/tracing/public/cpp/perfetto/dummy_producer.cc

@@ -72,4 +72,6 @@ bool DummyProducer::IsDummySystemProducerForTesting() {
   return true;
 }
 
+void DummyProducer::ResetSequenceForTesting() {}
+
 }  // namespace tracing

services/tracing/public/cpp/perfetto/dummy_producer.h

@@ -55,6 +55,7 @@ class COMPONENT_EXPORT(TRACING_CPP) DummyProducer : public SystemProducer {
   // Functions expected for SystemProducer
   void DisconnectWithReply(base::OnceClosure on_disconnect_complete) override;
   bool IsDummySystemProducerForTesting() override;
+  void ResetSequenceForTesting() override;
 };
 }  // namespace tracing
 

services/tracing/public/cpp/perfetto/perfetto_traced_process.cc

@@ -33,9 +33,7 @@ namespace {
 std::unique_ptr<SystemProducer> NewSystemProducer(PerfettoTaskRunner* runner,
                                                   const char* socket_name) {
 #if defined(OS_ANDROID)
-  if (base::FeatureList::IsEnabled(features::kEnablePerfettoSystemTracing)) {
-    // TODO(nuskos): We will also need eventually to check that any required
-    // consent has been given before constructing this producer.
+  if (ShouldSetupSystemTracing()) {
     DCHECK(socket_name);
     return std::make_unique<AndroidSystemProducer>(socket_name, runner);
   }
@@ -151,6 +149,12 @@ void PerfettoTracedProcess::ResetTaskRunnerForTesting(
   // call to Get()) which would then PostTask which would create races if we
   // reset the task runner right afterwards.
   DETACH_FROM_SEQUENCE(PerfettoTracedProcess::Get()->sequence_checker_);
+  PerfettoTracedProcess::GetTaskRunner()->GetOrCreateTaskRunner()->PostTask(
+      FROM_HERE, base::BindOnce([]() {
+        PerfettoTracedProcess::Get()
+            ->SystemProducerForTesting()
+            ->ResetSequenceForTesting();
+      }));
 }
 
 // static

services/tracing/public/cpp/perfetto/system_producer.h

@@ -24,6 +24,7 @@ class COMPONENT_EXPORT(TRACING_CPP) SystemProducer : public PerfettoProducer,
       base::OnceClosure on_disconnect_complete) = 0;
 
   virtual bool IsDummySystemProducerForTesting();
+  virtual void ResetSequenceForTesting() = 0;
 };
 }  // namespace tracing
 

services/tracing/public/cpp/perfetto/trace_event_data_source_unittest.cc

@@ -244,13 +244,22 @@ class TraceEventDataSourceTest : public testing::Test {
     producer_client_.reset();
   }
 
-  void CreateTraceEventDataSource(bool privacy_filtering_enabled = false) {
-    TraceEventDataSource::ResetForTesting();
-    perfetto::DataSourceConfig config;
-    config.mutable_chrome_config()->set_privacy_filtering_enabled(
-        privacy_filtering_enabled);
-    TraceEventDataSource::GetInstance()->StartTracing(producer_client(),
-                                                      config);
+  void CreateTraceEventDataSource(bool privacy_filtering_enabled = false,
+                                  bool start_trace = true) {
+    task_environment_.RunUntilIdle();
+    base::RunLoop tracing_started;
+    base::SequencedTaskRunnerHandle::Get()->PostTaskAndReply(
+        FROM_HERE,
+        base::BindOnce([]() { TraceEventDataSource::ResetForTesting(); }),
+        tracing_started.QuitClosure());
+    tracing_started.Run();
+    if (start_trace) {
+      perfetto::DataSourceConfig config;
+      config.mutable_chrome_config()->set_privacy_filtering_enabled(
+          privacy_filtering_enabled);
+      TraceEventDataSource::GetInstance()->StartTracing(producer_client(),
+                                                        config);
+    }
   }
 
   MockProducerClient* producer_client() { return producer_client_.get(); }
@@ -1060,6 +1069,7 @@ TEST_F(TraceEventDataSourceTest, FilteringMetadataSource) {
 }
 
 TEST_F(TraceEventDataSourceTest, ProtoMetadataSource) {
+  CreateTraceEventDataSource();
   auto* metadata_source = TraceEventMetadataSource::GetInstance();
   metadata_source->AddGeneratorFunction(base::BindRepeating(
       [](perfetto::protos::pbzero::ChromeMetadataPacket* metadata,
@@ -1073,8 +1083,6 @@ TEST_F(TraceEventDataSourceTest, ProtoMetadataSource) {
         rule->set_histogram_rule()->set_histogram_min_trigger(123);
       }));
 
-  CreateTraceEventDataSource();
-
   perfetto::DataSourceConfig config;
   config.mutable_chrome_config()->set_privacy_filtering_enabled(true);
   metadata_source->StartTracing(producer_client(), config);
@@ -1157,10 +1165,11 @@ TEST_F(TraceEventDataSourceNoInterningTest, InterningScopedToPackets) {
 }
 
 TEST_F(TraceEventDataSourceTest, StartupTracingTimeout) {
+  CreateTraceEventDataSource(/* privacy_filtering_enabled = */ false,
+                             /* start_trace = */ false);
   PerfettoTracedProcess::ResetTaskRunnerForTesting(
       base::SequencedTaskRunnerHandle::Get());
   constexpr char kStartupTestEvent1[] = "startup_registry";
-  TraceEventDataSource::ResetForTesting();
   auto* data_source = TraceEventDataSource::GetInstance();
 
   // Start startup tracing registry with no timeout. This would cause startup

services/tracing/public/cpp/trace_startup.cc

@@ -85,7 +85,7 @@ void InitTracingPostThreadPoolStartAndFeatureList() {
   CHECK(base::FeatureList::GetInstance());
   // Below are the things tracing must do once per process.
   TraceEventDataSource::GetInstance()->OnTaskSchedulerAvailable();
-  if (base::FeatureList::IsEnabled(features::kEnablePerfettoSystemTracing)) {
+  if (ShouldSetupSystemTracing()) {
     // We have to ensure that we register all the data sources we care about.
     TraceEventAgent::GetInstance();
     // To ensure System tracing connects we have to initialize the process wide

services/tracing/public/cpp/tracing_features.cc

@@ -12,6 +12,10 @@
 #include "build/build_config.h"
 #include "components/tracing/common/tracing_switches.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/build_info.h"  // nogncheck
+#endif
+
 namespace features {
 
 // Enables the perfetto tracing backend. For startup tracing, pass the
@@ -80,4 +84,17 @@ bool TracingUsesPerfettoBackend() {
          base::FEATURE_ENABLED_BY_DEFAULT;
 }
 
+bool ShouldSetupSystemTracing() {
+#if defined(OS_ANDROID)
+  if (base::android::BuildInfo::GetInstance()->is_debug_android()) {
+    return true;
+  }
+#endif  // defined(OS_ANDROID)
+  if (base::FeatureList::GetInstance()) {
+    return base::FeatureList::IsEnabled(features::kTracingPerfettoBackend);
+  }
+  return features::kEnablePerfettoSystemTracing.default_state ==
+         base::FEATURE_ENABLED_BY_DEFAULT;
+}
+
 }  // namespace tracing

services/tracing/public/cpp/tracing_features.h

@@ -36,6 +36,11 @@ namespace tracing {
 
 bool COMPONENT_EXPORT(TRACING_CPP) TracingUsesPerfettoBackend();
 
+// Returns true if the system tracing Perfetto producer should be setup. This
+// can be influenced by the feature above or other situations (like debug
+// android builds).
+bool COMPONENT_EXPORT(TRACING_CPP) ShouldSetupSystemTracing();
+
 }  // namespace tracing
 
 #endif  // SERVICES_TRACING_PUBLIC_CPP_TRACING_FEATURES_H_