Add an allowlist flag for Skia trace event categories (#25985)

Author: jason-simmons

common/settings.h

@@ -104,7 +104,8 @@ struct Settings {
   bool enable_checked_mode = false;
   bool start_paused = false;
   bool trace_skia = false;
-  std::string trace_allowlist;
+  std::vector<std::string> trace_allowlist;
+  std::vector<std::string> trace_skia_allowlist;
   bool trace_startup = false;
   bool trace_systrace = false;
   bool dump_skp_on_shader_compilation = false;

shell/common/shell.cc

@@ -68,16 +68,6 @@ std::unique_ptr<Engine> CreateEngine(
                                   volatile_path_tracker);
 }
 
-void Tokenize(const std::string& input,
-              std::vector<std::string>* results,
-              char delimiter) {
-  std::istringstream ss(input);
-  std::string token;
-  while (std::getline(ss, token, delimiter)) {
-    results->push_back(token);
-  }
-}
-
 // Though there can be multiple shells, some settings apply to all components in
 // the process. These have to be set up before the shell or any of its
 // sub-components can be initialized. In a perfect world, this would be empty.
@@ -103,13 +93,11 @@ void PerformInitializationTasks(Settings& settings) {
         [](const char* message) { FML_LOG(ERROR) << message; });
 
     if (settings.trace_skia) {
-      InitSkiaEventTracer(settings.trace_skia);
+      InitSkiaEventTracer(settings.trace_skia, settings.trace_skia_allowlist);
     }
 
     if (!settings.trace_allowlist.empty()) {
-      std::vector<std::string> prefixes;
-      Tokenize(settings.trace_allowlist, &prefixes, ',');
-      fml::tracing::TraceSetAllowlist(prefixes);
+      fml::tracing::TraceSetAllowlist(settings.trace_allowlist);
     }
 
     if (!settings.skia_deterministic_rendering_on_cpu) {

shell/common/skia_event_tracer_impl.cc

@@ -5,6 +5,8 @@
 #include "flutter/shell/common/skia_event_tracer_impl.h"
 
 #define TRACE_EVENT_HIDE_MACROS
+#include <map>
+#include <set>
 #include <vector>
 
 #include "flutter/fml/logging.h"
@@ -42,6 +44,10 @@ namespace flutter {
 
 namespace {
 
+// Skia prepends this string to the category names of its trace events.
+// Defined in Skia's src/core/SkTraceEvent.h.
+constexpr std::string_view kTraceCategoryPrefix = "disabled-by-default-";
+
 #if defined(OS_FUCHSIA)
 template <class T, class U>
 inline T BitCast(const U& u) {
@@ -61,7 +67,12 @@ class FlutterEventTracer : public SkEventTracer {
   static constexpr uint8_t kYes = 1;
   static constexpr uint8_t kNo = 0;
 
-  FlutterEventTracer(bool enabled) : enabled_(enabled ? kYes : kNo){};
+  FlutterEventTracer(bool enabled, const std::vector<std::string>& allowlist)
+      : enabled_(enabled ? kYes : kNo) {
+    for (const std::string& category : allowlist) {
+      allowlist_.insert(std::string(kTraceCategoryPrefix) + category);
+    }
+  };
 
   SkEventTracer::Handle addTraceEvent(char phase,
                                       const uint8_t* category_enabled_flag,
@@ -221,39 +232,43 @@ class FlutterEventTracer : public SkEventTracer {
   }
 
   const uint8_t* getCategoryGroupEnabled(const char* name) override {
-    return &enabled_;
+    // Skia will only use long-lived string literals as event names.
+    auto flag_it = category_flag_map_.find(name);
+    if (flag_it == category_flag_map_.end()) {
+      bool allowed;
+      if (enabled_) {
+        allowed =
+            allowlist_.empty() || allowlist_.find(name) != allowlist_.end();
+      } else {
+        allowed = false;
+      }
+      flag_it = category_flag_map_.insert(std::make_pair(name, allowed)).first;
+      reverse_flag_map_.insert(std::make_pair(&flag_it->second, name));
+    }
+    return &flag_it->second;
   }
 
   const char* getCategoryGroupName(
       const uint8_t* category_enabled_flag) override {
-    return kSkiaTag;
+    auto reverse_it = reverse_flag_map_.find(category_enabled_flag);
+    if (reverse_it != reverse_flag_map_.end()) {
+      return reverse_it->second;
+    } else {
+      return kSkiaTag;
+    }
   }
 
-  void enable() { enabled_ = kYes; }
-
  private:
   uint8_t enabled_;
+  std::set<std::string> allowlist_;
+  std::map<const char*, uint8_t> category_flag_map_;
+  std::map<const uint8_t*, const char*> reverse_flag_map_;
   FML_DISALLOW_COPY_AND_ASSIGN(FlutterEventTracer);
 };
 
-bool enableSkiaTracingCallback(const char* method,
-                               const char** param_keys,
-                               const char** param_values,
-                               intptr_t num_params,
-                               void* user_data,
-                               const char** json_object) {
-  FlutterEventTracer* tracer = static_cast<FlutterEventTracer*>(user_data);
-  tracer->enable();
-  *json_object = fml::strdup("{\"type\":\"Success\"}");
-  return true;
-}
-
-void InitSkiaEventTracer(bool enabled) {
-  // TODO(chinmaygarde): Leaked https://github.com/flutter/flutter/issues/30808.
-  auto tracer = new FlutterEventTracer(enabled);
-  Dart_RegisterRootServiceRequestCallback("_flutter.enableSkiaTracing",
-                                          enableSkiaTracingCallback,
-                                          static_cast<void*>(tracer));
+void InitSkiaEventTracer(bool enabled,
+                         const std::vector<std::string>& allowlist) {
+  auto tracer = new FlutterEventTracer(enabled, allowlist);
   // Initialize the binding to Skia's tracing events. Skia will
   // take ownership of and clean up the memory allocated here.
   SkEventTracer::SetInstance(tracer);

shell/common/skia_event_tracer_impl.h

@@ -5,9 +5,13 @@
 #ifndef FLUTTER_SHELL_COMMON_SKIA_EVENT_TRACER_IMPL_H_
 #define FLUTTER_SHELL_COMMON_SKIA_EVENT_TRACER_IMPL_H_
 
+#include <string>
+#include <vector>
+
 namespace flutter {
 
-void InitSkiaEventTracer(bool enabled);
+void InitSkiaEventTracer(bool enabled,
+                         const std::vector<std::string>& allowlist);
 
 }  // namespace flutter
 

shell/common/switches.cc

@@ -155,6 +155,16 @@ const std::string_view FlagForSwitch(Switch swtch) {
   return std::string_view();
 }
 
+static std::vector<std::string> ParseCommaDelimited(const std::string& input) {
+  std::istringstream ss(input);
+  std::vector<std::string> result;
+  std::string token;
+  while (std::getline(ss, token, ',')) {
+    result.push_back(token);
+  }
+  return result;
+}
+
 static bool IsAllowedDartVMFlag(const std::string& flag) {
   for (uint32_t i = 0; i < fml::size(gAllowedDartFlags); ++i) {
     const std::string& allowed = gAllowedDartFlags[i];
@@ -289,17 +299,15 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) {
   settings.trace_skia =
       command_line.HasOption(FlagForSwitch(Switch::TraceSkia));
 
-  command_line.GetOptionValue(FlagForSwitch(Switch::TraceAllowlist),
-                              &settings.trace_allowlist);
+  std::string trace_skia_allowlist;
+  command_line.GetOptionValue(FlagForSwitch(Switch::TraceSkiaAllowlist),
+                              &trace_skia_allowlist);
+  settings.trace_skia_allowlist = ParseCommaDelimited(trace_skia_allowlist);
 
-  if (settings.trace_allowlist.empty()) {
-    command_line.GetOptionValue(FlagForSwitch(Switch::TraceWhitelist),
-                                &settings.trace_allowlist);
-    if (!settings.trace_allowlist.empty()) {
-      FML_LOG(INFO)
-          << "--trace-whitelist is deprecated. Use --trace-allowlist instead.";
-    }
-  }
+  std::string trace_allowlist;
+  command_line.GetOptionValue(FlagForSwitch(Switch::TraceAllowlist),
+                              &trace_allowlist);
+  settings.trace_allowlist = ParseCommaDelimited(trace_allowlist);
 
   settings.trace_systrace =
       command_line.HasOption(FlagForSwitch(Switch::TraceSystrace));
@@ -384,11 +392,9 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) {
   std::string all_dart_flags;
   if (command_line.GetOptionValue(FlagForSwitch(Switch::DartFlags),
                                   &all_dart_flags)) {
-    std::stringstream stream(all_dart_flags);
-    std::string flag;
-
     // Assume that individual flags are comma separated.
-    while (std::getline(stream, flag, ',')) {
+    std::vector<std::string> flags = ParseCommaDelimited(all_dart_flags);
+    for (auto flag : flags) {
       if (!IsAllowedDartVMFlag(flag)) {
         FML_LOG(FATAL) << "Encountered disallowed Dart VM flag: " << flag;
       }

shell/common/switches.h

@@ -132,9 +132,10 @@ DEF_SWITCH(TraceSkia,
            "Trace Skia calls. This is useful when debugging the GPU threed."
            "By default, Skia tracing is not enabled to reduce the number of "
            "traced events")
-DEF_SWITCH(TraceWhitelist,
-           "trace-whitelist",
-           "(deprecated) Use --trace-allowlist instead.")
+DEF_SWITCH(TraceSkiaAllowlist,
+           "trace-skia-allowlist",
+           "Filters out all Skia trace event categories except those that are "
+           "specified in this comma separated list.")
 DEF_SWITCH(
     TraceAllowlist,
     "trace-allowlist",