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

ANR Detection Improvements Needed for Modal Dialog Handling #1016

Closed
smeubank opened this issue Nov 12, 2024 · 6 comments · Fixed by #1032
Closed

ANR Detection Improvements Needed for Modal Dialog Handling #1016

smeubank opened this issue Nov 12, 2024 · 6 comments · Fixed by #1032
Assignees

Comments

@smeubank
Copy link
Member

Problem Statement

Summary

The current ANR (Application Not Responding) detection in the Electron SDK generates false positives when modal dialogs are shown, particularly on Windows. This leads to misleading non-responsiveness reports for intentional UI pauses.

Problem Description

When using modal dialogs (e.g., dialog.showSaveDialog) in Electron applications, the UI thread is intentionally blocked while waiting for user input. However, the ANR detection system interprets this as an application freeze and generates false positive reports.

Current Behavior

  • ANR events are triggered after 5 seconds of UI thread blocking
  • Modal dialogs that take longer than 5 seconds to dismiss trigger ANR reports
  • No built-in way to distinguish between actual freezes and intentional modal pauses
  • High volume of false positives in common scenarios

Expected Behavior

The ANR detection should:

  1. Automatically detect and handle system modal dialogs
  2. Provide API to temporarily disable ANR detection during known modal operations
  3. Include context about foreground window/process ownership in ANR reports
  4. Distinguish between intentional UI blocks and actual application freezes

Solution Brainstorm

  1. Automatic Detection:

    • Detect when foreground window is owned by system process
    • Skip ANR reporting in these scenarios
  2. API Improvements:

    • Expose methods to manually pause/resume ANR detection
    • Allow configuration of modal dialog handling
    • Provide hooks for custom modal detection logic
  3. Enhanced Context:

    • Include foreground process information in ANR reports
    • Add window ownership details to event metadata
    • Enable filtering/grouping of modal-related ANRs

Impact

  • Customers are disabling ANR detection due to noise
  • Reduced reliability of actual freeze detection
  • Decreased confidence in ANR reporting accuracy

Related Documentation

Notes

  • Current workarounds using powerMonitor events exist but aren't officially supported/documented
    • not verified
  • Re-enabling consideration depends on proper modal handling implementation
@smeubank
Copy link
Member Author

https://www.electronjs.org/docs/latest/api/dialog#dialogshowsavedialogwindow-options

this is the electron API, i do not have anymore concrete examples of modals yet

@timfish
Copy link
Collaborator

timfish commented Nov 14, 2024

You could create a helper function that disables ANR while the callback is being called. It would look something like this:

export async function disableAnrIntegration<T>(doWork: () => Promise<T>): Promise<T> {
  const integration = getClient()?.getIntegrationByName('Anr') as ReturnType<typeof nodeAnrIntegration> | undefined;

  try {
    integration?.stopWorker();
    return await doWork();
  } finally {
    integration?.startWorker();
  }
}

This would then be used like this:

const result = await disableAnrIntegration(() => dialog.showSaveDialog());

I'll get something like this added to the SDK so it's easier to use. We could automatically wrap everything in the Electron dialog module too.

@kpujjigit
Copy link

Hey @timfish I'm the original engineer at Sentry that proposed this feedback from a client. So it looks like the above draft would expose methods to manually pause/resume ANR detection? Would this also implement automatic detection of when system modal dialogs appear?

For further context, this feedback was provided to Sentry in response to how we handle the showSaveDialog modal. The application in-question presents a save dialogue box to open/save data, which invokes an intentional UI blockage, but the Electron SDK currently flags that as an app freeze. As a result, false positives are captured.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Nov 20, 2024
@timfish
Copy link
Collaborator

timfish commented Nov 20, 2024

I've proposed adding this to the Node SDK (where the ANR feature originates) getsentry/sentry-javascript#14359

Once that is merged I can automatically wrap the Electron dialog code in this SDK which should fix the issue.

@kpujjigit
Copy link

@timfish how would wrapping the Electron dialog code automatically affect end users who'd still prefer to wrap modals manually? Perhaps this could be built as an integration or flag to accommodate?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 2, 2024
@timfish
Copy link
Collaborator

timfish commented Dec 4, 2024

how would wrapping the Electron dialog code automatically affect end users who'd still prefer to wrap modals manually?

I think initially it will be left up to users to wrap these manually and we will consider wrapping automatically optionally later?

@getsantry getsantry bot removed the status in GitHub Issues with 👀 3 Dec 4, 2024
Lms24 pushed a commit to getsentry/sentry-javascript that referenced this issue Dec 4, 2024
- Ref getsentry/sentry-electron#1016

```ts
disableAnrDetectionForCallback(() => dialog.openSaveDialog());
```

Maybe this should just go in the Electron SDK? 

I added it for Node because it may be useful outside of Electron and at
least here it has access to some private variables and types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants