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

fix: crash when call SentryUIApplication in background thread #3855

Merged
merged 10 commits into from
Apr 18, 2024

Conversation

mlch911
Copy link
Contributor

@mlch911 mlch911 commented Apr 15, 2024

📜 Description

Fix crash when call SentryUIApplication in background thread. #3836

💡 Motivation and Context

💚 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

@brustolin
Copy link
Contributor

Hello @mlch911, thanks for the help.
Since the problem occurs only during initialization, and, the responsibility to call every other SentryUIApplication function from the main thread lies with the caller, I believe we should only change the init function.

I need to check tho what is triggering the initialization. because just pushing it to a background thread may not be enough.

@mlch911
Copy link
Contributor Author

mlch911 commented Apr 15, 2024

Hello @mlch911, thanks for the help. Since the problem occurs only during initialization, and, the responsibility to call every other SentryUIApplication function from the main thread lies with the caller, I believe we should only change the init function.

I need to check tho what is triggering the initialization. because just pushing it to a background thread may not be enough.

This is not only occurs during initialization. Our app encounter this crash rarely when the network monitoring occurs at background thread just after we init sentry in main thread.

Here is the crash stack.

-[MaxWindow initWithFrame:] (MaxWindow.m:24)
-[AppDelegate window] (AppDelegate.m:184)
-[SentryUIApplication windows] (SentryUIApplication.m:84)
-[SentryCrashWrapper enrichScope:] (SentryCrashWrapper.m:187)
-[SentryHub initWithClient:andScope:] (SentryHub.m:80)
+[SentrySDK currentHub] (SentrySDK.m:76)
-[SentryRequestOperation initWithSession:request:completionHandler:]_block_invoke (SentryRequestOperation.m:37)

My code fixes crash in this case.

There may be other scenarios that can cause similar crashes. More sophisticated consideration may be needed.

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 84.09091% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 90.608%. Comparing base (eb077dd) to head (8269d26).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3855       +/-   ##
=============================================
- Coverage   90.672%   90.608%   -0.064%     
=============================================
  Files          579       579               
  Lines        45283     45295       +12     
  Branches     16121     16118        -3     
=============================================
- Hits         41059     41041       -18     
- Misses        4044      4184      +140     
+ Partials       180        70      -110     
Files Coverage Δ
Sources/Sentry/SentryUIApplication.m 45.299% <100.000%> (+0.951%) ⬆️
Sources/Sentry/SentryDispatchQueueWrapper.m 86.274% <50.000%> (-13.726%) ⬇️

... and 32 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 eb077dd...8269d26. Read the comment docs.

@brustolin brustolin enabled auto-merge (squash) April 18, 2024 15:18
@brustolin brustolin merged commit 953e914 into getsentry:main Apr 18, 2024
62 of 67 checks passed
armcknight pushed a commit that referenced this pull request Apr 18, 2024
Fix crash when call SentryUIApplication in background thread. #3836


Co-authored-by: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
…try#3855)

Fix crash when call SentryUIApplication in background thread. getsentry#3836


Co-authored-by: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
…try#3855)

Fix crash when call SentryUIApplication in background thread. getsentry#3836


Co-authored-by: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
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.

App crash due to a background call to -[UIApplication applicationState] in -[SentryUIApplication init]
3 participants