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.

Sign up for GitHub

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

Jump to bottom

Don't initialize NSApplication subclass during initialization #3993

Merged
merged 2 commits into from
May 17, 2024
Merged

Don't initialize NSApplication subclass during initialization #3993

merged 2 commits into from
May 17, 2024

Conversation

triplef
Copy link
Contributor

@triplef triplef commented May 16, 2024

📜 Description

💡 Motivation and Context

Fixes #3992.

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@codecov Codecov
Copy link

codecov bot commented May 16, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.877%. Comparing base (2b9e267) to head (ded95a8).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3993       +/-   ##
=============================================
+ Coverage   90.858%   90.877%   +0.019%     
=============================================
  Files          594       594               
  Lines        46391     46390        -1     
  Branches     16623     16607       -16     
=============================================
+ Hits         42150     42158        +8     
+ Misses        4171      4052      -119     
- Partials        70       180      +110
Files Coverage Δ
Sources/Sentry/SentrySDK.m 86.585% <ø> (ø)

... and 31 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b9e267...ded95a8. Read the comment docs.

armcknight
armcknight reviewed May 16, 2024
View reviewed changes
Copy link
Member

@armcknight armcknight left a comment •
edited

Choose a reason for hiding this comment

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

@philipphofmann Is there a reason the mac exception reporting isn't considered an integration, or a component of the crash reporter integration? Because if crash reporting is disabled, then we're crashing apps for no good reason.

If we moved this line of code to SentryOptions.defaultIntegrations or embedded it in SentryCrashInstallation, then it'd be nicely consistent with the rest of the -[NSObject class] invocations, and we wouldn't need this weird side effect here.

@triplef thanks for the PR! This fixes something that was recently added in #3957, I'm more questioning the original addition than your fix.

@triplef
Copy link
Contributor Author

triplef commented May 16, 2024

Thanks for taking a look @armcknight and feel free to discard this PR if there’s a better solution!

@philipphofmann
Copy link
Member

Is there a reason the mac exception reporting isn't considered an integration, or a component of the crash reporter integration?

No reason, no. It has always been like that, and we hardly touched it.

then it'd be nicely consistent with the rest of the -[NSObject class] invocations

What do you mean by that, @armcknight?

@brustolin
Copy link
Contributor

Is there a reason the mac exception reporting isn't considered an integration, or a component of the crash reporter integration?

As far as I know, having a NSApplication subclass is the only way to capture unhandled objc errors for macOS.

@brustolin
Copy link
Contributor

This solution looks good to me. It also avoid the stripping of the SentryCrashExceptionApplication.
Having it as an integration will require more refactoring and options, this could be a new issue for a later improvement.

philipphofmann
philipphofmann approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@philipphofmann
Copy link
Member

Is there a reason the mac exception reporting isn't considered an integration, or a component of the crash reporter integration?

As far as I know, having a NSApplication subclass is the only way to capture unhandled objc errors for macOS.

Yes, you need the NSApplication subclass thingy, but we could move the logic of calling [SentryCrashExceptionApplication class]; into the SentryCrashIntegration so it only gets called when you install the crash integration, but as most people have it enabled and we fixed the bug, I think it's okay to keep the code in the SDK.m.

I also tested the fix and it works. We could add some UI tests for the macOS sample to test the regression, but I don't think it's worth the effort.

@philipphofmann philipphofmann merged commit 0af45a9 into getsentry:main May 17, 2024
61 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armcknight armcknight armcknight left review comments

@philipphofmann philipphofmann philipphofmann approved these changes

@brustolin brustolin Awaiting requested review from brustolin brustolin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sentry registers own NSApplication subclass (8.26 regression)
4 participants
@triplef @philipphofmann @brustolin @armcknight