Reland "[tracing] Align --trace-startup and --enable-tracing."

This is a reland of 0823dbae

Original change's description:
> [tracing] Align --trace-startup and --enable-tracing.
>
> Use StartupTracingController both for --trace-startup (primary use case:
> browser startup perf profiling) and --enable-tracing (primary use case:
> browser test tracing).
>
> Due to historical reasons both flags are kept, but now their behaviour
> is mostly the same. If both are present, --trace-startup takes
> precedence.
>
> Differences:
> - `trace-startup` finishes after 5 seconds by default, while `enable-tracing`
>   continues until the browser is closed. (--trace-startup-duration overrides
>   both).
> - `trace-startup-file` always treats the path as absolute, while if
>   `enable-tracing-output` ends with /, it uses the provided value as the
>   directory and generates the base name based on the current runtime
>   metadata.
>
> R=skyostil@chromium.org,eseckler@chromium.org
> BUG=1157954
>
> Change-Id: Ia76316aa8927b431784e2fb94c8e42e4e2c0b479
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589834
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Eric Seckler <eseckler@chromium.org>
> Commit-Queue: Alexander Timin <altimin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#839992}

TBR=skyostil@chromium.org,eseckler@chromium.org,nasko@chromium.org

Bug: 1157954
Change-Id: Ic16992026beebddea39c8a6b247b4301512057e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2609139
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840015}

components/tracing/common/trace_startup_config.cc

@@ -88,11 +88,11 @@ TraceStartupConfig::TraceStartupConfig() {
     DCHECK(IsEnabled());
     DCHECK(!IsTracingStartupForDuration());
     DCHECK_EQ(SessionOwner::kBackgroundTracing, session_owner_);
-    CHECK(!ShouldTraceToResultFile());
+    CHECK(GetResultFile().empty());
   } else if (EnableFromATrace()) {
     DCHECK(IsEnabled());
     DCHECK_EQ(SessionOwner::kSystemTracing, session_owner_);
-    CHECK(!ShouldTraceToResultFile());
+    CHECK(GetResultFile().empty());
   }
 }
 
@@ -126,20 +126,11 @@ TraceStartupConfig::OutputFormat TraceStartupConfig::GetOutputFormat() const {
   return output_format_;
 }
 
-bool TraceStartupConfig::ShouldTraceToResultFile() const {
-  return IsEnabled() && should_trace_to_result_file_;
-}
-
 base::FilePath TraceStartupConfig::GetResultFile() const {
   DCHECK(IsEnabled());
-  DCHECK(ShouldTraceToResultFile());
   return result_file_;
 }
 
-void TraceStartupConfig::OnTraceToResultFileFinished() {
-  finished_writing_to_file_ = true;
-}
-
 void TraceStartupConfig::SetBackgroundStartupTracingEnabled(bool enabled) {
 #if defined(OS_ANDROID)
   base::android::SetBackgroundStartupTracingFlag(enabled);
@@ -172,25 +163,41 @@ bool TraceStartupConfig::EnableFromCommandLine() {
                     << "=" << startup_duration_str << " defaulting to 5 (secs)";
       startup_duration_in_seconds_ = kDefaultStartupDurationInSeconds;
     }
+  } else if (command_line->HasSwitch(switches::kEnableTracing)) {
+    // For --enable-tracing, tracing should last until browser shutdown.
+    startup_duration_in_seconds_ = 0;
   }
 
-  if (command_line->GetSwitchValueASCII(switches::kTraceStartupFormat) ==
-      "proto") {
-    // Default is "json".
+  if (command_line->HasSwitch(switches::kTraceStartupFormat)) {
+    if (command_line->GetSwitchValueASCII(switches::kTraceStartupFormat) ==
+        "proto") {
+      // Default is "json".
+      output_format_ = OutputFormat::kProto;
+    }
+  } else if (command_line->GetSwitchValueASCII(
+                 switches::kEnableTracingFormat) == "proto") {
     output_format_ = OutputFormat::kProto;
   }
 
-  if (!command_line->HasSwitch(switches::kTraceStartup))
+  if (!command_line->HasSwitch(switches::kTraceStartup) &&
+      !command_line->HasSwitch(switches::kEnableTracing)) {
     return false;
+  }
+
+  std::string categories;
+  if (command_line->HasSwitch(switches::kTraceStartup)) {
+    categories = command_line->GetSwitchValueASCII(switches::kTraceStartup);
+  } else {
+    categories = command_line->GetSwitchValueASCII(switches::kEnableTracing);
+  }
 
   trace_config_ = base::trace_event::TraceConfig(
-      command_line->GetSwitchValueASCII(switches::kTraceStartup),
+      categories,
       command_line->GetSwitchValueASCII(switches::kTraceStartupRecordMode));
 
   result_file_ = command_line->GetSwitchValuePath(switches::kTraceStartupFile);
 
   is_enabled_ = true;
-  should_trace_to_result_file_ = true;
   return true;
 }
 
