trace_events: fix trace events JS API writing

The Trace Events JS API isn't functional if none of
--trace-events-enabled or --trace-event-categories is passed as a CLI
argument. This commit fixes that.

In addition, we currently don't test the trace_events JS API in the
casewhere no CLI args are provided. This commit adds that test.

Fixes https://github.com/nodejs/node/issues/24944

PR-URL: https://github.com/nodejs/node/pull/24945
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

src/node_trace_events.cc

@@ -74,6 +74,9 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (!category_set->enabled_ && !categories.empty()) {
+    // Starts the Tracing Agent if it wasn't started already (e.g. through
+    // a command line flag.)
+    StartTracingAgent();
     GetTracingAgentWriter()->Enable(categories);
     category_set->enabled_ = true;
   }

src/node_v8_platform-inl.h

@@ -85,7 +85,11 @@ struct V8Platform {
         tracing_agent_->GetTracingController();
     trace_state_observer_.reset(new NodeTraceStateObserver(controller));
     controller->AddTraceStateObserver(trace_state_observer_.get());
-    StartTracingAgent();
+    tracing_file_writer_ = tracing_agent_->DefaultHandle();
+    // Only start the tracing agent if we enabled any tracing categories.
+    if (!per_process::cli_options->trace_event_categories.empty()) {
+      StartTracingAgent();
+    }
     // Tracing must be initialized before platform threads are created.
     platform_ = new NodePlatform(thread_pool_size, controller);
     v8::V8::InitializePlatform(platform_);
@@ -111,9 +115,9 @@ struct V8Platform {
   }
 
   inline void StartTracingAgent() {
-    if (per_process::cli_options->trace_event_categories.empty()) {
+    // Attach a new NodeTraceWriter only if this function hasn't been called
-      tracing_file_writer_ = tracing_agent_->DefaultHandle();
+    // before.
-    } else {
+    if (tracing_file_writer_.IsDefaultHandle()) {
       std::vector<std::string> categories =
           SplitString(per_process::cli_options->trace_event_categories, ',');
 
@@ -163,6 +167,10 @@ namespace per_process {
 extern struct V8Platform v8_platform;
 }
 
+inline void StartTracingAgent() {
+  return per_process::v8_platform.StartTracingAgent();
+}
+
 inline tracing::AgentWriterHandle* GetTracingAgentWriter() {
   return per_process::v8_platform.GetTracingAgentWriter();
 }

src/tracing/agent.h

@@ -59,6 +59,8 @@ class AgentWriterHandle {
   inline void Enable(const std::set<std::string>& categories);
   inline void Disable(const std::set<std::string>& categories);
 
+  inline bool IsDefaultHandle();
+
   inline Agent* agent() { return agent_; }
 
   inline v8::TracingController* GetTracingController();
@@ -175,6 +177,10 @@ void AgentWriterHandle::Disable(const std::set<std::string>& categories) {
   if (agent_ != nullptr) agent_->Disable(id_, categories);
 }
 
+bool AgentWriterHandle::IsDefaultHandle() {
+  return agent_ != nullptr && id_ == Agent::kDefaultHandleId;
+}
+
 inline v8::TracingController* AgentWriterHandle::GetTracingController() {
   return agent_ != nullptr ? agent_->GetTracingController() : nullptr;
 }

test/parallel/test-trace-events-api.js

@@ -17,8 +17,17 @@ const {
   getEnabledCategories
 } = require('trace_events');
 
+function getEnabledCategoriesFromCommandLine() {
+  const indexOfCatFlag = process.execArgv.indexOf('--trace-event-categories');
+  if (indexOfCatFlag === -1) {
+    return undefined;
+  } else {
+    return process.execArgv[indexOfCatFlag + 1];
+  }
+}
+
 const isChild = process.argv[2] === 'child';
-const enabledCategories = isChild ? 'foo' : undefined;
+const enabledCategories = getEnabledCategoriesFromCommandLine();
 
 assert.strictEqual(getEnabledCategories(), enabledCategories);
 [1, 'foo', true, false, null, undefined].forEach((i) => {
@@ -51,7 +60,9 @@ tracing.enable();  // purposefully enable twice to test calling twice
 assert.strictEqual(tracing.enabled, true);
 
 assert.strictEqual(getEnabledCategories(),
-                   isChild ? 'foo,node.perf' : 'node.perf');
+                   [
+                     ...[enabledCategories].filter((_) => !!_), 'node.perf'
+                   ].join(','));
 
 tracing.disable();
 assert.strictEqual(tracing.enabled, false);
@@ -106,7 +117,15 @@ if (isChild) {
     }
   }
 
+  testApiInChildProcess([], () => {
+    testApiInChildProcess(['--trace-event-categories', 'foo']);
+  });
+}
+
+function testApiInChildProcess(execArgs, cb) {
   tmpdir.refresh();
+  // Save the current directory so we can chdir back to it later
+  const parentDir = process.cwd();
   process.chdir(tmpdir.path);
 
   const expectedMarks = ['A', 'B'];
@@ -121,15 +140,14 @@ if (isChild) {
 
   const proc = cp.fork(__filename,
                        ['child'],
-                       { execArgv: [ '--expose-gc',
+                       { execArgv: [ '--expose-gc', ...execArgs ] });
-                                     '--trace-event-categories',
-                                     'foo' ] });
 
   proc.once('exit', common.mustCall(() => {
     const file = path.join(tmpdir.path, 'node_trace.1.log');
 
     assert(fs.existsSync(file));
     fs.readFile(file, common.mustCall((err, data) => {
+      assert.ifError(err);
       const traces = JSON.parse(data.toString()).traceEvents
         .filter((trace) => trace.cat !== '__metadata');
       assert.strictEqual(traces.length,
@@ -160,6 +178,8 @@ if (isChild) {
             assert.fail('Unexpected trace event phase');
         }
       });
+      process.chdir(parentDir);
+      cb && process.nextTick(cb);
     }));
   }));
 }