feat: Fallback to custom protocol #377

timfish · 2021-11-04T18:03:33Z

Closes #376 and #372

Adds support for receiving renderer events and scope via a custom protocol.

Add ipcMode: IPCMode option to main process. Default IPCMode.Both enables both IPC and protocol for best compatibility
Add getSessions: () => [session] option to main process. Defaults to () => [session.defaultSession]
- This allows users to pass in custom sessions if defaultSession doesn't suffice
- We use sessions for setting up the protocol and preload injection
- This option is a function because init must be called before app ready and sessions cannot be accessed until after app ready!
Configures custom protocol in main process when Electron >= v5 and when ipcMode is IPCMode.Protocol or IPCMode.Both
Renderers send events and scope via IPC or protocol. IPC is favoured because it has a bit more context
PreloadInjection integration modified to use getSessions option and not to run at all when ipcMode is IPCMode.Protocol
Modified preload scripts to guard from being run multiple times
- This could occur if preload injection succeeds and user also adds preload
Modified existing examples to no longer use preload

Also:

Moves contexts.electron.crashed_process to tags['event.process'] so users can filter by process type

Added Tests

Webpack example WITH preload
Renderer error with ipcMode: IPCMode.Protocol
Preload JavaScript error
Preload JavaScript error with ipcMode: IPCMode.Protocol
Webpack test using protocol with strict Content-Security-Policy
Sending JavaScript error from <iframe> within renderer!
Console log when SDK not started in main process
Throws error when init after app ready

timfish · 2021-11-05T17:14:43Z

I haven't yet decided what to do about processName and/or getRendererName.

🤔

For JavaScript errors in the renderers, we can pass processName through with the events
For native crashed renderers, we only have access to the WebContents instance in the main process
When renderers start up, I can pass the processName and probably link it to the WebContents via window.location.href but this has a number of downsides:
- Won't work if the renderer crashes before SDK init
- If multiple renderers use the same path, we can only link them by the order that they are started which will not be reliable

timfish · 2021-11-07T14:11:43Z

Currently v2 and v3-beta add the following contexts:

Renderer JavaScript and native crashes

    "contexts": {
      "electron": {
        "crashed_process": "WebContents[1]",
        "crashed_url": "app:https:///src/index.html"
      }
    },

Main process JavaScript and native crashes:
(Confusingly and forever stuck for backwards compatibility, Electrons main process is called browser)

    "contexts": {
      "electron": {
        "crashed_process": "browser"
      }
    },

The naming of these electron.crashed_* properties suggests that they should only be populated for native crashes which is currently not the case. In addition, having these in contexts means users will not be able to filter on them.

I would suggest that we move contexts.electron.crashed_process to a tag and set it for any kind of event. This means we'd end up with:

Tag	Possible Values
`event.origin`	`electron`
`event.environment`	`javascript` \| `native`
`event.process`	`browser` \| `renderer[id]` \| result of `getRendererName()`

Or would event.origin be a more suitable tag for the source process? Open to suggestions!

When it comes to URLs, we already have these for renderer JavaScript events via request.url. For renderer native crashes, shall we keep this at contexts.electron.crashed_url?

AbhiPrasad

Nice, the tests make everything pretty easy to understand.

Won't work if the renderer crashes before SDK init

I think we shouldn't spend too much time on this. Other platforms have a similar problem - the only solution is if the framework/OS gives us a way to read those crash events afterwards.

If multiple renderers use the same path, we can only link them by the order that they are started which will not be reliable

Is it a common pattern in Electron apps for multiple renderers to use the same path? If so, let's just not worry too much about linking processName and window.location.href, maybe we leave it for a future patch and just open an issue to collect feedback.

would event.origin be a more suitable tag for the source process

IMO the 3 tags shown in the table provide adequate cardinality to identify the crash location, don't think we need to change event.origin.

For renderer native crashes, shall we keep this at contexts.electron.crashed_url

Yup think this is a great idea.

Lmk and I can go over for a final review. Thanks Tim, you a champ!

AbhiPrasad · 2021-11-08T13:09:01Z

