Proposal: (almost) Entirely remove the need for preload script

[timfish]

Problem

Even after the large refactor for v3, the Electron SDK can be a little at odds with Sentry's developer philosophy:

• We optimize towards a great out of the box experience
• Assume Novice Developers
• etc, etc

Electron is a complex beast.

Electrons IPC modules are the primary means to communicate between the main and renderer processes. However the tightening of the security model and the many configuration options has made it complex for library authors and their consumers. The only reliable way to expose IPC across all configurations is via contextBridge in a preload script. We can make a best effort at injecting preload scripts but this is not compatible with bundlers and instructing users to configure preload scripts with various bundlers is not simple and still leaves outstanding issues (#372 (comment)) unsolved.

There is another option!

The protocol module allows you to setup a custom protocol from the main process which you can call from a renderer via fetch.

This has a few downsides over IPC:

• Only fully supported with CSP in Electron >= v5
• No easy way to initiate main to renderer communication
  • Not a problem for us as we don't do this
  • Turns out you can emulate this by keeping stream open!
• No way to tell which renderer made the call
  • Less context over which renderer the error came from

Proposal

• Setup IPC in the main process
• Setup sentry-ipc protocol in main process when Electron >= v5
• Attempt to inject preload scripts
• In renderers, favour IPC but fallback to fetch('sentry-ipc://event-etc')

Unsupported Configurations

When using Electron < v5 while bundling the main process code there is no custom protocol and preload injection will fail. Users will be required to configure a preload script.

Downsides

When users bundle the main process code, the SDK cannot inject preload scripts, will fallback to fetch + custom protocol and you'll loose context over exactly which renderer JavaScript errors come from. The URL/path information is all sill included.

Users can negate this by configuring a preload script.

Upsides

Unless you're using Electron < v5 and bundling your main process code, there is no longer a need to care about preload scripts.

[AbhiPrasad]

No way to tell which renderer made the call

Can we monkey patch fetch to attach context from the renderer somehow?

[timfish]

Can we monkey patch fetch to attach context from the renderer somehow?

We can just pass the renderer identifier through with the event. The issue is that in a renderer with nodeIntegration: false (now the default and highly recommended to not enable), the renderer is just a regular browser tab so there is no way to identify the renderer from there.

The only place we can reliably identify the renderer is in the main process, and the protocol api doesn't currently pass this information.

We can identify that the error came from a renderer, we just can't tell exactly which one.

However, we could add is as an option to the renderer/browser init:

init({
  processName: 'main-window'
})

EDIT
Come to think of it, passing processName might actually be preferable to our current way of custom naming renderers from the main process.

Currently, users have to keep track of which WebContents refers to which process which is a bit clunky:

export interface ElectronMainOptions extends NodeOptions {
  /**
   * Callback to allow custom naming of renderer processes
   * If the callback is not set, or it returns `undefined`, the default naming
   * scheme is used.
   */
  getRendererName?: (contents: WebContents) => string | undefined;
}

[AbhiPrasad]

+1 to adding processName.

Feel like this is something we should open an issue about in https://github.com/electron/electron, perhaps we can get a better experience out of the box for future versions.

[timfish]

I've looked into this a bit further at it looks like we wont be able to remove the existing getRendererName, however unergonomic it might be. It's used to add custom names to renderers from native crashes where we only have access to the WebContents instance that crashed.

Out of interest, I did a quick search on Github and couldn't find any public usages of getRendererName although I know I use it in a proprietary app as I added the API to the SDK for this purpose!

It would be a bit confusing to have both getRendererName in the main process and processName in the renderer. I'll have an explore and see what else might be possible...

Feel like this is something we should open an issue about in electron

Yes, I'll do that!