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: ARC issue for FileManager #2525

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Conversation

philipphofmann
Copy link
Member

📜 Description

The SentryFileManager kept a strong reference to self for running code 10 seconds delayed. The strong reference stops ARC from deallocating the SentryFileManager. This is fixed now by using a weak reference.

💡 Motivation and Context

Came across this cause tests are sometimes crashing with malloc: double free for ptr
The raw logs show logs from SentryFileManager

Test Case '-[SentryTests.SentryProfilerSwiftTests testStartTransaction_NotSamplingProfileUsingSampleRate]' started.
2022-12-13 09:52:30.402191+0000 xctest[5072:27885] [Sentry] [error] [SentryFileManager:215] Couldn't load files in folder /Users/runner/Library/Caches/io.sentry/envelopes/envelopes: Error Domain=NSCocoaErrorDomain Code=260 "The folder “envelopes” doesn’t exist." UserInfo={NSFilePath=/Users/runner/Library/Caches/io.sentry/envelopes/envelopes, NSUserStringVariant=(
    Folder
), NSUnderlyingError=0x7fa66ede8560 {Error Domain=NSOSStatusErrorDomain Code=-43 "fnfErr: File not found"}}
2022-12-13 09:52:30.402564+0000 xctest[5072:27885] [Sentry] [debug] [SentryFileManager:83] Dispatched deletion of old envelopes from <SentryFileManager: 0x7fa66de76c40>
2022-12-13 09:52:30.402646+0000 xctest[5072:27101] [Sentry] [debug] [SentryFileManager:643] SentryFileManager.cachePath: /Users/runner/Library/Caches
2022-12-13 09:52:30.402763+0000 xctest[5072:27101] [Sentry] [debug] [SentryFileManager:226] Deleting /Users/runner/Library/Caches/io.sentry/f7bb27206055cbb77c5019791680370519b4854a/events
2022-12-13 09:52:30.405494+0000 xctest[5072:27101] [Sentry] [debug] [SentryHttpTransport:244] sendAllCachedEnvelopes start.
2022-12-13 09:52:30.406054+0000 xctest[5072:27101] [Sentry] [debug] [SentryHttpTransport:256] No envelopes left to send.
2022-12-13 09:52:30.406261+0000 xctest[5072:27101] [Sentry] [debug] [SentryHttpTransport:323] Finished sending.
2022-12-13 09:52:30.408322+0000 xctest[5072:27101] [Sentry] [debug] [SentrySpanContext:56] Created span context with trace ID ba6bd2f9c4ae44f7953656c5e2e2d7c7; span ID e9a3e562d3a04fd2; parent span ID (null); operation Some Operation
2022-12-13 09:52:30.408475+0000 xctest[5072:27101] [Sentry] [debug] [SentryTransactionContext:140] Created transaction context with name Some Transaction
2022-12-13 09:52:30.408624+0000 xctest[5072:27101] [Sentry] [debug] [SentrySpanContext:56] Created span context with trace ID ba6bd2f9c4ae44f7953656c5e2e2d7c7; span ID e9a3e562d3a04fd2; parent span ID (null); operation Some Operation
2022-12-13 09:52:30.408689+0000 xctest[5072:27885] [Sentry] [error] [SentryFileManager:215] Couldn't load files in folder /Users/runner/Library/Caches/io.sentry/envelopes/envelopes: Error Domain=NSCocoaErrorDomain Code=260 "The folder “envelopes” doesn’t exist." UserInfo={NSFilePath=/Users/runner/Library/Caches/io.sentry/envelopes/envelopes, NSUserStringVariant=(
    Folder
), NSUnderlyingError=0x7fa689d7b5b0 {Error Domain=NSOSStatusErrorDomain Code=-43 "fnfErr: File not found"}}
2022-12-13 09:52:30.409531+0000 xctest[5072:27101] [Sentry] [debug] [SentryTracer:150] Starting transaction ID ba6bd2f9c4ae44f7953656c5e2e2d7c7 and name Some Transaction for span ID e9a3e562d3a04fd2 at system time 1288316001947
2022-12-13 09:52:30.409724+0000 xctest[5072:27101] [Sentry] [debug] [SentrySpan:30] Created span e9a3e562d3a04fd2 for trace ID (null)
2022-12-13 09:52:30.409814+0000 xctest[5072:27885] [Sentry] [debug] [SentryFileManager:83] Dispatched deletion of old envelopes from <SentryFileManager: 0x7fa66beb18c0>
2022-12-13 09:52:30.417099+0000 xctest[5072:27885] [Sentry] [error] [SentryFileManager:215] Couldn't load files in folder /Users/runner/Library/Caches/io.sentry/envelopes/envelopes: Error Domain=NSCocoaErrorDomain Code=260 "The folder “envelopes” doesn’t exist." UserInfo={NSFilePath=/Users/runner/Library/Caches/io.sentry/envelopes/envelopes, NSUserStringVariant=(
    Folder
), NSUnderlyingError=0x7fa66eb99780 {Error Domain=NSOSStatusErrorDomain Code=-43 "fnfErr: File not found"}}
2022-12-13 09:52:30.663047+0000 xctest[5072:28237] [Sentry] [debug] [SentryFileManager:83] Dispatched deletion of old envelopes from <SentryFileManager: 0x7fa66de876b0>
2022-12-13 09:52:30.663047+0000 xctest[5072:27885] [Sentry] [debug] [SentryFileManager:83] Dispatched deletion of old envelopes from <SentryFileManager: 0x7fa66bffb230>
2022-12-13 09:52:30.663047+0000 xctest[5072:27372] [Sentry] [debug] [SentryFileManager:83] Dispatched deletion of old envelopes from <SentryFileManager: 0x7fa66becb510>
xctest(5072,0x7000015f6000) malloc: double free for ptr 0x7fa67b0d9800
xctest(5072,0x7000015f6000) malloc: *** set a breakpoint in malloc_error_break to debug

