fix: extending tracing startRecording API to take a full tracing config

This allows memory-infra to be traced correctly.
Fixes #12506.

atom/browser/api/atom_api_content_tracing.cc

@@ -7,6 +7,7 @@
 
 #include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/file_path_converter.h"
+#include "atom/common/native_mate_converters/value_converter.h"
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "content/public/browser/tracing_controller.h"
@@ -23,15 +24,27 @@ struct Converter<base::trace_event::TraceConfig> {
   static bool FromV8(v8::Isolate* isolate,
                      v8::Local<v8::Value> val,
                      base::trace_event::TraceConfig* out) {
+    // XXX(alexeykuzmin): A combination of "categoryFilter" and "traceOptions"
+    // has to be checked first because none of the fields
+    // in the `memory_dump_config` dict below are mandatory
+    // and we cannot check the config format.
     Dictionary options;
-    if (!ConvertFromV8(isolate, val, &options))
-      return false;
-    std::string category_filter, trace_options;
-    if (!options.Get("categoryFilter", &category_filter) ||
-        !options.Get("traceOptions", &trace_options))
-      return false;
-    *out = base::trace_event::TraceConfig(category_filter, trace_options);
-    return true;
+    if (ConvertFromV8(isolate, val, &options)) {
+      std::string category_filter, trace_options;
+      if (options.Get("categoryFilter", &category_filter) &&
+          options.Get("traceOptions", &trace_options)) {
+        *out = base::trace_event::TraceConfig(category_filter, trace_options);
+        return true;
+      }
+    }
+
+    base::DictionaryValue memory_dump_config;
+    if (ConvertFromV8(isolate, val, &memory_dump_config)) {
+      *out = base::trace_event::TraceConfig(memory_dump_config);
+      return true;
+    }
+
+    return false;
   }
 };
 

docs/api/content-tracing.md

@@ -52,8 +52,6 @@ Once all child processes have acknowledged the `getCategories` request the
 ### `contentTracing.startRecording(options, callback)`
 
 * `options` Object
-  * `categoryFilter` String
-  * `traceOptions` String
 * `callback` Function
 
 Start recording on all processes.
@@ -62,6 +60,12 @@ Recording begins immediately locally and asynchronously on child processes
 as soon as they receive the EnableRecording request. The `callback` will be
 called once all child processes have acknowledged the `startRecording` request.
 
+TODO: Describe two formats of the `options` object.
+https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way
+* `options` Object
+  * `categoryFilter` String
+  * `traceOptions` String
+
 `categoryFilter` is a filter to control what category groups should be
 traced. A filter can have an optional `-` prefix to exclude category groups
 that contain a matching category. Having both included and excluded

spec/api-content-tracing-spec.js

@@ -0,0 +1,132 @@
+const { remote } = require('electron')
+const chai = require('chai')
+const dirtyChai = require('dirty-chai')
+const fs = require('fs')
+const path = require('path')
+
+const { expect } = chai
+const { app, contentTracing } = remote
+
+chai.use(dirtyChai)
+
+const timeout = async (milliseconds) => {
+  return new Promise((resolve) => {
+    setTimeout(resolve, milliseconds)
+  })
+}
+
+const getPathInATempFolder = (filename) => {
+  return path.join(app.getPath('temp'), filename)
+}
+
+describe('contentTracing', () => {
+  const startRecording = async (options) => {
+    return new Promise((resolve) => {
+      contentTracing.startRecording(options, () => {
+        resolve()
+      })
+    })
+  }
+
+  const stopRecording = async (filePath) => {
+    return new Promise((resolve) => {
+      contentTracing.stopRecording(filePath, (resultFilePath) => {
+        resolve(resultFilePath)
+      })
+    })
+  }
+
+  const record = async (options, outputFilePath) => {
+    await app.whenReady()
+    const recordTimeInMilliseconds = 1e3
+
+    await startRecording(options)
+    await timeout(recordTimeInMilliseconds)
+    const resultFilePath = await stopRecording(outputFilePath)
+
+    return resultFilePath
+  }
+
+  const outputFilePath = getPathInATempFolder('trace.json')
+  beforeEach(() => {
+    if (fs.existsSync(outputFilePath)) {
+      fs.unlinkSync(outputFilePath)
+    }
+  })
+
+  describe('startRecording', function () {
+    this.timeout(5e3)
+
+    const getFileSizeInKiloBytes = (filePath) => {
+      const stats = fs.statSync(filePath)
+      const fileSizeInBytes = stats.size
+      const fileSizeInKiloBytes = fileSizeInBytes / 1024
+      return fileSizeInKiloBytes
+    }
+
+    it('accepts an empty config', async () => {
+      const config = {}
+      await record(config, outputFilePath)
+
+      expect(fs.existsSync(outputFilePath)).to.be.true()
+
+      const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath)
+      expect(fileSizeInKiloBytes).to.be.above(0,
+        `the trace output file is empty, check "${outputFilePath}"`)
+    })
+
+    it('accepts a trace config', async () => {
+      // XXX(alexeykuzmin): All categories are excluded on purpose,
+      // so only metadata gets into the output file.
+      const config = {
+        excluded_categories: ['*']
+      }
+      await record(config, outputFilePath)
+
+      expect(fs.existsSync(outputFilePath)).to.be.true()
+
+      // If the `excluded_categories` param above is not respected
+      // the file size will be above 50KB.
+      const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath)
+      const expectedMaximumFileSize = 10 // Depends on a platform.
+
+      expect(fileSizeInKiloBytes).to.be.above(0,
+        `the trace output file is empty, check "${outputFilePath}"`)
+      expect(fileSizeInKiloBytes).to.be.below(expectedMaximumFileSize,
+        `the trace output file is suspiciously large (${fileSizeInKiloBytes}KB),
+        check "${outputFilePath}"`)
+    })
+
+    it('accepts "categoryFilter" and "traceOptions" as a config', async () => {
+      // XXX(alexeykuzmin): All categories are excluded on purpose,
+      // so only metadata gets into the output file.
+      const config = {
+        categoryFilter: '__ThisIsANonexistentCategory__',
+        traceOptions: ''
+      }
+      await record(config, outputFilePath)
+
+      expect(fs.existsSync(outputFilePath)).to.be.true()
+
+      // If the `categoryFilter` param above is not respected
+      // the file size will be above 50KB.
+      const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath)
+      const expectedMaximumFileSize = 10 // Depends on a platform.
+
+      expect(fileSizeInKiloBytes).to.be.above(0,
+        `the trace output file is empty, check "${outputFilePath}"`)
+      expect(fileSizeInKiloBytes).to.be.below(expectedMaximumFileSize,
+        `the trace output file is suspiciously large (${fileSizeInKiloBytes}KB),
+        check "${outputFilePath}"`)
+    })
+  })
+
+  describe('stopRecording', function () {
+    this.timeout(5e3)
+
+    it('calls its callback with a result file path', async () => {
+      const resultFilePath = await record({}, outputFilePath)
+      expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath)
+    })
+  })
+})