feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot() #14456

miniak · 2018-09-04T22:20:28Z

Description of Change

There is a node module for getting V8 heap snapshots https://github.com/bnoordhuis/node-heapdump, but it cannot be used in sandboxed renderers due to node not being available. Currently the only way to do this programmatically is to expose this API in Electron.

This PR is adding these new methods:

process.takeHeapSnapshot() - allows the main process and non-sandboxed renderers to take its own V8 heap snapshot
webContents.takeHeapSnapshot() - allows the main process to ask any renderer (including sandboxed ones) to create its V8 heap snapshot

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines

Release Notes

Notes: Added process.getHeapSnapshot() / webContents.takeHeapSnapshot() for taking V8 heap snapshots.

alexeykuzmin · 2018-09-05T18:46:09Z

there is a bug causing boolean return types for processobject methods to be incorrectly generated as Electron.Boolean, which causes failures when validating electron.d.ts.

@MarshallOfSound FYI

MarshallOfSound · 2018-09-06T02:26:11Z

@miniak The bug to which you refer was fixed in electron/typescript-definitions#107 if you are up to date with latest master and have the latest version of electron-typescript-definitions installed primitives should work fine in process modifications 🤔

alexeykuzmin · 2018-09-14T21:25:06Z

@miniak

Please make sure that "electron-lint" passes. You might need to rebase this PR onto the latest master to fix that check.
If you're done with the implementation, please remove the [wip] suffix.

alexeykuzmin

👍

alexeykuzmin · 2018-09-14T21:28:40Z

docs/api/process.md

@@ -171,6 +171,10 @@ Returns `Object`:
 Returns an object giving memory usage statistics about the entire system. Note
 that all statistics are reported in Kilobytes.

+### `process.takeHeapSnapshot()`
+
+Returns `String` - The file path if the heap snapshot has been created successfully.


The method has to be synchronous, right?

What is returned if there is an error?
If a method throws an exception, it has to be documented.

alexeykuzmin · 2018-09-14T21:31:35Z

atom/common/api/atom_bindings.cc