src/renderer/ipc.ts

+ );
+
+ // We include sentry_key in the URL so these dont end up in fetch breadcrumbs
+ // https://github.com/getsentry/sentry-javascript/blob/a3f70d8869121183bec037571a3ee78efaf26b0b/packages/browser/src/integrations/breadcrumbs.ts#L240


This is kinda hacky - but a nice solution

Yeah, maybe a bit fragile. I initially attempted to filter the breadcrumbs when they're sent from renderer to main but realised that scope updates were triggering fetch, which was triggering scope updates, which.... yeah not great!

Might be nice to have a way to disable Sentry fetch breadcrumbs via a magic header?

Lemme ask around to see what others think.

We're pinned to a specific version of @sentry/browser so this is only prone to breaking on dependency updates. Shall we merge this as it is and I'll open an issue on getsentry/javascript to track this?

Yes please, opening an issue would be great

timfish · 2021-11-08T19:00:59Z

Is it a common pattern in Electron apps for multiple renderers to use the same path?

I imagine so.

We've got a production app that has multiple windows that use a single entry point and the UI varies entirely by the startup hash/fragment identifier . If your multiple windows share most of their code, this is much easier than configuring webpack for shared dependency chunks between multiple entry points!

Anyway, my hairbrained idea was linking renderers via the fetch referrer URL which I've since remembered doesn't include the fragment identifier so that wouldn't have been be very useful 😂

Fallback to protocol

ea4333b

timfish force-pushed the feat/protocol branch from a9844fe to ea4333b Compare November 5, 2021 02:09

Simplify

d51c134

timfish marked this pull request as ready for review November 5, 2021 12:03

Test events from iframe

ff03e95

timfish requested a review from AbhiPrasad November 5, 2021 14:59

timfish added 2 commits November 5, 2021 15:11

Add strict CSP to basic webpack test

b6f5ed9

Merge 'upstream/master' into feat/protocol

f28a6ce

Fix failing tests after merge

cf0eed3

Add preload tests

b1dc23d

AbhiPrasad reviewed Nov 8, 2021

View reviewed changes

Move crashed_process to tags

493ef89

timfish mentioned this pull request Nov 8, 2021

Using sentry-electron with contextIsolation, preload.js, and nodeIntegration off #366

Closed

timfish merged commit 00e7a46 into getsentry:master Nov 10, 2021

timfish mentioned this pull request Nov 10, 2021

Provide a means to exclude certain fetch requests from breadcrumbs getsentry/sentry-javascript#4141

Closed

timfish deleted the feat/protocol branch November 10, 2021 12:59

andersk mentioned this pull request Mar 5, 2022

Importing @sentry/electron without calling init() yields an error #427

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Fallback to custom protocol #377

feat: Fallback to custom protocol #377

timfish commented Nov 4, 2021 •

edited

Loading

timfish commented Nov 5, 2021 •

edited

Loading

timfish commented Nov 7, 2021

AbhiPrasad left a comment

AbhiPrasad Nov 8, 2021

timfish Nov 8, 2021 •

edited

Loading

AbhiPrasad Nov 8, 2021

timfish Nov 10, 2021

AbhiPrasad Nov 10, 2021

timfish commented Nov 8, 2021

feat: Fallback to custom protocol #377

feat: Fallback to custom protocol #377

Conversation

timfish commented Nov 4, 2021 • edited Loading

timfish commented Nov 5, 2021 • edited Loading

timfish commented Nov 7, 2021

AbhiPrasad left a comment

Choose a reason for hiding this comment

AbhiPrasad Nov 8, 2021

Choose a reason for hiding this comment

timfish Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

AbhiPrasad Nov 8, 2021

Choose a reason for hiding this comment

timfish Nov 10, 2021

Choose a reason for hiding this comment

AbhiPrasad Nov 10, 2021

Choose a reason for hiding this comment

timfish commented Nov 8, 2021

timfish commented Nov 4, 2021 •

edited

Loading

timfish commented Nov 5, 2021 •

edited

Loading

timfish Nov 8, 2021 •

edited

Loading