Restarting after unexpected exit, crash, or test timeout in SentryProfilerSwiftTests.testStartTransaction_NotSamplingProfileUsingSampleRate(); summary will include totals from previous launches.

💚 How did you test it?

Unit tests.

📝 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 SentryFileManager kept a strong reference to self for running code 10
seconds delayed. The strong reference stops ARC from deallocating the
SentryFileManager. This is fixed now by using a weak reference.
@github-actions
Copy link

github-actions bot commented Dec 13, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1218.92 ms 1248.50 ms 29.58 ms
Size 20.75 KiB 405.23 KiB 384.48 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
838ad6b 1217.78 ms 1252.10 ms 34.32 ms
a064b3e 1197.47 ms 1220.58 ms 23.11 ms
58ec104 1244.29 ms 1269.67 ms 25.39 ms
f8045b6 1206.76 ms 1233.41 ms 26.65 ms
ae5ecd1 1226.73 ms 1250.96 ms 24.23 ms
2ae7db9 1231.37 ms 1239.98 ms 8.61 ms
dafc0fa 1254.41 ms 1274.31 ms 19.90 ms
9f8d429 1239.57 ms 1255.12 ms 15.55 ms
3fdb749 1227.42 ms 1248.48 ms 21.06 ms
459ae5e 1223.84 ms 1250.14 ms 26.31 ms

App size

Revision Plain With Sentry Diff
838ad6b 20.75 KiB 405.10 KiB 384.34 KiB
a064b3e 20.75 KiB 404.83 KiB 384.08 KiB
58ec104 20.75 KiB 379.11 KiB 358.36 KiB
f8045b6 20.75 KiB 404.83 KiB 384.08 KiB
ae5ecd1 20.75 KiB 383.31 KiB 362.55 KiB
2ae7db9 20.75 KiB 381.87 KiB 361.12 KiB
dafc0fa 20.75 KiB 383.87 KiB 363.12 KiB
9f8d429 20.75 KiB 383.40 KiB 362.65 KiB
3fdb749 20.75 KiB 383.81 KiB 363.06 KiB
459ae5e 20.75 KiB 383.50 KiB 362.75 KiB

Previous results on branch: fix/weak-ref-file-manager

Startup times

Revision Plain With Sentry Diff
94a687a 1233.08 ms 1242.82 ms 9.74 ms

App size

Revision Plain With Sentry Diff
94a687a 20.75 KiB 405.20 KiB 384.44 KiB

Copy link
Contributor

@kevinrenskers kevinrenskers left a comment

Choose a reason for hiding this comment

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

Nice one 👍

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #2525 (4cb8589) into 8.0.0 (a8ac44f) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2525      +/-   ##
==========================================
+ Coverage   78.37%   78.54%   +0.16%     
==========================================
  Files         241      241              
  Lines       22258    22293      +35     
  Branches     9821     9847      +26     
==========================================
+ Hits        17444    17509      +65     
+ Misses       4363     4336      -27     
+ Partials      451      448       -3     
Impacted Files Coverage Δ
Sources/Sentry/SentryFileManager.m 95.00% <100.00%> (-0.59%) ⬇️
Sources/Sentry/SentryRequestOperation.m 67.56% <0.00%> (-10.82%) ⬇️
...ryCrash/Recording/Tools/SentryCrashDynamicLinker.c 79.77% <0.00%> (-0.08%) ⬇️
Sources/Sentry/NSDictionary+SentrySanitize.m 100.00% <0.00%> (ø)
...yCrash/Recording/Tools/SentryCrashUUIDConversion.c 100.00% <0.00%> (ø)
Sources/Sentry/SentryBacktrace.cpp 90.17% <0.00%> (+1.78%) ⬆️
Sources/Sentry/SentryThreadInspector.m 97.93% <0.00%> (+2.06%) ⬆️
Sources/Sentry/SentryThreadHandle.cpp 72.67% <0.00%> (+3.27%) ⬆️
Sources/Sentry/SentryNetworkTracker.m 94.48% <0.00%> (+4.06%) ⬆️
... and 3 more

Continue to review full report at Codecov.

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

@philipphofmann philipphofmann merged commit bd7cfa8 into 8.0.0 Dec 14, 2022
@philipphofmann philipphofmann deleted the fix/weak-ref-file-manager branch December 14, 2022 09:56
kevinrenskers added a commit that referenced this pull request Dec 16, 2022
* 8.0.0: (31 commits)
  tests: Reenable testAddAndRemoveData (#2533)
  feat: support SENTRY_DSN environment var on macOS (#2534)
  Remove duplicate entry (#2532)
  fix: ARC issue for FileManager (#2525)
  feat: Add SwiftUI performance tracking (#2271)
  fix: Remove all permission checks (#2529)
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)
  test: Fix disabled failing watchdog test (#2521)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#2516)
  Rename the watchdog option and integration (#2513)
  feat: Enable CaptureFailedRequests by default (#2507)
  test: Fix asserts for SentryCrashTestInstallation (#2500)
  meta: User interaction tracing enabled per default (#2506)
  ref: Rename OOM to Watchdog Terminations (#2499)
  feat: enableUserInteractionTracing is GA (#2503)
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  ...
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.

3 participants