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

This is a reland of fd8826a6.

Includes a further fix to BackgroundTracingActiveScenario, which was
accidentally setting a default-sized trace buffer for all its sessions
and causing OOMs (crbug.com/997610). Also rebased after non-perfetto
tracing backend was removed recently. Relevant changes in PS 2 -> 5.

> 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}

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

base/trace_event/trace_config.h

@@ -256,6 +256,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.
@@ -323,8 +328,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";
+
 // Repeat internable data for each TraceEvent in the perfetto proto format.
 const char kPerfettoDisableInterning[] = "perfetto-disable-interning";
 

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 kPerfettoDisableInterning[];
 TRACING_EXPORT extern const char kPerfettoOutputFile[];
 TRACING_EXPORT extern const char kTraceToConsole[];

content/browser/browser_child_process_host_impl.cc

@@ -59,6 +59,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"
@@ -247,24 +248,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

@@ -354,7 +354,20 @@ 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;
-  base::trace_event::TraceLog::GetInstance()->SetEnabled(chrome_config, modes);
+
+// TODO(crbug.com/941318): Re-enable startup tracing for Perfetto backend on
+// Android once all Perfetto-related deadlocks are resolved.
+#if !defined(OS_ANDROID)
+  TraceConfig chrome_config_for_trace_log(chrome_config);
+  // Perfetto backend configures buffer sizes when tracing is started in the
+  // service (see perfetto_config.cc). Zero them out here for TraceLog to avoid
+  // DCHECKs in TraceConfig::Merge.
+  chrome_config_for_trace_log.SetTraceBufferSizeInKb(0);
+  chrome_config_for_trace_log.SetTraceBufferSizeInEvents(0);
+
+  base::trace_event::TraceLog::GetInstance()->SetEnabled(
+      chrome_config_for_trace_log, modes);
+#endif  // !defined(OS_ANDROID)
 
   DCHECK(!tracing_session_);
   if (base::FeatureList::IsEnabled(features::kBackgroundTracingProtoOutput)) {

content/browser/tracing/startup_tracing_browsertest.cc

@@ -127,6 +127,8 @@ IN_PROC_BROWSER_TEST_F(StartupTracingInProcessTest, TestFilledStartupBuffer) {
 
   auto config = tracing::TraceStartupConfig::GetInstance()
                     ->GetDefaultBrowserStartupConfig();
+  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

@@ -401,16 +401,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

@@ -157,6 +157,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>;
 
@@ -219,6 +223,7 @@ class COMPONENT_EXPORT(TRACING_CPP) TraceEventDataSource
       startup_writer_registry_;
   std::unique_ptr<perfetto::StartupTraceWriter> trace_writer_;
   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

@@ -14,6 +14,7 @@
 #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_agent.h"
+#include "services/tracing/public/cpp/trace_event_args_whitelist.h"
 #include "services/tracing/public/cpp/tracing_features.h"
 
 namespace tracing {
@@ -44,19 +45,39 @@ void EnableStartupTracingIfNeeded() {
   auto* startup_config = TraceStartupConfig::GetInstance();
 
   if (startup_config->IsEnabled()) {
-    const TraceConfig& trace_config = startup_config->GetTraceConfig();
-      if (trace_config.IsCategoryGroupEnabled(
-              TRACE_DISABLED_BY_DEFAULT("cpu_profiler"))) {
-        TracingSamplerProfiler::SetupStartupTracing();
-      }
-      TraceEventDataSource::GetInstance()->SetupStartupTracing(
-          startup_config->GetSessionOwner() ==
-          TraceStartupConfig::SessionOwner::kBackgroundTracing);
+    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));
+    }
+
+    // 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 ||
+        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.
@@ -71,9 +92,8 @@ void EnableStartupTracingIfNeeded() {
 }
 
 void InitTracingPostThreadPoolStartAndFeatureList() {
-  if (g_tracing_initialized_after_threadpool_and_featurelist) {
+  if (g_tracing_initialized_after_threadpool_and_featurelist)
     return;
-  }
   g_tracing_initialized_after_threadpool_and_featurelist = true;
   // TODO(nuskos): We should switch these to DCHECK once we're reasonably
   // confident we've ensured this is called properly in all processes. Probably
@@ -91,4 +111,59 @@ 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 (!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 configure the tracing service's central buffer, so also don't
+  // affect local tracing.
+  //
+  // 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())
+    return;
+
+  // Make sure that the startup session uses privacy filtering mode if it's
+  // enabled for the browser's session.
+  if (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_