+  }
+
+  if (!file.IsValid()) {
+    args->ThrowError("takeHeapSnapshot failed - cannot create file");


Can you please move all error messages to constants in an anonymous namespace on the top of the file?

why? we're not doing that it other places

atom/common/api/atom_bindings.cc

lib/browser/api/web-contents.js

deepak1556

The implementation looks good wrt to file handling for a sandboxed process, Is there any reason this api has to be a synchronous version ? Making it async would allow us to avoid sync IPC as well avoid file operations on the UI task runner, which would be a good thing.

miniak · 2018-09-16T08:17:30Z

@deepak1556, @alexeykuzmin I've refactored the implementation significantly, the file handle is now pushed from the main process when webContents.takeHeapSnapshot() is used. process.takeHeapSnapshot() can only be used in non-sandboxed processes. It's more secure like that.

miniak · 2018-09-16T08:24:25Z

@deepak1556 I am not sure if it is possible to offload this to a background thread. I've tried taking the snapshot on the main thread and then saving it on IO thread, it didn't work.

alexeykuzmin · 2018-09-17T13:48:24Z

spec/api-process-spec.js

+      const filePath = path.join(remote.app.getPath('userData'), 'test.heapsnapshot')
+      const success = process.takeHeapSnapshot(filePath)
+      expect(success).to.be.true()
+      fs.unlink(filePath, () => done())


Please call expect after the unlink.
And it would be better to call it sync version fs.unlinkSync()

refactored a bit differently

spec/api-process-spec.js

spec/api-web-contents-spec.js

alexeykuzmin · 2018-09-17T13:54:45Z

spec/api-web-contents-spec.js

+    const filePath = path.join(remote.app.getPath('userData'), 'test.heapsnapshot')
+
+    afterEach(done => {
+      fs.unlink(filePath, () => done())


Please call in a beforeEach hook.
afterEach won't be called for failed tests =/

refactored a bit differently

spec/api-process-spec.js

alexeykuzmin

Please replace "it('returns false on failure'" with "it('throws an exception on failure'" in the tests.
👍

miniak · 2018-09-17T23:12:04Z

@MarshallOfSound I've changed the implementation a bit and I don't need to return a string anymore. The filePath is now passed to the method. I just need to return whether it succeeded or failed. Boolean also has issues so I just throw an exception when it fails.

MarshallOfSound · 2018-09-17T23:34:04Z

docs/api/process.md

+* `filePath` String - Path to the output file.
+
+Takes a V8 heap snapshot and saves it to `filePath`.
+Throws an exception in case of failure.


We shouldn't make a worse API because the docs generator is breaking the PR. This should be returning a success state not throwing an error. Working around a docs generator bug by changing the API is a bad idea.

Requesting changes and as mentioned I'll look into the docs generator issue

@MarshallOfSound I get your point, but is it actually worse? An exception can give you potentially much more information why something failed than just a false return value.

waiting for https://github.com/electron/electron-typescript-definitions/releases/tag/v2.0.1 to be published to NPM

@miniak It was published when that github release went out an hour or so ago --> https://www.npmjs.com/package/electron-typescript-definitions/v/2.0.1

It's an automated pipeline

The npmjs webpage took a while to reflect the update.

Working around a docs generator bug by changing the API is a bad idea.

I agree that generally it is a bad idea.

This should be returning a success state not throwing an error.

But in this case I would argue.
In most cases an exception will be thrown because of a user "error", like an invalid file path or lack of permission to write to a specified location.
Returning just a boolean "state" value simply doesn't provide any useful feedback to a user.

@alexeykuzmin Sure if we have things like EACCESS, EPERM and other such standard file errors then throwing them to be consistent with things like node fs makes sense. From what I saw this was just throwing a generic "something went wrong" error which is just as helpful as returning false but harder / more annoying to catch.

miniak · 2018-09-18T07:42:49Z

@MarshallOfSound electron-typescript-definitions bumped to 2.0.1, boolean return value restored.

miniak · 2018-09-18T13:42:42Z

@MarshallOfSound I am going to push a small fix

diff --git a/atom/common/heap_snapshot.cc b/atom/common/heap_snapshot.cc
index d66ec474c..bb36e98f5 100644
--- a/atom/common/heap_snapshot.cc
+++ b/atom/common/heap_snapshot.cc
@@ -42,6 +42,8 @@ bool TakeHeapSnapshot(v8::Isolate* isolate, base::File* file) {
     return false;
 
   auto* snapshot = isolate->GetHeapProfiler()->TakeHeapSnapshot();
+  if (!snapshot)
+    return false;
 
   HeapSnapshotOutputStream stream(file);
   snapshot->Serialize(&stream, v8::HeapSnapshot::kJSON);

release-clerk · 2018-09-18T18:00:34Z

Release Notes Persisted

Added process.getHeapSnapshot() / webContents.takeHeapSnapshot() for taking V8 heap snapshots.

miniak requested a review from a team September 4, 2018 22:20

miniak force-pushed the miniak/heap-snapshot branch 4 times, most recently from 7d30e10 to ab6a9fe Compare September 4, 2018 22:46

miniak requested review from juturu and alexeykuzmin September 4, 2018 22:48

miniak force-pushed the miniak/heap-snapshot branch 4 times, most recently from 58ec892 to a238aa2 Compare September 5, 2018 18:19

miniak force-pushed the miniak/heap-snapshot branch 2 times, most recently from e662710 to 139eb32 Compare September 5, 2018 19:11

miniak force-pushed the miniak/heap-snapshot branch 3 times, most recently from 0a9532f to a2fa4cf Compare September 6, 2018 19:01

miniak requested a review from a team September 6, 2018 19:01

miniak force-pushed the miniak/heap-snapshot branch from a2fa4cf to 852c51e Compare September 6, 2018 19:15

miniak changed the title ~~[wip] feat: add process.getHeapSnapshot()~~ [wip] feat: add process.takeHeapSnapshot() Sep 6, 2018

miniak force-pushed the miniak/heap-snapshot branch 4 times, most recently from e4ca655 to 174dd5a Compare September 12, 2018 21:52

alexeykuzmin suggested changes Sep 14, 2018

View reviewed changes

miniak force-pushed the miniak/heap-snapshot branch from 174dd5a to f717965 Compare September 14, 2018 22:32

MarshallOfSound reviewed Sep 15, 2018

View reviewed changes

lib/browser/api/web-contents.js Outdated Show resolved Hide resolved

miniak changed the title ~~[wip] feat: add process.takeHeapSnapshot()~~ [wip] feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot() Sep 15, 2018

deepak1556 reviewed Sep 15, 2018

View reviewed changes

miniak force-pushed the miniak/heap-snapshot branch from c536b5f to 324eb17 Compare September 16, 2018 08:12

miniak force-pushed the miniak/heap-snapshot branch from 324eb17 to ed93b64 Compare September 16, 2018 08:22

miniak force-pushed the miniak/heap-snapshot branch from ed93b64 to 42baf25 Compare September 16, 2018 09:10

miniak requested review from nornagon and ckerr September 16, 2018 10:26

alexeykuzmin suggested changes Sep 17, 2018

View reviewed changes

miniak force-pushed the miniak/heap-snapshot branch from 42baf25 to b0d42b8 Compare September 17, 2018 20:29

deepak1556 approved these changes Sep 17, 2018

View reviewed changes

alexeykuzmin reviewed Sep 17, 2018

View reviewed changes

spec/api-process-spec.js Show resolved Hide resolved

alexeykuzmin approved these changes Sep 17, 2018

View reviewed changes

miniak force-pushed the miniak/heap-snapshot branch 2 times, most recently from d4305d6 to 83b7f23 Compare September 17, 2018 21:35

MarshallOfSound requested changes Sep 17, 2018

View reviewed changes

MarshallOfSound mentioned this pull request Sep 18, 2018

fix: handle top level primitives in the process module electron/typescript-definitions#113

Merged

miniak force-pushed the miniak/heap-snapshot branch 2 times, most recently from a3a1e6b to a02e4ac Compare September 18, 2018 07:41

MarshallOfSound approved these changes Sep 18, 2018

View reviewed changes

feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot()

69294ae

miniak force-pushed the miniak/heap-snapshot branch from a02e4ac to 69294ae Compare September 18, 2018 13:44

codebytere approved these changes Sep 18, 2018

View reviewed changes

codebytere merged commit e22142e into master Sep 18, 2018

deepak1556 deleted the miniak/heap-snapshot branch September 18, 2018 18:02

ckerr mentioned this pull request Dec 18, 2018

Draft 4.0.0 Release Notes #16117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot() #14456

feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot() #14456

miniak commented Sep 4, 2018 •

edited

alexeykuzmin commented Sep 5, 2018

MarshallOfSound commented Sep 6, 2018

alexeykuzmin commented Sep 14, 2018

alexeykuzmin left a comment

alexeykuzmin Sep 14, 2018

alexeykuzmin Sep 14, 2018

miniak Sep 15, 2018

deepak1556 left a comment •

edited

miniak commented Sep 16, 2018

miniak commented Sep 16, 2018

alexeykuzmin Sep 17, 2018

miniak Sep 17, 2018

alexeykuzmin Sep 17, 2018

miniak Sep 17, 2018

alexeykuzmin left a comment

miniak commented Sep 17, 2018

MarshallOfSound Sep 17, 2018

miniak Sep 18, 2018

miniak Sep 18, 2018

MarshallOfSound Sep 18, 2018

miniak Sep 18, 2018

alexeykuzmin Sep 18, 2018

MarshallOfSound Sep 18, 2018 •

edited

miniak commented Sep 18, 2018

miniak commented Sep 18, 2018

release-clerk bot commented Sep 18, 2018

feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot() #14456

feat: add process.takeHeapSnapshot() / webContents.takeHeapSnapshot() #14456

Conversation

miniak commented Sep 4, 2018 • edited

Description of Change

Checklist

Release Notes

alexeykuzmin commented Sep 5, 2018

MarshallOfSound commented Sep 6, 2018

alexeykuzmin commented Sep 14, 2018

alexeykuzmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepak1556 left a comment • edited

Choose a reason for hiding this comment

miniak commented Sep 16, 2018

miniak commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykuzmin left a comment

Choose a reason for hiding this comment

miniak commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarshallOfSound Sep 18, 2018 • edited

Choose a reason for hiding this comment

miniak commented Sep 18, 2018

miniak commented Sep 18, 2018

release-clerk bot commented Sep 18, 2018

miniak commented Sep 4, 2018 •

edited

deepak1556 left a comment •

edited

MarshallOfSound Sep 18, 2018 •

edited