@@ -225,7 +232,6 @@ bool TraceStartupConfig::EnableFromConfigFile() {
 
   if (trace_config_file.empty()) {
     is_enabled_ = true;
-    should_trace_to_result_file_ = true;
     DLOG(WARNING) << "Use default trace config.";
     return true;
   }
@@ -245,7 +251,6 @@ bool TraceStartupConfig::EnableFromConfigFile() {
   is_enabled_ = ParseTraceConfigFileContent(trace_config_file_content);
   if (!is_enabled_)
     DLOG(WARNING) << "Cannot parse the trace config file correctly.";
-  should_trace_to_result_file_ = is_enabled_;
   return is_enabled_;
 }
 
@@ -267,7 +272,6 @@ bool TraceStartupConfig::EnableFromBackgroundTracing() {
 
   is_enabled_ = true;
   session_owner_ = SessionOwner::kBackgroundTracing;
-  should_trace_to_result_file_ = false;
   // Set startup duration to 0 since background tracing config will configure
   // the durations later.
   startup_duration_in_seconds_ = 0;

components/tracing/common/trace_startup_config.h

@@ -123,21 +123,12 @@ class TRACING_EXPORT TraceStartupConfig {
   base::trace_event::TraceConfig GetTraceConfig() const;
   int GetStartupDuration() const;
 
-  // Returns true while startup tracing is not finished, if trace should be
-  // saved to result file.
-  bool ShouldTraceToResultFile() const;
+  // Returns the name of the file to write the trace result into.
   base::FilePath GetResultFile() const;
-  void OnTraceToResultFileFinished();
 
   // Set the background tracing config in preferences for the next session.
   void SetBackgroundStartupTracingEnabled(bool enabled);
 
-  // Returns when the startup tracing is finished and written to file, false on
-  // all other cases.
-  bool finished_writing_to_file_for_testing() const {
-    return finished_writing_to_file_;
-  }
-
   SessionOwner GetSessionOwner() const;
 
   OutputFormat GetOutputFormat() const;
@@ -170,9 +161,7 @@ class TRACING_EXPORT TraceStartupConfig {
   bool enable_background_tracing_for_testing_ = false;
   base::trace_event::TraceConfig trace_config_;
   int startup_duration_in_seconds_ = kDefaultStartupDurationInSeconds;
-  bool should_trace_to_result_file_ = false;
   base::FilePath result_file_;
-  bool finished_writing_to_file_ = false;
   SessionOwner session_owner_ = SessionOwner::kTracingController;
   bool session_adopted_ = false;
   OutputFormat output_format_ = OutputFormat::kLegacyJSON;

components/tracing/common/tracing_switches.cc

@@ -18,14 +18,25 @@ const char kTraceConfigFile[]               = "trace-config-file";
 // specify the specific trace categories to include (e.g.
 // --trace-startup=base,net) otherwise, all events are recorded. Setting this
 // flag results in the first call to BeginTracing() to receive all trace events
-// since startup. In Chrome, you may find --trace-startup-file and
+// since startup.
+//
+// Historically, --trace-startup was used for browser startup profiling and
+// --enable-tracing was used for browsertest tracing. Now they are share the
+// same implementation, but both are still supported to avoid disrupting
+// existing workflows. The only difference between them is the default duration
+// (5 seconds for trace-startup, unlimited for enable-tracing). If both are
+// specified, 'trace-startup' takes precedence.
+//
+// In Chrome, you may find --trace-startup-file and
 // --trace-startup-duration to control the auto-saving of the trace (not
 // supported in the base-only TraceLog component).
-const char kTraceStartup[]                  = "trace-startup";
+const char kTraceStartup[] = "trace-startup";
+const char kEnableTracing[] = "enable-tracing";
 
-// Sets the time in seconds until startup tracing ends. If omitted a default of
-// 5 seconds is used. Has no effect without --trace-startup, or if
-// --startup-trace-file=none was supplied.
+// Sets the time in seconds until startup tracing ends. If omitted:
+// - if --trace-startup is specified, a default of 5 seconds is used.
+// - if --enable-tracing is specified, tracing lasts until the browser is
+// closed. Has no effect otherwise.
 const char kTraceStartupDuration[]          = "trace-startup-duration";
 
 // If supplied, sets the file which startup tracing will be stored into, if
@@ -35,7 +46,16 @@ const char kTraceStartupDuration[]          = "trace-startup-duration";
 // As a special case, can be set to 'none' - this disables automatically saving
 // the result to a file and the first manually recorded trace will then receive
 // all events since startup.
-const char kTraceStartupFile[]              = "trace-startup-file";
+const char kTraceStartupFile[] = "trace-startup-file";
+
+// Similar to the flag above, with the following differences:
+// - A more detailed basename will be generated.
+// - If the value is empty or ends with path separator, the provided directory
+// will be used (with empty standing for current directory) and a detailed
+// basename file will be generated.
+//
+// It is ignored if --trace-startup-file is specified.
+const char kEnableTracingOutput[] = "enable-tracing-output";
 
 // Sets the output format for the trace, valid values are "json" and "proto".
 // If not set, the current default is "json".
@@ -44,6 +64,7 @@ const char kTraceStartupFile[]              = "trace-startup-file";
 // unexpectedly terminates.
 // Ignored if "trace-startup-owner" is not "controller".
 const char kTraceStartupFormat[] = "trace-startup-format";
+const char kEnableTracingFormat[] = "enable-tracing-format";
 
 // If supplied, sets the tracing record mode and options; otherwise, the default
 // "record-until-full" mode will be used.

components/tracing/common/tracing_switches.h

@@ -12,10 +12,13 @@ namespace switches {
 TRACING_EXPORT extern const char kEnableBackgroundTracing[];
 TRACING_EXPORT extern const char kTraceConfigFile[];
 TRACING_EXPORT extern const char kTraceStartup[];
+TRACING_EXPORT extern const char kEnableTracing[];
 TRACING_EXPORT extern const char kTraceStartupDuration[];
 TRACING_EXPORT extern const char kTraceStartupFile[];
+TRACING_EXPORT extern const char kEnableTracingOutput[];
 TRACING_EXPORT extern const char kTraceStartupRecordMode[];
 TRACING_EXPORT extern const char kTraceStartupFormat[];
+TRACING_EXPORT extern const char kEnableTracingFormat[];
 TRACING_EXPORT extern const char kTraceStartupOwner[];
 TRACING_EXPORT extern const char kTraceStartupEnablePrivacyFiltering[];
 TRACING_EXPORT extern const char kPerfettoDisableInterning[];

content/browser/tracing/startup_tracing_browsertest.cc

@@ -12,6 +12,7 @@
 #include "build/build_config.h"
 #include "components/tracing/common/trace_startup_config.h"
 #include "components/tracing/common/tracing_switches.h"
+#include "content/browser/tracing/startup_tracing_controller.h"
 #include "content/browser/tracing/tracing_controller_impl.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/content_browser_test.h"
@@ -42,52 +43,6 @@ void WaitForCondition(base::RepeatingCallback<bool()> condition,
 
 }  // namespace
 
-class CommandlineStartupTracingTest : public ContentBrowserTest {
- public:
-  CommandlineStartupTracingTest() = default;
-
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    base::CreateTemporaryFile(&temp_file_path_);
-    command_line->AppendSwitch(switches::kTraceStartup);
-    command_line->AppendSwitchASCII(switches::kTraceStartupDuration, "3");
-    command_line->AppendSwitchASCII(switches::kTraceStartupFile,
-                                    temp_file_path_.AsUTF8Unsafe());
-  }
-
- protected:
-  base::FilePath temp_file_path_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(CommandlineStartupTracingTest);
-};
-
-// Failing on Android/Win ASAN, Linux TSAN. crbug.com/1041392
-#if (defined(OS_ANDROID) && defined(ADDRESS_SANITIZER)) || \
-    (defined(OS_WIN) && defined(ADDRESS_SANITIZER)) ||     \
-    ((defined(OS_LINUX) || defined(OS_CHROMEOS)) && defined(THREAD_SANITIZER))
-#define MAYBE_TestStartupTracing DISABLED_TestStartupTracing
-#else
-#define MAYBE_TestStartupTracing TestStartupTracing
-#endif
-IN_PROC_BROWSER_TEST_F(CommandlineStartupTracingTest,
-                       MAYBE_TestStartupTracing) {
-  EXPECT_TRUE(NavigateToURL(shell(), GetTestUrl("", "title1.html")));
-  WaitForCondition(base::BindRepeating([]() {
-                     return tracing::TraceStartupConfig::GetInstance()
-                         ->finished_writing_to_file_for_testing();
-                   }),
-                   "finish file write");
-
-  std::string trace;
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  ASSERT_TRUE(base::ReadFileToString(temp_file_path_, &trace));
-  EXPECT_TRUE(base::JSONReader::Read(trace));
-  EXPECT_TRUE(trace.find("StartupTracingController::Start") !=
-              std::string::npos);
-}
-
-#undef MAYBE_TestStartupTracing
-
 class StartupTracingInProcessTest : public ContentBrowserTest {
  public:
   StartupTracingInProcessTest() {
@@ -154,4 +109,210 @@ IN_PROC_BROWSER_TEST_F(StartupTracingInProcessTest, TestFilledStartupBuffer) {
   wait_for_stop.Run();
 }
 
+namespace {
+
+enum class FinishType {
+  kWaitForTimeout,
+  kStopExplicitly,
+};
+
+std::ostream& operator<<(std::ostream& o, FinishType type) {
+  switch (type) {
+    case FinishType::kStopExplicitly:
+      o << "Stop";
+      return o;
+    case FinishType::kWaitForTimeout:
+      o << "Wait";
+      return o;
+  }
+}
+
+enum class OutputType {
+  kProto,
+  kJSON,
+};
+
+std::ostream& operator<<(std::ostream& o, OutputType type) {
+  switch (type) {
+    case OutputType::kJSON:
+      o << "json";
+      return o;
+    case OutputType::kProto:
+      o << "proto";
+      return o;
+  }
+}
+
+enum class OutputLocation {
+  // Write trace to a given file.
+  kGivenFile,
+  // Write trace into a given directory (basename will be set to trace1 before
+  // starting).
+  kDirectoryWithDefaultBasename,
+  // Write trace into a given directory (basename will be set to trace1 before
+  // starting, and updated to trace2 before calling Stop()).
+  kDirectoryWithBasenameUpdatedBeforeStop,
+};
+
+std::ostream& operator<<(std::ostream& o, OutputLocation type) {
+  switch (type) {
+    case OutputLocation::kGivenFile:
+      o << "file";
+      return o;
+    case OutputLocation::kDirectoryWithDefaultBasename:
+      o << "dir/trace1";
+      return o;
+    case OutputLocation::kDirectoryWithBasenameUpdatedBeforeStop:
+      o << "dir/trace2";
+      return o;
+  }
+}
+
+}  // namespace
+
+class StartupTracingTest
+    : public ContentBrowserTest,
+      public testing::WithParamInterface<
+          std::tuple<FinishType, OutputType, OutputLocation>> {
+ public:
+  StartupTracingTest() = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kTraceStartup);
+    if (GetFinishType() == FinishType::kWaitForTimeout) {
+      command_line->AppendSwitchASCII(switches::kTraceStartupDuration, "3");
+    } else {
+      command_line->AppendSwitchASCII(switches::kTraceStartupDuration, "0");
+    }
+    command_line->AppendSwitchASCII(switches::kTraceStartupFormat,
+                                    GetOutputTypeAsString());
+
+    if (GetOutputLocation() == OutputLocation::kGivenFile) {
+      base::CreateTemporaryFile(&temp_file_path_);
+    } else {
+      base::CreateNewTempDirectory(base::FilePath::StringType(),
+                                   &temp_file_path_);
+      temp_file_path_ = temp_file_path_.AsEndingWithSeparator();
+    }
+
+    command_line->AppendSwitchASCII(switches::kEnableTracingOutput,
+                                    temp_file_path_.AsUTF8Unsafe());
+
+    if (GetOutputLocation() != OutputLocation::kGivenFile) {
+      // --enable-tracing-format switch should be initialised before
+      // calling SetDefaultBasenameForTest, which forces the creation of
+      // TraceStartupConfig, which queries the command line flags and
+      // stores the snapshot.
+      StartupTracingController::GetInstance().SetDefaultBasenameForTest(
+          "trace1",
+          StartupTracingController::ExtensionType::kAppendAppropriate);
+    }
+  }
+
+  FinishType GetFinishType() { return std::get<0>(GetParam()); }
+
+  OutputType GetOutputType() { return std::get<1>(GetParam()); }
+
+  std::string GetOutputTypeAsString() {
+    switch (GetOutputType()) {
+      case OutputType::kJSON:
+        return "json";
+      case OutputType::kProto:
+        return "proto";
+    }
+  }
+
+  OutputLocation GetOutputLocation() { return std::get<2>(GetParam()); }
+
+  base::FilePath GetExpectedPath() {
+    std::string filename;
+
+    switch (GetOutputLocation()) {
+      case OutputLocation::kGivenFile:
+        return temp_file_path_;
+      case OutputLocation::kDirectoryWithDefaultBasename:
+        filename = "trace1";
+        break;
+      case OutputLocation::kDirectoryWithBasenameUpdatedBeforeStop:
+        filename = "trace2";
+        break;
+    }
+
+    // Renames are not supported together with timeouts.
+    if (GetFinishType() == FinishType::kWaitForTimeout)
+      filename = "trace1";
+
+    return temp_file_path_.AppendASCII(filename + "." +
+                                       GetOutputTypeAsString());
+  }
+
+  void CheckOutput(base::FilePath path) {
+    std::string trace;
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    ASSERT_TRUE(base::ReadFileToString(path, &trace))
+        << "Failed to read file " << path;
+
+    if (GetOutputType() == OutputType::kJSON) {
+      EXPECT_TRUE(base::JSONReader::Read(trace));
+    }
+
+    // Both proto and json should have the trace event name recorded somewhere
+    // as a substring.
+    EXPECT_TRUE(trace.find("StartupTracingController::Start") !=
+                std::string::npos);
+  }
+
+  void Wait() {
+    if (GetFinishType() == FinishType::kWaitForTimeout) {
+      WaitForCondition(base::BindRepeating([]() {
+                         return StartupTracingController::GetInstance()
+                             .is_finished_for_testing();
+                       }),
+                       "finish file write");
+    } else {
+      StartupTracingController::GetInstance().WaitUntilStopped();
+    }
+  }
+
+ protected:
+  base::FilePath temp_file_path_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(StartupTracingTest);
+};
+
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    StartupTracingTest,
+    testing::Combine(
+        testing::Values(FinishType::kStopExplicitly,
+                        FinishType::kWaitForTimeout),
+        testing::Values(OutputType::kJSON, OutputType::kProto),
+        testing::Values(
+            OutputLocation::kGivenFile,
+            OutputLocation::kDirectoryWithDefaultBasename,
+            OutputLocation::kDirectoryWithBasenameUpdatedBeforeStop)));
+
+// Failing on Android/Win ASAN, Linux TSAN. crbug.com/1041392
+#if (defined(OS_ANDROID) && defined(ADDRESS_SANITIZER)) || \
+    (defined(OS_WIN) && defined(ADDRESS_SANITIZER)) ||     \
+    ((defined(OS_LINUX) || defined(OS_CHROMEOS)) && defined(THREAD_SANITIZER))
+#define MAYBE_TestEnableTracing DISABLED_TestStartupTracing
+#else
+#define MAYBE_TestEnableTracing TestStartupTracing
+#endif
+IN_PROC_BROWSER_TEST_P(StartupTracingTest, TestEnableTracing) {
+  EXPECT_TRUE(NavigateToURL(shell(), GetTestUrl("", "title1.html")));
+
+  if (GetOutputLocation() ==
+      OutputLocation::kDirectoryWithBasenameUpdatedBeforeStop) {
+    StartupTracingController::GetInstance().SetDefaultBasenameForTest(
+        "trace2", StartupTracingController::ExtensionType::kAppendAppropriate);
+  }
+
+  Wait();
+
+  CheckOutput(GetExpectedPath());
+}
+
 }  // namespace content

content/browser/tracing/startup_tracing_controller.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "content/browser/tracing/startup_tracing_controller.h"
+#include "base/bind_post_task.h"
 #include "base/command_line.h"
 #include "base/files/file.h"
 #include "base/files/file_util.h"
@@ -11,6 +12,7 @@
 #include "base/run_loop.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/typed_macros.h"
 #include "build/build_config.h"
 #include "components/tracing/common/trace_startup_config.h"
@@ -34,12 +36,14 @@ class StartupTracingController::BackgroundTracer {
   enum class WriteMode { kAfterStopping, kStreaming };
 
   BackgroundTracer(WriteMode write_mode,
+                   TempFilePolicy temp_file_policy,
                    base::FilePath output_file,
                    tracing::TraceStartupConfig::OutputFormat output_format,
                    perfetto::TraceConfig trace_config,
                    base::OnceClosure on_tracing_finished)
       : state_(State::kTracing),
         write_mode_(write_mode),
+        temp_file_policy_(temp_file_policy),
         task_runner_(base::SequencedTaskRunnerHandle::Get()),
         output_file_(output_file),
         output_format_(output_format),
@@ -63,11 +67,17 @@ class StartupTracingController::BackgroundTracer {
     TRACE_EVENT("startup", "StartupTracingController::Start");
   }
 
-  void Stop() {
+  void Stop(base::FilePath output_file) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    if (state_ != State::kTracing)
+
+    // Tracing might have already been finished due to a timeout.
+    if (state_ == State::kFinished) {
+      // Note: updating output files is not supported together with
+      // timeout-based tracing.
       return;
+    }
 
+    output_file_ = output_file;
     tracing_session_->StopBlocking();
   }
 
@@ -86,7 +96,7 @@ class StartupTracingController::BackgroundTracer {
     if (write_mode_ == WriteMode::kStreaming) {
       // No need to explicitly call ReadTrace as Perfetto has already written
       // the file.
-      Finalise(output_file_);
+      Finalise();
       return;
     }
     state_ = State::kWritingToFile;
@@ -102,7 +112,7 @@ class StartupTracingController::BackgroundTracer {
           if (args.has_more)
             return;
 
-          Finalise(output_file_);
+          Finalise();
         });
   }
 
@@ -138,16 +148,16 @@ class StartupTracingController::BackgroundTracer {
   // In order to atomically commit the trace file, create a temporary file first
   // which then will be subsequently renamed.
   void OpenFile(const base::FilePath& path) {
-    file_ = base::CreateAndOpenTemporaryFileInDir(path.DirName(),
-                                                  &written_to_file_);
-    if (file_.IsValid()) {
-      LOG(ERROR) << "Created valid file";
-      return;
+    if (temp_file_policy_ == TempFilePolicy::kUseTemporaryFile) {
+      file_ = base::CreateAndOpenTemporaryFileInDir(path.DirName(),
+                                                    &written_to_file_);
+      if (file_.IsValid())
+        return;
+
+      VLOG(1) << "Failed to create temporary file, using file '" << path
+              << "' directly instead";
     }
 
-    VLOG(1) << "Failed to create temporary file, using file '" << path
-            << "' directly instead";
-
     // On Android, it might not be possible to create a temporary file.
     // In that case, we should use the file directly.
     file_.Initialize(output_file_,
@@ -159,23 +169,25 @@ class StartupTracingController::BackgroundTracer {
   }
 
   // Close the file and rename if needed.
-  void Finalise(const base::FilePath& path) {
+  void Finalise() {
     DCHECK_NE(state_, State::kFinished);
     file_.Close();
 
-    if (written_to_file_ != path) {
+    if (written_to_file_ != output_file_) {
       base::File::Error error;
       if (!base::ReplaceFile(written_to_file_, output_file_, &error)) {
         LOG(ERROR) << "Cannot move file '" << written_to_file_ << "' to '"
                    << output_file_
                    << "' : " << base::File::ErrorToString(error);
+      } else {
+        written_to_file_ = output_file_;
       }
     }
 
+    VLOG(0) << "Completed startup tracing to " << written_to_file_;
+
     state_ = State::kFinished;
     std::move(on_tracing_finished_).Run();
-
-    VLOG(0) << "Completed startup tracing to " << path;
   }
 
   enum class State {
@@ -186,11 +198,17 @@ class StartupTracingController::BackgroundTracer {
   State state_;
 
   const WriteMode write_mode_;
+  const TempFilePolicy temp_file_policy_;
 
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
+  // Output file might be customised during the execution (e.g. test result
+  // becomes available), which means that if Perfetto has already started
+  // streaming the trace, the trace file should be renamed after trace
+  // completes.
   base::FilePath output_file_;
   base::FilePath written_to_file_;
+
   base::File file_;
 
   const tracing::TraceStartupConfig::OutputFormat output_format_;
@@ -200,27 +218,30 @@ class StartupTracingController::BackgroundTracer {
   std::unique_ptr<tracing::TracePacketTokenizer> trace_packet_tokenizer_;
 
   base::OnceClosure on_tracing_finished_;
+
   std::unique_ptr<perfetto::TracingSession> tracing_session_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 };
 
-namespace {
+// static
+StartupTracingController& StartupTracingController::GetInstance() {
+  // Note: no DCHECK_CURRENTLY_ON, as it can be called prior to initialisation
+  // of BrowserThreads.
 
-base::FilePath GetStartupTraceFileName() {
-  base::FilePath trace_file;
+  static base::NoDestructor<StartupTracingController> g_instance;
+  return *g_instance;
+}
+
+namespace {
 
-  trace_file = tracing::TraceStartupConfig::GetInstance()->GetResultFile();
-  if (trace_file.empty()) {
+base::FilePath BasenameToPath(std::string basename) {
 #if defined(OS_ANDROID)
-    trace_file = TracingControllerAndroid::GenerateTracingFilePath("");
+  return TracingControllerAndroid::GenerateTracingFilePath(basename);
 #else
-    // Default to saving the startup trace into the current dir.
-    trace_file = base::FilePath().AppendASCII("chrometrace.log");
+  // Default to saving the startup trace into the current dir.
+  return base::FilePath().AppendASCII(basename);
 #endif
-  }
-
-  return trace_file;
 }
 
 }  // namespace
@@ -228,12 +249,46 @@ base::FilePath GetStartupTraceFileName() {
 StartupTracingController::StartupTracingController() = default;
 StartupTracingController::~StartupTracingController() = default;
 
-// static
-StartupTracingController& StartupTracingController::GetInstance() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+base::FilePath StartupTracingController::GetOutputPath() {
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+
+  base::FilePath path_from_config =
+      tracing::TraceStartupConfig::GetInstance()->GetResultFile();
+  if (!path_from_config.empty())
+    return path_from_config;
+
+  // If --trace-startup-file is specified, use it.
+  if (command_line->HasSwitch(switches::kTraceStartupFile)) {
+    base::FilePath result =
+        command_line->GetSwitchValuePath(switches::kTraceStartupFile);
+    if (result.empty())
+      return BasenameToPath("chrometrace.log");
+    return result;
+  }
 
-  static base::NoDestructor<StartupTracingController> g_instance;
-  return *g_instance;
+  base::FilePath result =
+      command_line->GetSwitchValuePath(switches::kEnableTracingOutput);
+  if (result.empty() && command_line->HasSwitch(switches::kTraceStartup)) {
+    // If --trace-startup is present, return chrometrace.log for backwards
+    // compatibility.
+    return BasenameToPath("chrometrace.log");
+  }
+
+  // If a non-directory path is specified, use it.
+  if (!result.empty() && !result.EndsWithSeparator())
+    return result;
+
+  std::string basename = default_basename_;
+  if (basename.empty())
+    basename = "chrometrace.log";
+
+  // If a non-empty directory is specified, use it.
+  if (!result.empty())
+    return result.AppendASCII(basename);
+
+  // If the directory is empty, go through BasenameToPath to generate a valid
+  // path on Android.
+  return BasenameToPath(basename);
 }
 
 void StartupTracingController::StartIfNeeded() {
@@ -283,12 +338,9 @@ void StartupTracingController::StartIfNeeded() {
       tracing::TraceStartupConfig::GetInstance()->GetStartupDuration();
   perfetto_config.set_duration_ms(duration_in_seconds * 1000);
 
-  if (output_file_.empty())
-    output_file_ = GetStartupTraceFileName();
-
   background_tracer_ = base::SequenceBound<BackgroundTracer>(
-      std::move(background_task_runner), write_mode, output_file_,
-      output_format, perfetto_config,
+      std::move(background_task_runner), write_mode, temp_file_policy_,
+      GetOutputPath(), output_format, perfetto_config,
       base::BindOnce(
           [](StartupTracingController* controller) {
             GetUIThreadTaskRunner({})->PostTask(
@@ -302,7 +354,8 @@ void StartupTracingController::StartIfNeeded() {
 void StartupTracingController::Stop(base::OnceClosure on_tracing_finished) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (state_ == State::kNotRunning) {
+  if (state_ != State::kRunning) {
+    // Both kStopped and kNotRunning are valid states.
     std::move(on_tracing_finished).Run();
     return;
   }
@@ -310,22 +363,58 @@ void StartupTracingController::Stop(base::OnceClosure on_tracing_finished) {
   DCHECK(!on_tracing_finished_) << "Stop() should be called only once.";
   on_tracing_finished_ = std::move(on_tracing_finished);
 
-  background_tracer_.AsyncCall(&BackgroundTracer::Stop);
+  background_tracer_.AsyncCall(&BackgroundTracer::Stop)
+      .WithArgs(GetOutputPath());
 }
 
 void StartupTracingController::OnStoppedOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(state_, State::kRunning);
-  state_ = State::kNotRunning;
+  state_ = State::kStopped;
   background_tracer_.Reset();
 
   if (on_tracing_finished_)
     std::move(on_tracing_finished_).Run();
 
-  tracing::TraceStartupConfig::GetInstance()->OnTraceToResultFileFinished();
   tracing::TraceStartupConfig::GetInstance()->SetDisabled();
 }
 
+void StartupTracingController::SetUsingTemporaryFile(
+    StartupTracingController::TempFilePolicy temp_file_policy) {
+  DCHECK_EQ(state_, State::kNotEnabled) << "Should be called before Start()";
+  temp_file_policy_ = temp_file_policy;
+}
+
+void StartupTracingController::SetDefaultBasename(
+    std::string basename,
+    ExtensionType extension_type) {
+  if (!tracing::TraceStartupConfig::GetInstance()->IsEnabled())
+    return;
+
+  if (basename_for_test_set_)
+    return;
+
+  if (extension_type == ExtensionType::kAppendAppropriate) {
+    switch (tracing::TraceStartupConfig::GetInstance()->GetOutputFormat()) {
+      case tracing::TraceStartupConfig::OutputFormat::kLegacyJSON:
+        basename += ".json";
+        break;
+      case tracing::TraceStartupConfig::OutputFormat::kProto:
+        basename += ".proto";
+        break;
+    }
+  }
+  default_basename_ = basename;
+}
+
+void StartupTracingController::SetDefaultBasenameForTest(
+    std::string basename,
+    ExtensionType extension_type) {
+  basename_for_test_set_ = false;
+  SetDefaultBasename(basename, extension_type);
+  basename_for_test_set_ = true;
+}
+
 void StartupTracingController::WaitUntilStopped() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 

content/browser/tracing/startup_tracing_controller.h

@@ -6,13 +6,14 @@
 #define CONTENT_BROWSER_TRACING_STARTUP_TRACING_CONTROLLER_H_
 
 #include "base/threading/sequence_bound.h"
+#include "content/common/content_export.h"
 
 namespace content {
 
 // Class responsible for starting and stopping startup tracing as configured by
 // StartupTracingConfig. All interactions with it are limited to UI thread, but
 // the actual logic lives on a background ThreadPool sequence.
-class StartupTracingController {
+class CONTENT_EXPORT StartupTracingController {
  public:
   StartupTracingController();
   ~StartupTracingController();
@@ -22,16 +23,55 @@ class StartupTracingController {
   void StartIfNeeded();
   void WaitUntilStopped();
 
+  // By default, a trace is written into a temporary file which then is renamed,
+  // however this can lead to data loss when the browser process crashes.
+  // Embedders can disable this (especially if a name provided to
+  // SetDefaultBasename makes it clear that the trace is incomplete and final
+  // name will be provided via SetDefaultBasename call before calling Stop).
+  enum class TempFilePolicy {
+    kUseTemporaryFile,
+    kWriteDirectly,
+  };
+  void SetUsingTemporaryFile(TempFilePolicy temp_file_policy);
+
+  // Set default basename for the trace output file to allow //content embedders
+  // to customise it using some metadata (like test names).
+  //
+  // If --enable-trace-output is a directory (default value, empty, designated
+  // "current directory"), then the startup trace will be written in a file with
+  // the given basename in this directory. Depending on the |extension_type|,
+  // an appropriate extension (.json or .proto) will be added.
+  //
+  // Note that embedders can call it even after tracing has started and Perfetto
+  // started streaming the trace into it — in that case,
+  // StartupTracingController will rename the file after finishing. However,
+  // this is guaranteed to work only when tracing lasts until Stop() (not with
+  // duration-based tracing).
+  enum class ExtensionType {
+    kAppendAppropriate,
+    kNone,
+  };
+  void SetDefaultBasename(std::string basename, ExtensionType extension_type);
+  // As the test harness calls SetDefaultBasename, expose ForTest() version for
+  // the tests checking the StartupTracingController logic itself.
+  void SetDefaultBasenameForTest(std::string basename,
+                                 ExtensionType extension_type);
+
+  bool is_finished_for_testing() const { return state_ == State::kStopped; }
+
  private:
   void Stop(base::OnceClosure on_finished_callback);
 
   void OnStoppedOnUIThread();
 
+  base::FilePath GetOutputPath();
+
   enum class State {
+    kNotEnabled,
     kRunning,
-    kNotRunning,
+    kStopped,
   };
-  State state_ = State::kNotRunning;
+  State state_ = State::kNotEnabled;
 
   // All actual interactions with the tracing service and the process of writing
   // files happens on a background thread.
@@ -40,6 +80,11 @@ class StartupTracingController {
 
   base::OnceClosure on_tracing_finished_;
   base::FilePath output_file_;
+
+  std::string default_basename_;
+  bool basename_for_test_set_ = false;
+
+  TempFilePolicy temp_file_policy_ = TempFilePolicy::kUseTemporaryFile;
 };
 
 }  // namespace content

content/public/common/content_switches.cc

@@ -402,14 +402,6 @@ const char kEnableStrictPowerfulFeatureRestrictions[] =
 // Enabled threaded compositing for web tests.
 const char kEnableThreadedCompositing[]     = "enable-threaded-compositing";
 
-// Enable tracing during the execution of browser tests.
-const char kEnableTracing[]                 = "enable-tracing";
-
-// The filename to write the output of the test tracing to. If it is empty
-// or it ends in a directory separator then an auto-generated filename will be
-// appended.
-const char kEnableTracingOutput[]           = "enable-tracing-output";
-
 // Enable screen capturing support for MediaStream API.
 const char kEnableUserMediaScreenCapturing[] =
     "enable-usermedia-screen-capturing";

content/public/common/content_switches.h

@@ -123,8 +123,6 @@ CONTENT_EXPORT extern const char kEnableSpatialNavigation[];
 CONTENT_EXPORT extern const char kEnableStrictMixedContentChecking[];
 CONTENT_EXPORT extern const char kEnableStrictPowerfulFeatureRestrictions[];
 CONTENT_EXPORT extern const char kEnableThreadedCompositing[];
-CONTENT_EXPORT extern const char kEnableTracing[];
-CONTENT_EXPORT extern const char kEnableTracingOutput[];
 CONTENT_EXPORT extern const char kEnableUserMediaScreenCapturing[];
 CONTENT_EXPORT extern const char kEnableUseZoomForDSF[];
 CONTENT_EXPORT extern const char kEnableViewport[];

content/public/test/browser_test_base.cc

@@ -46,6 +46,7 @@
 #include "content/browser/startup_helper.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/browser/tracing/memory_instrumentation_util.h"
+#include "content/browser/tracing/startup_tracing_controller.h"
 #include "content/browser/tracing/tracing_controller_impl.h"
 #include "content/public/app/content_main.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -166,10 +167,47 @@ void RunTaskOnRendererThread(base::OnceClosure task,
   GetUIThreadTaskRunner({})->PostTask(FROM_HERE, std::move(quit_task));
 }
 
-void TraceStopTracingComplete(base::OnceClosure quit,
-                              const base::FilePath& file_path) {
-  LOG(ERROR) << "Tracing written to: " << file_path.value();
-  std::move(quit).Run();
+enum class TraceBasenameType {
+  kWithoutTestStatus,
+  kWithTestStatus,
+};
+
+std::string GetDefaultTraceBasename(TraceBasenameType type) {
+  std::string test_suite_name = ::testing::UnitTest::GetInstance()
+                                    ->current_test_info()
+                                    ->test_suite_name();
+  std::string test_name =
+      ::testing::UnitTest::GetInstance()->current_test_info()->name();
+  // Parameterised tests might have slashes in their full name — replace them
+  // before using it as a file name to avoid trying to write to an incorrect
+  // location.
+  base::ReplaceChars(test_suite_name, "/", "_", &test_suite_name);
+  base::ReplaceChars(test_name, "/", "_", &test_name);
+  // Add random number to the trace file to distinguish traces from different
+  // test runs. We don't use timestamp here to avoid collisions with parallel
+  // runs of the same test. Browser test runner runs one test per browser
+  // process instantiation, so saving the seed here is appopriate.
+  // GetDefaultTraceBasename() is going to be called twice:
+  // - for the first time, before the test starts to get the name of the file to
+  // stream the results (to avoid losing them if test crashes).
+  // - the second time, if test execution finishes normally, to calculate the
+  // resulting name of the file, including test result.
+  static std::string random_seed =
+      base::NumberToString(base::RandInt(1e7, 1e8 - 1));
+  std::string status;
+  if (type == TraceBasenameType::kWithTestStatus) {
+    status = ::testing::UnitTest::GetInstance()
+                     ->current_test_info()
+                     ->result()
+                     ->Passed()
+                 ? "OK"
+                 : "FAIL";
+  } else {
+    // In order to be able to stream the test to the file,
+    status = "NOT_FINISHED";
+  }
+  return "trace_test_" + test_suite_name + "_" + test_name + "_" + random_seed +
+         "_" + status;
 }
 
 // See SetInitialWebContents comment for more information.
@@ -483,6 +521,16 @@ void BrowserTestBase::SetUp() {
       std::make_unique<CreatedMainPartsClosure>(base::BindOnce(
           &BrowserTestBase::CreatedBrowserMainParts, base::Unretained(this)));
 
+  // If tracing is enabled, customise the output filename based on the name of
+  // the test.
+  StartupTracingController::GetInstance().SetDefaultBasename(
+      GetDefaultTraceBasename(TraceBasenameType::kWithoutTestStatus),
+      StartupTracingController::ExtensionType::kAppendAppropriate);
+  // Write to the provided file directly to recover at least some data when the
+  // test crashes or times out.
+  StartupTracingController::GetInstance().SetUsingTemporaryFile(
+      StartupTracingController::TempFilePolicy::kWriteDirectly);
+
 #if defined(OS_ANDROID)
   // For all other platforms, we call ContentMain for browser tests which goes
   // through the normal browser initialization paths. For Android, we must set
@@ -670,36 +718,6 @@ void BrowserTestBase::WaitUntilJavaIsReady(base::OnceClosure quit_closure) {
 }
 #endif
 
-namespace {
-
-std::string GetDefaultTraceFilename() {
-  std::string test_suite_name = ::testing::UnitTest::GetInstance()
-                                    ->current_test_info()
-                                    ->test_suite_name();
-  std::string test_name =
-      ::testing::UnitTest::GetInstance()->current_test_info()->name();
-  // Parameterised tests might have slashes in their full name — replace them
-  // before using it as a file name to avoid trying to write to an incorrect
-  // location.
-  base::ReplaceChars(test_suite_name, "/", "_", &test_suite_name);
-  base::ReplaceChars(test_name, "/", "_", &test_name);
-  // Add random number to the trace file to distinguish traces from different
-  // test runs.
-  // We don't use timestamp here to avoid collisions with parallel runs of the
-  // same test.
-  std::string random_seed = base::NumberToString(base::RandInt(1e7, 1e8 - 1));
-  std::string status = ::testing::UnitTest::GetInstance()
-                               ->current_test_info()
-                               ->result()
-                               ->Passed()
-                           ? "OK"
-                           : "FAIL";
-  return "trace_test_" + test_suite_name + "_" + test_name + "_" + random_seed +
-         "_" + status + ".json";
-}
-
-}  // namespace
-
 void BrowserTestBase::ProxyRunTestOnMainThreadLoop() {
 #if !defined(OS_ANDROID)
   // All FeatureList overrides should have been registered prior to browser test
@@ -728,17 +746,6 @@ void BrowserTestBase::ProxyRunTestOnMainThreadLoop() {
     signal(SIGTERM, DumpStackTraceSignalHandler);
 #endif  // defined(OS_POSIX)
 
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableTracing)) {
-    base::trace_event::TraceConfig trace_config(
-        base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-            switches::kEnableTracing),
-        base::trace_event::RECORD_CONTINUOUSLY);
-    TracingController::GetInstance()->StartTracing(
-        trace_config,
-        TracingController::StartTracingDoneCallback());
-  }
-
   {
     // This can be called from a posted task. Allow nested tasks here, because
     // otherwise the test body will have to do it in order to use RunLoop for
@@ -782,31 +789,23 @@ void BrowserTestBase::ProxyRunTestOnMainThreadLoop() {
     TearDownOnMainThread();
   }
 
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableTracing)) {
-    base::FilePath trace_file =
-        base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
-            switches::kEnableTracingOutput);
-    // If |trace_file| ends in a directory separator or is empty use a generated
-    // name in that directory (empty means current directory).
-    if (trace_file.empty() || trace_file.EndsWithSeparator())
-      trace_file = trace_file.AppendASCII(GetDefaultTraceFilename());
-
-    // Wait for tracing to collect results from the renderers.
-    base::RunLoop run_loop;
-    TracingController::GetInstance()->StopTracing(
-        TracingControllerImpl::CreateFileEndpoint(
-            trace_file, base::BindOnce(&TraceStopTracingComplete,
-                                       run_loop.QuitClosure(), trace_file)));
-    run_loop.Run();
-  }
-
   PostRunTestOnMainThread();
 
   // Sometimes tests initialize a storage partition and the initialization
   // schedules some tasks which need to be executed before finishing tests.
   // Run these tasks.
   content::RunAllPendingInMessageLoop();
+
+  // Update the trace output filename to include the test result.
+  StartupTracingController::GetInstance().SetDefaultBasename(
+      GetDefaultTraceBasename(TraceBasenameType::kWithTestStatus),
+      StartupTracingController::ExtensionType::kAppendAppropriate);
+
+#if defined(OS_ANDROID)
+  // On Android, browser main runner is not shut down, so stop trace recording
+  // here.
+  StartupTracingController::GetInstance().WaitUntilStopped();
+#endif
 }
 
 void BrowserTestBase::SetAllowNetworkAccessToHostResolutions() {