fix: Expose missing Add/RemoveExtraParameter methods to macOS node child processes (electron#15790)

lepinay · zcbenz · commit 4e0e6154066f · 2019-07-29T09:46:35.000+09:00
* Expose missing crash reporter methods in mac node processes

* Crashpad migration
diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc
@@ -5,6 +5,7 @@
 #include "shell/app/node_main.h"
 
 #include <memory>
+#include <string>
 #include <utility>
 
 #include "base/command_line.h"
@@ -31,6 +32,16 @@
 
 namespace electron {
 
+#if !defined(OS_LINUX)
+void AddExtraParameter(const std::string& key, const std::string& value) {
+  crash_reporter::CrashReporter::GetInstance()->AddExtraParameter(key, value);
+}
+
+void RemoveExtraParameter(const std::string& key) {
+  crash_reporter::CrashReporter::GetInstance()->RemoveExtraParameter(key);
+}
+#endif
+
 int NodeMain(int argc, char* argv[]) {
   base::CommandLine::Init(argc, argv);
 
@@ -87,6 +98,12 @@ int NodeMain(int argc, char* argv[]) {
     // Setup process.crashReporter.start in child node processes
     auto reporter = mate::Dictionary::CreateEmpty(gin_env.isolate());
     reporter.SetMethod("start", &crash_reporter::CrashReporter::StartInstance);
+
+#if !defined(OS_LINUX)
+    reporter.SetMethod("addExtraParameter", &AddExtraParameter);
+    reporter.SetMethod("removeExtraParameter", &RemoveExtraParameter);
+#endif
+
     process.Set("crashReporter", reporter);
 
     mate::Dictionary versions;
diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js
@@ -23,6 +23,7 @@ describe('crashReporter module', () => {
 
   let originalTempDirectory = null
   let tempDirectory = null
+  const specTimeout = 180000
 
   before(() => {
     tempDirectory = temp.mkdirSync('electronCrashReporterSpec-')
@@ -56,7 +57,7 @@ describe('crashReporter module', () => {
       })
 
       it('should send minidump when renderer crashes', function (done) {
-        this.timeout(180000)
+        this.timeout(specTimeout)
 
         stopServer = startServer({
           callback (port) {
@@ -68,7 +69,7 @@ describe('crashReporter module', () => {
       })
 
       it('should send minidump when node processes crash', function (done) {
-        this.timeout(180000)
+        this.timeout(specTimeout)
 
         stopServer = startServer({
           callback (port) {
@@ -84,7 +85,7 @@ describe('crashReporter module', () => {
       })
 
       it('should not send minidump if uploadToServer is false', function (done) {
-        this.timeout(180000)
+        this.timeout(specTimeout)
 
         let dumpFile
         let crashesDir = crashReporter.getCrashesDirectory()
@@ -149,8 +150,46 @@ describe('crashReporter module', () => {
         })
       })
 
+      it('should send minidump with updated extra parameters when node processes crash', function (done) {
+        if (process.platform === 'linux') {
+          // FIXME(alexeykuzmin): Skip the test.
+          // this.skip()
+          return
+        }
+        // TODO(alexeykuzmin): Skip the test instead of marking it as passed.
+        if (process.env.APPVEYOR === 'True') return done()
+        this.timeout(specTimeout)
+        stopServer = startServer({
+          callback (port) {
+            const crashesDir = path.join(app.getPath('temp'), `${process.platform === 'win32' ? 'Zombies' : app.getName()} Crashes`)
+            const version = app.getVersion()
+            const crashPath = path.join(fixtures, 'module', 'crash.js')
+            if (process.platform === 'win32') {
+              const crashServiceProcess = childProcess.spawn(process.execPath, [
+                `--reporter-url=http://127.0.0.1:${port}`,
+                '--application-name=Zombies',
+                `--crashes-directory=${crashesDir}`
+              ], {
+                env: {
+                  ELECTRON_INTERNAL_CRASH_SERVICE: 1
+                },
+                detached: true
+              })
+              remote.process.crashServicePid = crashServiceProcess.pid
+            }
+            childProcess.fork(crashPath, [port, version, crashesDir], { silent: true })
+          },
+          processType: 'browser',
+          done: done,
+          preAssert: fields => {
+            expect(String(fields.newExtra)).to.equal('newExtra')
+            expect(String(fields.removeExtra)).to.equal(undefined)
+          }
+        })
+      })
+
       it('should send minidump with updated extra parameters', function (done) {
-        this.timeout(180000)
+        this.timeout(specTimeout)
 
         stopServer = startServer({
           callback (port) {
@@ -395,7 +434,7 @@ const waitForCrashReport = () => {
   })
 }
 
-const startServer = ({ callback, processType, done }) => {
+const startServer = ({ callback, processType, done, preAssert, postAssert }) => {
   let called = false
   const server = http.createServer((req, res) => {
     const form = new multiparty.Form()
@@ -413,10 +452,12 @@ const startServer = ({ callback, processType, done }) => {
       expect(String(fields._productName)).to.equal('Zombies')
       expect(String(fields._companyName)).to.equal('Umbrella Corporation')
       expect(String(fields._version)).to.equal(app.getVersion())
+      if (preAssert) preAssert(fields)
 
       const reportId = 'abc-123-def-456-abc-789-abc-123-abcd'
       res.end(reportId, () => {
         waitForCrashReport().then(() => {
+          if (postAssert) postAssert(reportId)
           expect(crashReporter.getLastCrashReport().id).to.equal(reportId)
           expect(crashReporter.getUploadedReports()).to.be.an('array').that.is.not.empty()
           expect(crashReporter.getUploadedReports()[0].id).to.equal(reportId)
diff --git a/spec/fixtures/module/crash.js b/spec/fixtures/module/crash.js
@@ -10,4 +10,10 @@ process.crashReporter.start({
   }
 })
 
+if (process.platform !== 'linux') {
+  process.crashReporter.addExtraParameter('newExtra', 'newExtra')
+  process.crashReporter.addExtraParameter('removeExtra', 'removeExtra')
+  process.crashReporter.removeExtraParameter('removeExtra')
+}
+
 process.nextTick(() => process.crash())
