Fix startup tracing in renderers

Startup tracing is broken for several reasons:

1. When renderers are forked from a Zygote process, --trace-startup
   flags are ignored.

2. In a non-zygote mode, when --trace-config-file is used, it is
   ignored since sandboxed renderers do not have access to file
   system.

3. Another bug is that --trace-config-file is always passed to
   renderer processes, even if they are created after tracing is
   stopped, e.g. via DevTools.

4. Startup tracing flags are not passed along to the ppapi plugin
   and utility processes.

This CL fixes 1, 3, and 4 and partially fixes 2 by passing categories
and record mode of a trace config file using flags to sandboxed
renderer processes so that they do not need to ready these information
from a file. Obviously, this does not work for complex trace config
files, but should be enough to resolve existing telemetry issues
(please see discussions in crbug.com/809833).

Bug: 809833
Change-Id: I6723664fd477117262763476c0a86fd6881b94a7
Reviewed-on: https://chromium-review.googlesource.com/972145Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545950}

base/trace_event/trace_config.cc

@@ -182,6 +182,23 @@ bool TraceConfig::EventFilterConfig::IsCategoryGroupEnabled(
   return category_filter_.IsCategoryGroupEnabled(category_group_name);
 }
 
+// static
+std::string TraceConfig::TraceRecordModeToStr(TraceRecordMode record_mode) {
+  switch (record_mode) {
+    case RECORD_UNTIL_FULL:
+      return kRecordUntilFull;
+    case RECORD_CONTINUOUSLY:
+      return kRecordContinuously;
+    case RECORD_AS_MUCH_AS_POSSIBLE:
+      return kRecordAsMuchAsPossible;
+    case ECHO_TO_CONSOLE:
+      return kTraceToConsole;
+    default:
+      NOTREACHED();
+  }
+  return kRecordUntilFull;
+}
+
 TraceConfig::TraceConfig() {
   InitializeDefault();
 }
