Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Fallback to custom protocol #377

Merged
merged 8 commits into from
Nov 10, 2021
Merged

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Nov 4, 2021

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 timfish marked this pull request as ready for review November 5, 2021 12:03
@timfish
Copy link
Collaborator Author

timfish commented Nov 5, 2021

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
Copy link
Collaborator Author

timfish commented Nov 7, 2021

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?

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

);

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda hacky - but a nice solution

Copy link
Collaborator Author

@timfish timfish Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme ask around to see what others think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, opening an issue would be great

@timfish
Copy link
Collaborator Author

timfish commented Nov 8, 2021

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 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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