Start crash reporter inside child processes

[ramya-rao-a]

Starting the crash reporter for the extension host
Related to #21944

If this works as expected, we can do the same for the rest of the child processes that we spawn/fork and need crash reporting

[mention-bot]

@ramya-rao-a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tyriar and @joaomoreno to be potential reviewers.

[bpasero]

@ramya-rao-a I suggest to just introduce process.env variables for the crash reporter metadata and read them in the forked process. Using the local storage as a way to communicate data is a bad hack. At the very minimum it should be a ICrashReporterService that we can require in places where we need it (except for forked processes where we do not have services). Anyway, using process environment would save you that issue as well as you would not have to pass arguments around.

We still only have one crash reporter process even when we start it from other processes, right?

[ramya-rao-a]

@bpasero

Using the local storage as a way to communicate data is a bad hack

Will look into creating ICrashReporterService which can have 2 methods. One to get the args another to start the crash reporter

We still only have one crash reporter process even when we start it from other processes, right?

In Mac which uses crashpad yes. There is a single handler for monitoring crashes for all processes (main, renderer, child)

In Windows, we need start another process that does the monitoring. This PR doesn't include that. See https://github.com/electron/electron/blob/master/docs/api/crash-reporter.md#crashreporterstartoptions

I'll have to dig a little more to get the more details

[bpasero]

@ramya-rao-a what we could also do as a starter is to only enable this on macOS for e.g. the extension host.

[ramya-rao-a]

@bpasero PR updated as we discussed

[bpasero]

@ramya-rao-a I pushed some refactorings and a bugfixes on top of your commit to this PR, please have a look. When trying this out locally I was getting a crash right on startup on the extension host.

One thing to look into is your use of crashesDirectory. Shouldn't this property be set on the crash reporter options directly and not the extra bag? And then, we should probably not set this option all the time but only from the object returned from getChildProcessStartOptions

From the documentation at https://github.com/electron/electron/blob/master/docs/api/crash-reporter.md#crashreporterstartoptions it was not clear to me how crashesDirectory has to be used...

[ramya-rao-a]

@bpasero

crash right on startup on the extension host

Ah! This was due to the object being passed and not the stringified object. That was a last minute change I made as an after thought, but didn't test. Sorry, my bad.

crashesDirectory on the crash reporter options directly and not the extra bag

Yes, this should be part of the main payload and not the extra bag. Fixed with 95c8517

we should probably not set this option all the time but only from the object returned from getChildProcessStartOptions

Ideally yes. This option is not used by the main/renderer process, so I didn't see the harm in sending it all the time rather than just for the child process. Fixed with 95c8517

btw I spent a lot of time getting the service plumbing to work, but kept getting unhelpful errors while instantiating the ExtensionHostProcessWorker which I then narrowed down to my service not defining "_serviceBrand". Do we have any docs around do's don'ts for such cases?

[bpasero]

@ramya-rao-a looks good now 👍