@@ -193,24 +210,8 @@ TraceConfig::TraceConfig(StringPiece category_filter_string,
 
 TraceConfig::TraceConfig(StringPiece category_filter_string,
                          TraceRecordMode record_mode) {
-  std::string trace_options_string;
-  switch (record_mode) {
-    case RECORD_UNTIL_FULL:
-      trace_options_string = kRecordUntilFull;
-      break;
-    case RECORD_CONTINUOUSLY:
-      trace_options_string = kRecordContinuously;
-      break;
-    case RECORD_AS_MUCH_AS_POSSIBLE:
-      trace_options_string = kRecordAsMuchAsPossible;
-      break;
-    case ECHO_TO_CONSOLE:
-      trace_options_string = kTraceToConsole;
-      break;
-    default:
-      NOTREACHED();
-  }
-  InitializeFromStrings(category_filter_string, trace_options_string);
+  InitializeFromStrings(category_filter_string,
+                        TraceConfig::TraceRecordModeToStr(record_mode));
 }
 
 TraceConfig::TraceConfig(const DictionaryValue& config) {
@@ -470,23 +471,8 @@ void TraceConfig::SetEventFiltersFromConfigList(
 
 std::unique_ptr<DictionaryValue> TraceConfig::ToDict() const {
   auto dict = std::make_unique<DictionaryValue>();
-  switch (record_mode_) {
-    case RECORD_UNTIL_FULL:
-      dict->SetString(kRecordModeParam, kRecordUntilFull);
-      break;
-    case RECORD_CONTINUOUSLY:
-      dict->SetString(kRecordModeParam, kRecordContinuously);
-      break;
-    case RECORD_AS_MUCH_AS_POSSIBLE:
-      dict->SetString(kRecordModeParam, kRecordAsMuchAsPossible);
-      break;
-    case ECHO_TO_CONSOLE:
-      dict->SetString(kRecordModeParam, kTraceToConsole);
-      break;
-    default:
-      NOTREACHED();
-  }
-
+  dict->SetString(kRecordModeParam,
+                  TraceConfig::TraceRecordModeToStr(record_mode_));
   dict->SetBoolean(kEnableSystraceParam, enable_systrace_);
   dict->SetBoolean(kEnableArgumentFilterParam, enable_argument_filter_);
 

base/trace_event/trace_config.h

@@ -120,6 +120,8 @@ class BASE_EXPORT TraceConfig {
   };
   typedef std::vector<EventFilterConfig> EventFilters;
 
+  static std::string TraceRecordModeToStr(TraceRecordMode record_mode);
+
   TraceConfig();
 
   // Create TraceConfig object from category filter and trace options strings.

chrome/browser/tracing/background_tracing_field_trial_unittest.cc

@@ -34,7 +34,7 @@ TEST_P(BackgroundTracingTest, SetupBackgroundTracingFieldTrial) {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kTraceStartup);
     // Normally is runned from ContentMainRunnerImpl::Initialize().
-    tracing::EnableStartupTracingIfNeeded(false);
+    tracing::EnableStartupTracingIfNeeded();
   }
 
   base::FieldTrialList field_trial_list(nullptr);

components/tracing/common/trace_config_file.cc

@@ -126,6 +126,10 @@ bool TraceConfigFile::IsEnabled() const {
   return is_enabled_;
 }
 
+void TraceConfigFile::SetDisabled() {
+  is_enabled_ = false;
+}
+
 base::trace_event::TraceConfig TraceConfigFile::GetTraceConfig() const {
   DCHECK(IsEnabled());
   return trace_config_;

components/tracing/common/trace_config_file.h

@@ -73,7 +73,15 @@ class TRACING_EXPORT TraceConfigFile {
  public:
   static TraceConfigFile* GetInstance();
 
+  // IsEnabled() returns true if a valid trace config file is specified and
+  // either we are passed the trace duration, if it is positive, or startup
+  // tracing is stopped by other means, e.g. via DevTools protocol.
   bool IsEnabled() const;
+
+  // SetDisabled() is used by the tracing controller to indicate that startup
+  // tracing is finished.
+  void SetDisabled();
+
   base::trace_event::TraceConfig GetTraceConfig() const;
   int GetStartupDuration() const;
 #if !defined(OS_ANDROID)

components/tracing/common/trace_startup.cc

@@ -14,7 +14,7 @@
 
 namespace tracing {
 
-void EnableStartupTracingIfNeeded(bool can_access_file_system) {
+void EnableStartupTracingIfNeeded() {
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
 
@@ -25,7 +25,7 @@ void EnableStartupTracingIfNeeded(bool can_access_file_system) {
   if (command_line.HasSwitch(switches::kTraceStartup)) {
     base::trace_event::TraceConfig trace_config(
         command_line.GetSwitchValueASCII(switches::kTraceStartup),
-        base::trace_event::RECORD_UNTIL_FULL);
+        command_line.GetSwitchValueASCII(switches::kTraceStartupRecordMode));
     base::trace_event::TraceLog::GetInstance()->SetEnabled(
         trace_config, base::trace_event::TraceLog::RECORDING_MODE);
   } else if (command_line.HasSwitch(switches::kTraceToConsole)) {
@@ -36,8 +36,7 @@ void EnableStartupTracingIfNeeded(bool can_access_file_system) {
                << trace_config.ToCategoryFilterString() << "'.";
     base::trace_event::TraceLog::GetInstance()->SetEnabled(
         trace_config, base::trace_event::TraceLog::RECORDING_MODE);
-  } else if (can_access_file_system &&
-             tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
+  } else if (tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
     const base::trace_event::TraceConfig& trace_config =
         tracing::TraceConfigFile::GetInstance()->GetTraceConfig();
     uint8_t modes = base::trace_event::TraceLog::RECORDING_MODE;

components/tracing/common/trace_startup.h

@@ -10,9 +10,7 @@
 namespace tracing {
 
 // Enables TraceLog with config based on the command line flags of the process.
-// If |can_access_file_system| is false then TraceLog is not enabled in case it
-// is required to read config file to start tracing.
-void TRACING_EXPORT EnableStartupTracingIfNeeded(bool can_access_file_system);
+void TRACING_EXPORT EnableStartupTracingIfNeeded();
 
 }  // namespace tracing
 

components/tracing/common/tracing_switches.cc

@@ -46,6 +46,10 @@ 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
+// "record-until-full" mode will be used.
+const char kTraceStartupRecordMode[] = "trace-startup-record-mode";
+
 // Sends a pretty-printed version of tracing info to the console.
 const char kTraceToConsole[]                = "trace-to-console";
 

components/tracing/common/tracing_switches.h

@@ -15,6 +15,7 @@ TRACING_EXPORT extern const char kTraceShutdownFile[];
 TRACING_EXPORT extern const char kTraceStartup[];
 TRACING_EXPORT extern const char kTraceStartupDuration[];
 TRACING_EXPORT extern const char kTraceStartupFile[];
+TRACING_EXPORT extern const char kTraceStartupRecordMode[];
 TRACING_EXPORT extern const char kTraceToConsole[];
 TRACING_EXPORT extern const char kTraceUploadURL[];
 

content/app/android/library_loader_hooks.cc

@@ -14,7 +14,7 @@ namespace content {
 
 bool LibraryLoaded(JNIEnv* env, jclass clazz) {
   // Enable startup tracing asap to avoid early TRACE_EVENT calls being ignored.
-  tracing::EnableStartupTracingIfNeeded(true /* can_access_file_system */);
+  tracing::EnableStartupTracingIfNeeded();
 
   // Android's main browser loop is custom so we set the browser
   // name here as early as possible.

content/app/content_main_runner.cc

@@ -336,6 +336,10 @@ int RunZygote(ContentMainDelegate* delegate) {
       command_line.GetSwitchValueASCII(switches::kProcessType);
   ContentClientInitializer::Set(process_type, delegate);
 
+#if !defined(OS_ANDROID)
+  tracing::EnableStartupTracingIfNeeded();
+#endif  // !OS_ANDROID
+
   MainFunctionParams main_params(command_line);
   main_params.zygote_child = true;
 
@@ -544,12 +548,11 @@ class ContentMainRunnerImpl : public ContentMainRunner {
     // Enable startup tracing asap to avoid early TRACE_EVENT calls being
     // ignored. For Android, startup tracing is enabled in an even earlier place
     // content/app/android/library_loader_hooks.cc.
-    // Zygote process does not have file thread and renderer process on Win10
-    // cannot access the file system.
-    // TODO(ssid): Check if other processes can enable startup tracing here.
-    bool can_access_file_system = (process_type != switches::kZygoteProcess &&
-                                   process_type != switches::kRendererProcess);
-    tracing::EnableStartupTracingIfNeeded(can_access_file_system);
+    //
+    // Startup tracing flags are not (and should not) passed to Zygote
+    // processes. We will enable tracing when forked, if needed.
+    if (process_type != switches::kZygoteProcess)
+      tracing::EnableStartupTracingIfNeeded();
 #endif  // !OS_ANDROID
 
 #if defined(OS_WIN)

content/browser/browser_child_process_host_impl.cc

@@ -26,8 +26,10 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
+#include "components/tracing/common/trace_config_file.h"
 #include "components/tracing/common/tracing_switches.h"
 #include "content/browser/bad_message.h"
+#include "content/browser/browser_main_loop.h"
 #include "content/browser/histogram_controller.h"
 #include "content/browser/loader/resource_message_filter.h"
 #include "content/browser/service_manager/service_manager_context.h"
@@ -209,6 +211,44 @@ void BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(
       switches::kDisableFeatures, cmd_line);
 }
 
+// static
+void BrowserChildProcessHostImpl::CopyTraceStartupFlags(
+    base::CommandLine* cmd_line) {
+  const base::CommandLine& browser_cmd_line =
+      *base::CommandLine::ForCurrentProcess();
+
+  if (browser_cmd_line.HasSwitch(switches::kTraceStartup) &&
+      BrowserMainLoop::GetInstance()->is_tracing_startup_for_duration()) {
+    // Pass kTraceStartup switch to renderer only if startup tracing has not
+    // finished.
+    cmd_line->AppendSwitchASCII(
+        switches::kTraceStartup,
+        browser_cmd_line.GetSwitchValueASCII(switches::kTraceStartup));
+    if (browser_cmd_line.HasSwitch(switches::kTraceStartupRecordMode)) {
+      cmd_line->AppendSwitchASCII(switches::kTraceStartupRecordMode,
+                                  browser_cmd_line.GetSwitchValueASCII(
+                                      switches::kTraceStartupRecordMode));
+    }
+  } else if (tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
+    const auto trace_config =
+        tracing::TraceConfigFile::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()));
+    }
+  }
+}
+
 void BrowserChildProcessHostImpl::Launch(
     std::unique_ptr<SandboxedProcessLauncherDelegate> delegate,
     std::unique_ptr<base::CommandLine> cmd_line,

content/browser/browser_child_process_host_impl.h

@@ -64,6 +64,10 @@ class CONTENT_EXPORT BrowserChildProcessHostImpl
   // from FieldTrials.
   static void CopyFeatureAndFieldTrialFlags(base::CommandLine* cmd_line);
 
+  // Appends kTraceStartup and kTraceRecordMode flags to the command line, if
+  // needed.
+  static void CopyTraceStartupFlags(base::CommandLine* cmd_line);
+
   // BrowserChildProcessHost implementation:
   bool Send(IPC::Message* message) override;
   void Launch(std::unique_ptr<SandboxedProcessLauncherDelegate> delegate,

content/browser/browser_main_loop.cc

@@ -1606,7 +1606,8 @@ void BrowserMainLoop::InitializeMojo() {
   if (parsed_command_line_.HasSwitch(switches::kTraceStartup)) {
     base::trace_event::TraceConfig trace_config(
         parsed_command_line_.GetSwitchValueASCII(switches::kTraceStartup),
-        base::trace_event::RECORD_UNTIL_FULL);
+        parsed_command_line_.GetSwitchValueASCII(
+            switches::kTraceStartupRecordMode));
     TracingController::GetInstance()->StartTracing(
         trace_config, TracingController::StartTracingDoneCallback());
   } else if (parsed_command_line_.HasSwitch(switches::kTraceToConsole)) {

content/browser/browser_main_runner.cc

@@ -26,7 +26,6 @@
 #include "content/browser/browser_shutdown_profile_dumper.h"
 #include "content/browser/notification_service_impl.h"
 #include "content/common/content_switches_internal.h"
-#include "content/public/browser/tracing_controller.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/main_function_params.h"
 #include "third_party/skia/include/core/SkGraphics.h"
@@ -191,8 +190,7 @@ class BrowserMainRunnerImpl : public BrowserMainRunner {
         startup_profiler.reset(
             new BrowserShutdownProfileDumper(main_loop_->startup_trace_file()));
       }
-    } else if (tracing::TraceConfigFile::GetInstance()->IsEnabled() &&
-               TracingController::GetInstance()->IsTracing()) {
+    } else if (tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
       base::FilePath result_file;
 #if defined(OS_ANDROID)
       TracingControllerAndroid::GenerateTracingFilePath(&result_file);

content/browser/devtools/protocol/tracing_handler.cc

@@ -490,8 +490,7 @@ bool TracingHandler::IsTracing() const {
 }
 
 bool TracingHandler::IsStartupTracingActive() {
-  return ::tracing::TraceConfigFile::GetInstance()->IsEnabled() &&
-      TracingController::GetInstance()->IsTracing();
+  return ::tracing::TraceConfigFile::GetInstance()->IsEnabled();
 }
 
 // static

content/browser/gpu/gpu_process_host.cc

@@ -152,8 +152,6 @@ static const char* const kSwitchNames[] = {
     switches::kNoSandbox,
     switches::kRunAllCompositorStagesBeforeDraw,
     switches::kTestGLLib,
-    switches::kTraceConfigFile,
-    switches::kTraceStartup,
     switches::kTraceToConsole,
     switches::kUseFakeJpegDecodeAccelerator,
     switches::kUseGpuInTests,
@@ -1223,6 +1221,7 @@ bool GpuProcessHost::LaunchGpuProcess() {
   cmd_line->AppendSwitchASCII(switches::kProcessType, switches::kGpuProcess);
 
   BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(cmd_line.get());
+  BrowserChildProcessHostImpl::CopyTraceStartupFlags(cmd_line.get());
 
 #if defined(OS_WIN)
   cmd_line->AppendArg(switches::kPrefetchArgumentGpu);

content/browser/ppapi_plugin_process_host.cc

@@ -351,6 +351,7 @@ bool PpapiPluginProcessHost::Init(const PepperPluginInfo& info) {
                               is_broker_ ? switches::kPpapiBrokerProcess
                                          : switches::kPpapiPluginProcess);
   BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(cmd_line.get());
+  BrowserChildProcessHostImpl::CopyTraceStartupFlags(cmd_line.get());
 
 #if defined(OS_WIN)
   cmd_line->AppendArg(is_broker_ ? switches::kPrefetchArgumentPpapiBroker

content/browser/renderer_host/render_process_host_impl.cc

@@ -2655,7 +2655,6 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
     switches::kTestType,
     switches::kTouchEventFeatureDetection,
     switches::kTouchTextSelectionStrategy,
-    switches::kTraceConfigFile,
     switches::kTraceToConsole,
     switches::kUseFakeUIForMediaStream,
     // This flag needs to be propagated to the renderer process for
@@ -2734,15 +2733,7 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
                                  arraysize(kSwitchNames));
 
   BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(renderer_cmd);
-
-  if (browser_cmd.HasSwitch(switches::kTraceStartup) &&
-      BrowserMainLoop::GetInstance()->is_tracing_startup_for_duration()) {
-    // Pass kTraceStartup switch to renderer only if startup tracing has not
-    // finished.
-    renderer_cmd->AppendSwitchASCII(
-        switches::kTraceStartup,
-        browser_cmd.GetSwitchValueASCII(switches::kTraceStartup));
-  }
+  BrowserChildProcessHostImpl::CopyTraceStartupFlags(renderer_cmd);
 
 #if BUILDFLAG(ENABLE_WEBRTC)
   // Only run the Stun trials in the first renderer.

content/browser/tracing/tracing_controller_impl.cc

@@ -21,6 +21,7 @@
 #include "base/trace_event/trace_config.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "components/tracing/common/trace_config_file.h"
 #include "content/browser/tracing/file_tracing_provider_impl.h"
 #include "content/browser/tracing/tracing_ui.h"
 #include "content/public/browser/browser_thread.h"
@@ -326,6 +327,7 @@ bool TracingControllerImpl::StopTracing(
     return false;
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  tracing::TraceConfigFile::GetInstance()->SetDisabled();
   trace_data_endpoint_ = std::move(trace_data_endpoint);
   is_data_complete_ = false;
   is_metadata_available_ = false;

content/browser/utility_process_host.cc

@@ -255,6 +255,7 @@ bool UtilityProcessHost::StartProcess() {
     cmd_line->AppendSwitchASCII(switches::kProcessType,
                                 switches::kUtilityProcess);
     BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(cmd_line.get());
+    BrowserChildProcessHostImpl::CopyTraceStartupFlags(cmd_line.get());
     std::string locale = GetContentClient()->browser()->GetApplicationLocale();
     cmd_line->AppendSwitchASCII(switches::kLang, locale);