feat: [contentTracing] allow calling stopTracing() with no arguments (#18411)

atom/browser/api/atom_api_content_tracing.cc

@@ -4,6 +4,7 @@
 
 #include <set>
 #include <string>
+#include <utility>
 
 #include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/file_path_converter.h"
@@ -12,6 +13,7 @@
 #include "atom/common/promise_util.h"
 #include "base/bind.h"
 #include "base/files/file_util.h"
+#include "base/optional.h"
 #include "base/threading/thread_restrictions.h"
 #include "content/public/browser/tracing_controller.h"
 #include "native_mate/dictionary.h"
@@ -53,36 +55,44 @@ struct Converter<base::trace_event::TraceConfig> {
 
 namespace {
 
-using CompletionCallback = base::RepeatingCallback<void(const base::FilePath&)>;
+using CompletionCallback = base::OnceCallback<void(const base::FilePath&)>;
 
-scoped_refptr<TracingController::TraceDataEndpoint> GetTraceDataEndpoint(
+base::Optional<base::FilePath> CreateTemporaryFileOnIO() {
-    const base::FilePath& path,
+  base::FilePath temp_file_path;
-    const CompletionCallback& callback) {
+  if (!base::CreateTemporaryFile(&temp_file_path))
-  base::FilePath result_file_path = path;
+    return base::nullopt;
-
+  return base::make_optional(std::move(temp_file_path));
-  // base::CreateTemporaryFile prevents blocking so we need to allow it
-  // for now since offloading this to a different sequence would require
-  // changing the api shape
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-  if (result_file_path.empty() && !base::CreateTemporaryFile(&result_file_path))
-    LOG(ERROR) << "Creating temporary file failed";
-
-  return TracingController::CreateFileEndpoint(
-      result_file_path, base::BindRepeating(callback, result_file_path));
 }
 
-v8::Local<v8::Promise> StopRecording(v8::Isolate* isolate,
+void StopTracing(atom::util::Promise promise,
-                                     const base::FilePath& path) {
+                 base::Optional<base::FilePath> file_path) {
-  atom::util::Promise promise(isolate);
+  if (file_path) {
+    auto endpoint = TracingController::CreateFileEndpoint(
+        *file_path, base::AdaptCallbackForRepeating(base::BindOnce(
+                        &atom::util::Promise::ResolvePromise<base::FilePath>,
+                        std::move(promise), *file_path)));
+    TracingController::GetInstance()->StopTracing(endpoint);
+  } else {
+    promise.RejectWithErrorMessage(
+        "Failed to create temporary file for trace data");
+  }
+}
+
+v8::Local<v8::Promise> StopRecording(mate::Arguments* args) {
+  atom::util::Promise promise(args->isolate());
   v8::Local<v8::Promise> handle = promise.GetHandle();
 
-  // TODO(zcbenz): Remove the use of CopyablePromise when the
+  base::FilePath path;
-  // CreateFileEndpoint API accepts OnceCallback.
+  if (args->GetNext(&path) && !path.empty()) {
-  TracingController::GetInstance()->StopTracing(GetTraceDataEndpoint(
+    StopTracing(std::move(promise), base::make_optional(path));
-      path,
+  } else {
-      base::BindRepeating(atom::util::CopyablePromise::ResolveCopyablePromise<
+    // use a temporary file.
-                              const base::FilePath&>,
+    base::PostTaskWithTraitsAndReplyWithResult(
-                          atom::util::CopyablePromise(promise))));
+        FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
+        base::BindOnce(CreateTemporaryFileOnIO),
+        base::BindOnce(StopTracing, std::move(promise)));
+  }
+
   return handle;
 }
 
@@ -104,10 +114,15 @@ v8::Local<v8::Promise> StartTracing(
   atom::util::Promise promise(isolate);
   v8::Local<v8::Promise> handle = promise.GetHandle();
 
-  // Note: This method always succeeds.
+  if (!TracingController::GetInstance()->StartTracing(
-  TracingController::GetInstance()->StartTracing(
+          trace_config, base::BindOnce(atom::util::Promise::ResolveEmptyPromise,
-      trace_config, base::BindOnce(atom::util::Promise::ResolveEmptyPromise,
+                                       std::move(promise)))) {
-                                   std::move(promise)));
+    // If StartTracing returns false, that means it didn't invoke its callback.
+    // Return an already-resolved promise and abandon the previous promise (it
+    // was std::move()d into the StartTracing callback and has been deleted by
+    // this point).
+    return atom::util::Promise::ResolvedPromise(isolate);
+  }
   return handle;
 }
 

atom/common/promise_util.h

@@ -82,6 +82,21 @@ class Promise {
     }
   }
 
+  // Returns an already-resolved promise.
+  template <typename T>
+  static v8::Local<v8::Promise> ResolvedPromise(v8::Isolate* isolate,
+                                                T result) {
+    Promise resolved(isolate);
+    resolved.Resolve(result);
+    return resolved.GetHandle();
+  }
+
+  static v8::Local<v8::Promise> ResolvedPromise(v8::Isolate* isolate) {
+    Promise resolved(isolate);
+    resolved.Resolve();
+    return resolved.GetHandle();
+  }
+
   v8::Local<v8::Promise> GetHandle() const;
 
   v8::Maybe<bool> Resolve() {

docs/api/content-tracing.md

@@ -1,13 +1,11 @@
 # contentTracing
 
-> Collect tracing data from Chromium's content module for finding performance
+> Collect tracing data from Chromium to find performance bottlenecks and slow operations.
-bottlenecks and slow operations.
 
 Process: [Main](../glossary.md#main-process)
 
-This module does not include a web interface so you need to open
+This module does not include a web interface. To view recorded traces, use
-`chrome://tracing/` in a Chrome browser and load the generated file to view the
+[trace viewer][], available at `chrome://tracing` in Chrome.
-result.
 
 **Note:** You should not use this module until the `ready` event of the app
 module is emitted.
@@ -16,20 +14,15 @@ module is emitted.
 const { app, contentTracing } = require('electron')
 
 app.on('ready', () => {
-  const options = {
+  (async () => {
-    categoryFilter: '*',
+    await contentTracing.startRecording({
-    traceOptions: 'record-until-full,enable-sampling'
+      include_categories: ['*']
-  }
+    })
-
-  contentTracing.startRecording(options, () => {
     console.log('Tracing started')
-
+    await new Promise(resolve => setTimeout(resolve, 5000))
-    setTimeout(() => {
+    const path = await contentTracing.stopRecording()
-      contentTracing.stopRecording('', (path) => {
+    console.log('Tracing data recorded to ' + path)
-        console.log('Tracing data recorded to ' + path)
+  })()
-      })
-    }, 5000)
-  })
 })
 ```
 
@@ -41,11 +34,13 @@ The `contentTracing` module has the following methods:
 
 Returns `Promise<String[]>` - resolves with an array of category groups once all child processes have acknowledged the `getCategories` request
 
-Get a set of category groups. The category groups can change as new code paths are reached.
+Get a set of category groups. The category groups can change as new code paths
+are reached. See also the [list of built-in tracing
+categories](https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/builtin_categories.h).
 
 ### `contentTracing.startRecording(options)`
 
-* `options` ([TraceCategoriesAndOptions](structures/trace-categories-and-options.md) | [TraceConfig](structures/trace-config.md))
+* `options` ([TraceConfig](structures/trace-config.md) | [TraceCategoriesAndOptions](structures/trace-categories-and-options.md))
 
 Returns `Promise<void>` - resolved once all child processes have acknowledged the `startRecording` request.
 
@@ -54,22 +49,26 @@ Start recording on all processes.
 Recording begins immediately locally and asynchronously on child processes
 as soon as they receive the EnableRecording request.
 
+If a recording is already running, the promise will be immediately resolved, as
+only one trace operation can be in progress at a time.
+
 ### `contentTracing.stopRecording(resultFilePath)`
 
-* `resultFilePath` String
+* `resultFilePath` String (optional)
 
-Returns `Promise<String>` - resolves with a file that contains the traced data once all child processes have acknowledged the `stopRecording` request
+Returns `Promise<String>` - resolves with a path to a file that contains the traced data once all child processes have acknowledged the `stopRecording` request
 
 Stop recording on all processes.
 
 Child processes typically cache trace data and only rarely flush and send
 trace data back to the main process. This helps to minimize the runtime overhead
 of tracing since sending trace data over IPC can be an expensive operation. So,
-to end tracing, we must asynchronously ask all child processes to flush any
+to end tracing, Chromium asynchronously asks all child processes to flush any
 pending trace data.
 
-Trace data will be written into `resultFilePath` if it is not empty or into a
+Trace data will be written into `resultFilePath`. If `resultFilePath` is empty
-temporary file.
+or not provided, trace data will be written to a temporary file, and the path
+will be returned in the promise.
 
 ### `contentTracing.getTraceBufferUsage()`
 
@@ -77,3 +76,5 @@ Returns `Promise<Object>` - Resolves with an object containing the `value` and `
 
 Get the maximum usage across processes of trace buffer as a percentage of the
 full state.
+
+[trace viewer]: https://github.com/catapult-project/catapult/blob/master/tracing

docs/api/structures/trace-config.md

@@ -1,9 +1,51 @@
 # TraceConfig Object
 
-* `included_categories` String[] (optional)
+* `recording_mode` String (optional) - one of "record-until-full" | "record-continuously" | "record-as-much-as-possible" | "trace-to-console". Defaults to "record-until-full".
-* `excluded_categories` String[] (optional)
+* `trace_buffer_size_in_kb` number (optional) - maximum size of the trace
-* `memory_dump_config` Object (optional)
+  recording buffer in kilobytes. Defaults to 100MB.
+* `trace_buffer_size_in_events` number (optional) - maximum size of the trace
+  recording buffer in events.
+* `enable_argument_filter` boolean (optional) - if true, filter event data
+  according to a whitelist of events that have been manually vetted to not
+  include any PII. See [the implementation in
+  Chromium][trace_event_args_whitelist.cc] for specifics.
+* `included_categories` String[] (optional) - a list of tracing categories to
+  include. Can include glob-like patterns using `*` at the end of the category
+  name. See [tracing categories][] for the list of categories.
+* `excluded_categories` String[] (optional) - a list of tracing categories to
+  exclude. Can include glob-like patterns using `*` at the end of the category
+  name. See [tracing categories][] for the list of categories.
+* `included_process_ids` number[] (optional) - a list of process IDs to
+  include in the trace. If not specified, trace all processes.
+* `histogram_names` String[] (optional) - a list of [histogram][] names to report
+  with the trace.
+* `memory_dump_config` Object (optional) - if the
+  `disabled-by-default-memory-infra` category is enabled, this contains
+  optional additional configuration for data callection. See the [Chromium
+  memory-infra docs][memory-infra docs] for more information.
 
-See an example in the [Chromium docs][1].
+An example TraceConfig that roughly matches what Chrome DevTools records:
 
-[1]: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way
+```js
+{
+  recording_mode: 'record-until-full',
+  included_categories: [
+    'devtools.timeline',
+    'disabled-by-default-devtools.timeline',
+    'disabled-by-default-devtools.timeline.frame',
+    'disabled-by-default-devtools.timeline.stack',
+    'v8.execute',
+    'blink.console',
+    'blink.user_timing',
+    'latencyInfo',
+    'disabled-by-default-v8.cpu_profiler',
+    'disabled-by-default-v8.cpu_profiler.hires'
+  ],
+  excluded_categories: [ '*' ]
+}
+```
+
+[tracing categories]: https://chromium.googlesource.com/chromium/src/+/master/base/trace_event/builtin_categories.h
+[memory-infra docs]: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_infra_startup_tracing.md#the-advanced-way
+[trace_event_args_whitelist.cc]: https://chromium.googlesource.com/chromium/src/+/master/services/tracing/public/cpp/trace_event_args_whitelist.cc
+[histogram]: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md

lib/browser/api/content-tracing.js

@@ -1,5 +1,2 @@
 'use strict'
-const { deprecate } = require('electron')
-const contentTracing =
-
 module.exports = process.electronBinding('content_tracing')

spec/api-content-tracing-spec.js

@@ -132,5 +132,10 @@ describe('contentTracing', () => {
       const resultFilePath = await record(/* options */ {}, /* outputFilePath */ '')
       expect(resultFilePath).to.be.a('string').that.is.not.empty()
     })
+
+    it('creates a temporary file when no path is passed', async function () {
+      const resultFilePath = await record(/* options */ {}, /* outputFilePath */ undefined)
+      expect(resultFilePath).to.be.a('string').that.is.not.empty()
+    })
   })
 })