Reland "tracing: Enable startup tracing for spawned child processes"

This is a reland of e40282f6.

Includes a fix to BackgroundTracingActiveScenario, which could end up enabling
TraceLog without installing the TraceEventDataSource overrides on Android, see
patch set 1 -> 2. This likely caused the crashes observed in crbug.com/997582.

Original change's description:
> tracing: Enable startup tracing for spawned child processes
>
> Enables startup tracing for child processes spawned by the browser while
> a tracing session is active.
>
> This allows us to collect early trace events in the period of time
> before the child connects to the tracing service.
>
> Bug: 968424
> Change-Id: I1708cabae77cdd2ab3a33929c28cfab04eb723e3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755746
> Commit-Queue: Eric Seckler <eseckler@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: ssid <ssid@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#689599}

Bug: 968424
Change-Id: Id69f0821671caf05238896d4a0e1b581b5d5be44
TBR: avi@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1782812
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: ssid <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694981}

base/trace_event/trace_config.h

@@ -255,6 +255,11 @@ class BASE_EXPORT TraceConfig {
   // Write the string representation of the CategoryFilter part.
   std::string ToCategoryFilterString() const;
 
+  // Write the string representation of the trace options part (record mode,
+  // systrace, argument filtering). Does not include category filters, event
+  // filters, or memory dump configs.
+  std::string ToTraceOptionsString() const;
+
   // Returns true if at least one category in the list is enabled by this
   // trace config. This is used to determine if the category filters are
   // enabled in the TRACE_* macros.
@@ -317,8 +322,6 @@ class BASE_EXPORT TraceConfig {
   void SetEventFiltersFromConfigList(const Value& event_filters);
   Value ToValue() const;
 
-  std::string ToTraceOptionsString() const;
-
   TraceRecordMode record_mode_;
   size_t trace_buffer_size_in_events_ = 0;  // 0 specifies default size
   size_t trace_buffer_size_in_kb_ = 0;      // 0 specifies default size

components/tracing/common/tracing_switches.cc

@@ -37,7 +37,7 @@ const char kTraceStartupDuration[]          = "trace-startup-duration";
 // all events since startup.
 const char kTraceStartupFile[]              = "trace-startup-file";
 
-// If supplied, sets the tracing record mode; otherwise, the default
+// If supplied, sets the tracing record mode and options; otherwise, the default
 // "record-until-full" mode will be used.
 const char kTraceStartupRecordMode[] = "trace-startup-record-mode";
 
@@ -58,6 +58,11 @@ const char kTraceStartupRecordMode[] = "trace-startup-record-mode";
 // through the normal methods for stopping system traces.
 const char kTraceStartupOwner[] = "trace-startup-owner";
 
+// If the perfetto tracing backend is used, this enables privacy filtering in
+// the TraceEvent data sources for the startup tracing session.
+const char kTraceStartupEnablePrivacyFiltering[] =
+    "trace-startup-enable-privacy-filtering";
+
 // Disables the perfetto tracing backend. We need a separate command line
 // argument from the kTracingPerfettoBackend feature, because feature flags are
 // parsed too late during startup for early startup tracing support.

components/tracing/common/tracing_switches.h

@@ -16,6 +16,7 @@ TRACING_EXPORT extern const char kTraceStartupDuration[];
 TRACING_EXPORT extern const char kTraceStartupFile[];
 TRACING_EXPORT extern const char kTraceStartupRecordMode[];
 TRACING_EXPORT extern const char kTraceStartupOwner[];
+TRACING_EXPORT extern const char kTraceStartupEnablePrivacyFiltering[];
 TRACING_EXPORT extern const char kDisablePerfetto[];
 TRACING_EXPORT extern const char kEnablePerfetto[];
 TRACING_EXPORT extern const char kPerfettoDisableInterning[];

content/browser/browser_child_process_host_impl.cc

@@ -55,6 +55,7 @@
 #include "net/websockets/websocket_channel.h"
 #include "services/service_manager/embedder/switches.h"
 #include "services/service_manager/public/cpp/constants.h"
+#include "services/tracing/public/cpp/trace_startup.h"
 
 #if defined(OS_MACOSX)
 #include "content/browser/child_process_task_port_provider_mac.h"
@@ -217,24 +218,7 @@ void BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(
 // static
 void BrowserChildProcessHostImpl::CopyTraceStartupFlags(
     base::CommandLine* cmd_line) {
-  if (tracing::TraceStartupConfig::GetInstance()->IsEnabled()) {
-    const auto trace_config =
-        tracing::TraceStartupConfig::GetInstance()->GetTraceConfig();
-    if (!trace_config.IsArgumentFilterEnabled()) {
-      // The only trace option that we can pass through switches is the record
-      // mode. Other trace options should have the default value.
-      //
-      // TODO(chiniforooshan): Add other trace options to switches if, for
-      // example, they are used in a telemetry test that needs startup trace
-      // events from renderer processes.
-      cmd_line->AppendSwitchASCII(switches::kTraceStartup,
-                                  trace_config.ToCategoryFilterString());
-      cmd_line->AppendSwitchASCII(
-          switches::kTraceStartupRecordMode,
-          base::trace_event::TraceConfig::TraceRecordModeToStr(
-              trace_config.GetTraceRecordMode()));
-    }
-  }
+  tracing::PropagateTracingFlagsToChildProcessCmdLine(cmd_line);
 }
 
 void BrowserChildProcessHostImpl::Launch(

content/browser/tracing/background_tracing_active_scenario.cc

@@ -356,7 +356,25 @@ bool BackgroundTracingActiveScenario::StartTracing() {
   uint8_t modes = base::trace_event::TraceLog::RECORDING_MODE;
   if (!chrome_config.event_filters().empty())
     modes |= base::trace_event::TraceLog::FILTERING_MODE;
+
+  // Perfetto backend configures buffer sizes when tracing is started in the
+  // service (see perfetto_config.cc). Zero them out here to avoid DCHECKs in
+  // TraceConfig::Merge.
+  if (tracing::TracingUsesPerfettoBackend()) {
+    chrome_config.SetTraceBufferSizeInKb(0);
+    chrome_config.SetTraceBufferSizeInEvents(0);
+  }
+
+#if defined(OS_ANDROID)
+  // TODO(crbug.com/941318): Re-enable startup tracing for Perfetto backend on
+  // Android once all Perfetto-related deadlocks are resolved.
+  if (!tracing::TracingUsesPerfettoBackend()) {
+    base::trace_event::TraceLog::GetInstance()->SetEnabled(chrome_config,
+                                                           modes);
+  }
+#else   // defined(OS_ANDROID)
   base::trace_event::TraceLog::GetInstance()->SetEnabled(chrome_config, modes);
+#endif  // defined(OS_ANDROID)
 
   DCHECK(!tracing_session_);
   if (base::FeatureList::IsEnabled(features::kBackgroundTracingProtoOutput)) {

content/browser/tracing/startup_tracing_browsertest.cc

@@ -128,6 +128,10 @@ IN_PROC_BROWSER_TEST_F(StartupTracingInProcessTest, TestFilledStartupBuffer) {
 
   auto config = tracing::TraceStartupConfig::GetInstance()
                     ->GetDefaultBrowserStartupConfig();
+  if (tracing::TracingUsesPerfettoBackend()) {
+    config.SetTraceBufferSizeInEvents(0);
+    config.SetTraceBufferSizeInKb(0);
+  }
   uint8_t modes = base::trace_event::TraceLog::RECORDING_MODE;
   base::trace_event::TraceLog::GetInstance()->SetEnabled(config, modes);
 

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

@@ -71,14 +71,19 @@ perfetto::TraceConfig GetDefaultPerfettoConfig(
   // 5 seconds of the trace (if we wrap around perfetto's central buffer).
   perfetto_config.mutable_incremental_state_config()->set_clear_period_ms(5000);
 
-  // We strip the process filter from the config string we send to Perfetto,
-  // so perfetto doesn't reject it from a future
-  // TracingService::ChangeTraceConfig call due to being an unsupported
-  // update.
-  base::trace_event::TraceConfig processfilter_stripped_config(chrome_config);
-  processfilter_stripped_config.SetProcessFilterConfig(
+  // We strip the process filter from the config string we send to Perfetto, so
+  // perfetto doesn't reject it from a future TracingService::ChangeTraceConfig
+  // call due to being an unsupported update. We also strip the trace buffer
+  // size configuration to prevent chrome from rejecting an update to it after
+  // startup tracing via TraceConfig::Merge (see trace_startup.cc). For
+  // perfetto, the buffer size is configured via perfetto's buffer config and
+  // only affects the perfetto service.
+  base::trace_event::TraceConfig stripped_config(chrome_config);
+  stripped_config.SetProcessFilterConfig(
       base::trace_event::TraceConfig::ProcessFilterConfig());
-  std::string chrome_config_string = processfilter_stripped_config.ToString();
+  stripped_config.SetTraceBufferSizeInKb(0);
+  stripped_config.SetTraceBufferSizeInEvents(0);
+  std::string chrome_config_string = stripped_config.ToString();
 
   // Capture actual trace events.
   auto* trace_event_data_source = AddDataSourceConfig(

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

@@ -277,16 +277,24 @@ void TraceEventDataSource::RegisterWithTraceLog() {
       &TraceEventDataSource::OnAddTraceEvent,
       &TraceEventDataSource::FlushCurrentThread,
       &TraceEventDataSource::OnUpdateDuration);
+  base::AutoLock l(lock_);
+  is_enabled_ = true;
 }
 
 void TraceEventDataSource::UnregisterFromTraceLog() {
   RegisterTracedValueProtoWriter(false);
   TraceLog::GetInstance()->SetAddTraceEventOverrides(nullptr, nullptr, nullptr);
   base::AutoLock l(lock_);
+  is_enabled_ = false;
   flushing_trace_log_ = false;
   DCHECK(!flush_complete_task_);
 }
 
+bool TraceEventDataSource::IsEnabled() {
+  base::AutoLock l(lock_);
+  return is_enabled_;
+}
+
 void TraceEventDataSource::SetupStartupTracing(bool privacy_filtering_enabled) {
   {
     base::AutoLock lock(lock_);

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

@@ -140,6 +140,10 @@ class COMPONENT_EXPORT(TRACING_CPP) TraceEventDataSource
     startup_tracing_timeout_ = timeout_us;
   }
 
+  bool privacy_filtering_enabled() const { return privacy_filtering_enabled_; }
+
+  bool IsEnabled();
+
  private:
   friend class base::NoDestructor<TraceEventDataSource>;
 
@@ -199,6 +203,7 @@ class COMPONENT_EXPORT(TRACING_CPP) TraceEventDataSource
   std::unique_ptr<perfetto::StartupTraceWriterRegistry>
       startup_writer_registry_;
   base::OneShotTimer startup_tracing_timer_;
+  bool is_enabled_ = false;
   bool flushing_trace_log_ = false;
   base::OnceClosure flush_complete_task_;
   std::vector<std::string> histograms_;

services/tracing/public/cpp/trace_startup.cc

@@ -13,6 +13,7 @@
 #include "components/tracing/common/tracing_switches.h"
 #include "services/tracing/public/cpp/perfetto/trace_event_data_source.h"
 #include "services/tracing/public/cpp/stack_sampling/tracing_sampler_profiler.h"
+#include "services/tracing/public/cpp/trace_event_args_whitelist.h"
 #include "services/tracing/public/cpp/tracing_features.h"
 
 namespace tracing {
@@ -43,21 +44,42 @@ void EnableStartupTracingIfNeeded() {
   auto* startup_config = TraceStartupConfig::GetInstance();
 
   if (startup_config->IsEnabled()) {
-    const TraceConfig& trace_config = startup_config->GetTraceConfig();
+    TraceConfig trace_config = startup_config->GetTraceConfig();
+
+    // Make sure we only record whitelisted arguments even during early startup
+    // tracing if the config specifies argument filtering.
+    if (trace_config.IsArgumentFilterEnabled() &&
+        base::trace_event::TraceLog::GetInstance()
+            ->GetArgumentFilterPredicate()
+            .is_null()) {
+      base::trace_event::TraceLog::GetInstance()->SetArgumentFilterPredicate(
+          base::BindRepeating(&IsTraceEventArgsWhitelisted));
+      base::trace_event::TraceLog::GetInstance()->SetMetadataFilterPredicate(
+          base::BindRepeating(&IsMetadataWhitelisted));
+    }
+
     if (TracingUsesPerfettoBackend()) {
+      // Perfetto backend configures buffer sizes when tracing is started in the
+      // service (see perfetto_config.cc). Zero them out here to avoid DCHECKs
+      // in TraceConfig::Merge.
+      trace_config.SetTraceBufferSizeInKb(0);
+      trace_config.SetTraceBufferSizeInEvents(0);
+
       if (trace_config.IsCategoryGroupEnabled(
               TRACE_DISABLED_BY_DEFAULT("cpu_profiler"))) {
         TracingSamplerProfiler::SetupStartupTracing();
       }
       TraceEventDataSource::GetInstance()->SetupStartupTracing(
           startup_config->GetSessionOwner() ==
-          TraceStartupConfig::SessionOwner::kBackgroundTracing);
+              TraceStartupConfig::SessionOwner::kBackgroundTracing ||
+          command_line.HasSwitch(
+              switches::kTraceStartupEnablePrivacyFiltering));
     }
 
     uint8_t modes = TraceLog::RECORDING_MODE;
     if (!trace_config.event_filters().empty())
       modes |= TraceLog::FILTERING_MODE;
-    trace_log->SetEnabled(startup_config->GetTraceConfig(), modes);
+    trace_log->SetEnabled(trace_config, modes);
   } else if (command_line.HasSwitch(switches::kTraceToConsole)) {
     // TODO(eseckler): Remove ability to trace to the console, perfetto doesn't
     // support this and noone seems to use it.
@@ -91,4 +113,67 @@ void InitTracingPostThreadPoolStartAndFeatureList() {
   }
 }
 
+void PropagateTracingFlagsToChildProcessCmdLine(base::CommandLine* cmd_line) {
+  base::trace_event::TraceLog* trace_log =
+      base::trace_event::TraceLog::GetInstance();
+
+  if (!trace_log->IsEnabled())
+    return;
+
+  // It's possible that tracing is enabled only for atrace, in which case the
+  // TraceEventDataSource isn't registered. In that case, there's no reason to
+  // enable startup tracing in the child process (and we wouldn't know the
+  // correct value for privacy_filtering_enabled below).
+  if (TracingUsesPerfettoBackend() &&
+      !TraceEventDataSource::GetInstance()->IsEnabled()) {
+    return;
+  }
+
+  // The child process startup may race with a concurrent disabling of the
+  // tracing session by the tracing service. To avoid being stuck in startup
+  // tracing mode forever, the child process will discard the startup session
+  // after a timeout (|startup_tracing_timer_| in TraceEventDataSource).
+  //
+  // Note that we disregard the config's process filter, since it's possible
+  // that the trace consumer will update the config to include the process
+  // shortly. Otherwise, the startup tracing timeout in the child will
+  // eventually disable tracing for the process.
+
+  const auto trace_config = trace_log->GetCurrentTraceConfig();
+
+  // We can't currently propagate event filter options, memory dump configs, or
+  // trace buffer sizes via command line flags (they only support categories,
+  // trace options, record mode). If event filters are set, we bail out here to
+  // avoid recording events that we shouldn't in the child process. Even if
+  // memory dump config is set, it's OK to propagate the remaining config,
+  // because the child won't record events it shouldn't without it and will
+  // adopt the memory dump config once it connects to the tracing service.
+  // Buffer sizes only affect the legacy tracing backend's startup tracing, so
+  // we only bail out if perfetto is disabled.
+  //
+  // TODO(eseckler): Support propagating the full config via command line flags
+  // somehow (--trace-config?). This will also need some rethinking to support
+  // multiple concurrent tracing sessions in the future.
+  if (!trace_config.event_filters().empty() ||
+      ((trace_config.GetTraceBufferSizeInEvents() ||
+        trace_config.GetTraceBufferSizeInKb()) &&
+       !TracingUsesPerfettoBackend())) {
+    return;
+  }
+
+  // Make sure that the startup session uses privacy filtering mode if it's
+  // enabled for the browser's session.
+  if (TracingUsesPerfettoBackend() &&
+      TraceEventDataSource::GetInstance()->privacy_filtering_enabled()) {
+    cmd_line->AppendSwitch(switches::kTraceStartupEnablePrivacyFiltering);
+  }
+
+  cmd_line->AppendSwitchASCII(switches::kTraceStartup,
+                              trace_config.ToCategoryFilterString());
+  // The argument filtering setting is passed via trace options as part of
+  // --trace-startup-record-mode.
+  cmd_line->AppendSwitchASCII(switches::kTraceStartupRecordMode,
+                              trace_config.ToTraceOptionsString());
+}
+
 }  // namespace tracing

services/tracing/public/cpp/trace_startup.h

@@ -7,6 +7,10 @@
 
 #include "base/component_export.h"
 
+namespace base {
+class CommandLine;
+}  // namespace base
+
 namespace tracing {
 
 // Returns true if InitTracingPostThreadPoolStartAndFeatureList has been called
@@ -23,6 +27,11 @@ void COMPONENT_EXPORT(TRACING_CPP) EnableStartupTracingIfNeeded();
 void COMPONENT_EXPORT(TRACING_CPP)
     InitTracingPostThreadPoolStartAndFeatureList();
 
+// If tracing is enabled, grabs the current trace config & mode and tells the
+// child to begin tracing right away via startup tracing command line flags.
+void COMPONENT_EXPORT(TRACING_CPP)
+    PropagateTracingFlagsToChildProcessCmdLine(base::CommandLine* cmd_line);
+
 }  // namespace tracing
 
 #endif  // SERVICES_TRACING_PUBLIC_CPP_TRACE_STARTUP_H_