From 58d03dcf59dafdbcf824da398f9e7cdff7253b78 Mon Sep 17 00:00:00 2001 From: Zac Walker Date: Thu, 2 Aug 2018 16:51:31 +0200 Subject: [PATCH] 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 | 29 ++-- docs/api/content-tracing.md | 11 +- spec/api-content-tracing-spec.js | 140 +++++++++++++++++++ 3 files changed, 170 insertions(+), 10 deletions(-) create mode 100644 spec/api-content-tracing-spec.js diff --git a/atom/browser/api/atom_api_content_tracing.cc b/atom/browser/api/atom_api_content_tracing.cc index d340b167a5f35..c5ad2e126ab2d 100644 --- a/atom/browser/api/atom_api_content_tracing.cc +++ b/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 { static bool FromV8(v8::Isolate* isolate, v8::Local 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; } }; diff --git a/docs/api/content-tracing.md b/docs/api/content-tracing.md index 7fe9f1dd2ff1c..8b9e960d6b92d 100644 --- a/docs/api/content-tracing.md +++ b/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. +The `options` object can be in two formats. +1. A combination of "categoryFilter" and "traceOptions". +* `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 @@ -91,6 +95,9 @@ 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. +2. A trace config file format. +See an example [here](https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way). + ### `contentTracing.stopRecording(resultFilePath, callback)` * `resultFilePath` String diff --git a/spec/api-content-tracing-spec.js b/spec/api-content-tracing-spec.js new file mode 100644 index 0000000000000..ebb3b0c738a4f --- /dev/null +++ b/spec/api-content-tracing-spec.js @@ -0,0 +1,140 @@ +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) => { + 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) + }) + }) +})