From fc87e409bea7c58c4142d4c19b40d8df8cc23e6e 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 | 8 +- spec/api-content-tracing-spec.js | 129 +++++++++++++++++++ 3 files changed, 156 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..e7c105175cb26 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. +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 diff --git a/spec/api-content-tracing-spec.js b/spec/api-content-tracing-spec.js new file mode 100644 index 0000000000000..50ad356e4bec7 --- /dev/null +++ b/spec/api-content-tracing-spec.js @@ -0,0 +1,129 @@ +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 / 1000 + 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 deliberately excluded, + // 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) + expect(fileSizeInKiloBytes).to.be.above(0, + `the trace output file is empty, check "${outputFilePath}"`) + expect(fileSizeInKiloBytes).to.be.below(5, + `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 deliberately excluded, + // 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) + expect(fileSizeInKiloBytes).to.be.above(0, + `the trace output file is empty, check "${outputFilePath}"`) + expect(fileSizeInKiloBytes).to.be.below(5, + `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 outputFilePath = getPathInATempFolder('trace.json') + const resultFilePath = await record({}, outputFilePath) + expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath) + }) + }) +})