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: Reporting crashes when restarting the SDK #2440

Merged
merged 42 commits into from
Nov 29, 2022

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Nov 23, 2022

📜 Description

The SDK did not report crashes after restarting the SDK. This is fixed by restoring the SentryCrashMonitors when installing SentryCrash after uninstalling it.

💡 Motivation and Context

Fixes GH-2419

💚 How did you test it?

Manually with the steps explained in GH-2419, and some unit tests to validate the internal state of SentryCrash.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

The SDK did not report crashes after restarting the SDK. This is
fixed by restoring the SentryCrashMonitors when installing
SentryCrash after uninstalling it.

Fixes GH-2419
@github-actions
Copy link

github-actions bot commented Nov 23, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.02 ms 1254.96 ms 24.94 ms
Size 20.75 KiB 374.71 KiB 353.96 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
7d3d1a3 1218.26 ms 1238.94 ms 20.68 ms
b15627c 1228.88 ms 1269.70 ms 40.82 ms
8b040e4 1234.76 ms 1244.71 ms 9.95 ms
4a66f00 1224.73 ms 1241.14 ms 16.41 ms
7ade7ad 1253.31 ms 1267.08 ms 13.77 ms
48c7211 1214.90 ms 1248.93 ms 34.04 ms
0fdf0b2 1249.20 ms 1254.08 ms 4.88 ms
377c78b 1239.63 ms 1261.51 ms 21.88 ms
15a0fa1 1196.66 ms 1243.94 ms 47.28 ms
f5f8910 1222.41 ms 1251.78 ms 29.37 ms

App size

Revision Plain With Sentry Diff
7d3d1a3 20.50 KiB 342.24 KiB 321.73 KiB
b15627c 20.50 KiB 337.76 KiB 317.25 KiB
8b040e4 20.50 KiB 333.54 KiB 313.04 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
7ade7ad 20.50 KiB 365.18 KiB 344.68 KiB
48c7211 20.75 KiB 374.72 KiB 353.97 KiB
0fdf0b2 20.51 KiB 332.90 KiB 312.39 KiB
377c78b 20.50 KiB 337.71 KiB 317.20 KiB
15a0fa1 20.75 KiB 374.72 KiB 353.97 KiB
f5f8910 20.75 KiB 368.04 KiB 347.29 KiB

Previous results on branch: fix/no-crash-reports-restarting-sdk

Startup times

Revision Plain With Sentry Diff
e779524 1241.08 ms 1274.90 ms 33.82 ms
b0a6c22 1232.44 ms 1253.69 ms 21.25 ms
3267f4e 1238.90 ms 1254.87 ms 15.97 ms
b5f4086 1224.86 ms 1258.62 ms 33.76 ms
5a54a95 1227.82 ms 1271.59 ms 43.78 ms
e0221a1 1235.90 ms 1250.36 ms 14.46 ms
8c7eeb1 1210.45 ms 1232.30 ms 21.85 ms
9c8a7ba 1234.28 ms 1248.34 ms 14.06 ms
d7f7b00 1231.73 ms 1254.00 ms 22.27 ms
5761439 1230.29 ms 1244.94 ms 14.65 ms

App size

Revision Plain With Sentry Diff
e779524 20.75 KiB 374.68 KiB 353.93 KiB
b0a6c22 20.75 KiB 375.09 KiB 354.34 KiB
3267f4e 20.75 KiB 374.88 KiB 354.13 KiB
b5f4086 20.75 KiB 374.70 KiB 353.95 KiB
5a54a95 20.75 KiB 374.71 KiB 353.96 KiB
e0221a1 20.75 KiB 375.35 KiB 354.60 KiB
8c7eeb1 20.75 KiB 375.34 KiB 354.58 KiB
9c8a7ba 20.75 KiB 375.21 KiB 354.45 KiB
d7f7b00 20.75 KiB 374.65 KiB 353.90 KiB
5761439 20.75 KiB 375.12 KiB 354.37 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

I looked around and noticed that in sentrycrash_install, g_installed is set to 1. It looks like the case was that on subsequent calls to sentrycrash_install, since g_installed is never set to 0 in any uninstall codepath, it skips reinstallation. Was there a reason we couldn't go this route instead? Are there some non-idempotent operations in sentrycrash_install?

@philipphofmann
Copy link
Member Author

I looked around and noticed that in sentrycrash_install, g_installed is set to 1. It looks like the case was that on subsequent calls to sentrycrash_install, since g_installed is never set to 0 in any uninstall codepath, it skips reinstallation. Was there a reason we couldn't go this route instead? Are there some non-idempotent operations in sentrycrash_install?

Good point, I changed it now to set g_installed = 0; in sentrycrash_uninstall.

@@ -51,6 +51,17 @@
IMPLEMENT_REPORT_VALUE_PROPERTY(NAME, NAMEUPPER, TYPE) \
IMPLEMENT_REPORT_KEY_PROPERTY(NAME, NAMEUPPER)

typedef struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Taken from SentryCrashInstallation.m for testing

@brustolin
Copy link
Contributor

Why so many errors with SentryNetworkTrackerIntegrationTests?

@philipphofmann
Copy link
Member Author

Why so many errors with SentryNetworkTrackerIntegrationTests?

No idea. Need to figure that out.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Looks good, just one more question but nothing blocking 🚢

@philipphofmann philipphofmann merged commit 9ba19c0 into master Nov 29, 2022
@philipphofmann philipphofmann deleted the fix/no-crash-reports-restarting-sdk branch November 29, 2022 14:43
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Reporting crashes when restarting the SDK ([#2440](https://github.com/getsentry/sentry-cocoa/pull/2440))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 61948d6

kevinrenskers added a commit that referenced this pull request Nov 29, 2022
* 8.0.0:
  Merge branch 'master' into 8.0.0
  release: 7.31.3
  revert rename of sentryuser (#2462)
  fix: Reporting crashes when restarting the SDK (#2440)
  Update integration-tests.yml (#2461)
  fix: Core data span status with error (#2439)
  meta: disable swiftlint file length check in TBDBClient.swift (#2435)
  Revert "test: shorten some tests (#2422)" (#2427)
  test: shorten some tests (#2422)

# Conflicts:
#	Tests/SentryTests/Helper/TestNSNotificationCenterWrapper.swift
@@ -1,19 +1,27 @@
import Foundation

class TestNSNotificationCenterWrapper: SentryNSNotificationCenterWrapper {
@objc public class TestNSNotificationCenterWrapper: SentryNSNotificationCenterWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for these @objc public additions?

Copy link
Member Author

Choose a reason for hiding this comment

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

So I can pass TestNSNotificationCenterWrapper to an ObjC class.

kevinrenskers added a commit that referenced this pull request Nov 29, 2022
…ERSIZE

* 8.0.0:
  fix: SentryAppStateManager should always stop when closing the SDK (#2460)
  Merge branch 'master' into 8.0.0
  release: 7.31.3
  fix: Reporting crashes when restarting the SDK (#2440)
  Update integration-tests.yml (#2461)
  fix: Core data span status with error (#2439)
  meta: disable swiftlint file length check in TBDBClient.swift (#2435)
  Revert "test: shorten some tests (#2422)" (#2427)
  test: shorten some tests (#2422)
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.

Crash reports not emitted when client is re-started
4 participants