fix: extend tracing startRecording API to take a full tracing config (#13914)

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,16 +24,28 @@ struct Converter<base::trace_event::TraceConfig> {
   static bool FromV8(v8::Isolate* isolate,
                      v8::Local<v8::Value> val,
                      base::trace_event::TraceConfig* out) {
+    // (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))
+    if (ConvertFromV8(isolate, val, &options)) {
-      return false;
       std::string category_filter, trace_options;
-    if (!options.Get("categoryFilter", &category_filter) ||
+      if (options.Get("categoryFilter", &category_filter) &&
-        !options.Get("traceOptions", &trace_options))
+          options.Get("traceOptions", &trace_options)) {
-      return false;
         *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;
+  }
 };
 
 }  // namespace mate

docs/api/content-tracing.md

@@ -51,9 +51,7 @@ Once all child processes have acknowledged the `getCategories` request the
 
 ### `contentTracing.startRecording(options, callback)`
 
-* `options` Object
+* `options` ([TraceCategoriesAndOptions](structures/trace-categories-and-options.md) | [TraceConfig](structures/trace-config.md))
-  * `categoryFilter` String
-  * `traceOptions` String
 * `callback` Function
 
 Start recording on all processes.
@@ -62,35 +60,6 @@ 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.
 
-`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
-category patterns in the same list is not supported.
-
-Examples:
-
-* `test_MyTest*`,
-* `test_MyTest*,test_OtherStuff`,
-* `"-excluded_category1,-excluded_category2`
-
-`traceOptions` controls what kind of tracing is enabled, it is a comma-delimited
-list. Possible options are:
-
-* `record-until-full`
-* `record-continuously`
-* `trace-to-console`
-* `enable-sampling`
-* `enable-systrace`
-
-The first 3 options are trace recording modes and hence mutually exclusive.
-If more than one trace recording modes appear in the `traceOptions` string,
-the last one takes precedence. If none of the trace recording modes are
-specified, recording mode is `record-until-full`.
-
-The trace option will first be reset to the default option (`record_mode` set to
-`record-until-full`, `enable_sampling` and `enable_systrace` set to `false`)
-before options parsed from `traceOptions` are applied on it.
-
 ### `contentTracing.stopRecording(resultFilePath, callback)`
 
 * `resultFilePath` String

docs/api/structures/trace-categories-and-options.md

@@ -0,0 +1,18 @@
+# TraceCategoriesAndOptions Object
+
+* `categoryFilter` String – 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 category patterns in the same list is not supported. Examples:
+  `test_MyTest*`, `test_MyTest*,test_OtherStuff`, `-excluded_category1,-excluded_category2`.
+* `traceOptions` String - Controls what kind of tracing is enabled,
+  it is a comma-delimited sequence of the following strings:
+  `record-until-full`, `record-continuously`, `trace-to-console`, `enable-sampling`, `enable-systrace`,
+  e.g. `'record-until-full,enable-sampling'`.
+  The first 3 options are trace recording modes and hence mutually exclusive.
+  If more than one trace recording modes appear in the `traceOptions` string,
+  the last one takes precedence. If none of the trace recording modes are
+  specified, recording mode is `record-until-full`.
+  The trace option will first be reset to the default option (`record_mode` set
+  to `record-until-full`, `enable_sampling` and `enable_systrace`
+  set to `false`) before options parsed from `traceOptions` are applied on it.

docs/api/structures/trace-config.md

@@ -0,0 +1,9 @@
+# TraceConfig Object
+
+* `included_categories` String[] (optional)
+* `excluded_categories` String[] (optional)
+* `memory_dump_config` Object (optional)
+
+See an example in the [Chromium docs][1].
+
+[1]: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way

spec/api-content-tracing-spec.js

@@ -0,0 +1,145 @@
+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', () => {
+  beforeEach(function () {
+    // FIXME: The tests are skipped on arm/arm64.
+    if (process.platform === 'linux' &&
+        ['arm', 'arm64'].includes(process.arch)) {
+      this.skip()
+    }
+  })
+
+  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, recordTimeInMilliseconds = 1e3) => {
+    await app.whenReady()
+
+    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 () => {
+      // (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 () => {
+      // (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(/* options */ {}, outputFilePath)
+      expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath)
+    })
+
+    // FIXME(alexeykuzmin): https://github.com/electron/electron/issues/16019
+    xit('creates a temporary file when an empty string is passed', async function () {
+      const resultFilePath = await record(/* options */ {}, /* outputFilePath */ '')
+      expect(resultFilePath).to.be.a('string').that.is.not.empty()
+    })
+  })
+})