fix: Cache binary images to be used for crashes

[brustolin]

📜 Description

During crashes we use 2 async-signal-unsafe functions _dyld_get_image_header and _dyld_get_image_name.
This may lead to a crash report not being properly saved to disk.

This proposal will cache binary images during runtime and use it when the app crashes.

💡 Motivation and Context

Solves #1892

Maybe do #1096 in an extra PR.

💚 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.

#skip-changelog

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1252.04 ms	1253.12 ms	1.08 ms
Size	20.76 KiB	393.33 KiB	372.57 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
156e771	1228.06 ms	1242.64 ms	14.58 ms
17afc4b	1228.94 ms	1251.10 ms	22.16 ms
e84bc3f	1257.08 ms	1267.41 ms	10.33 ms
ecd9ecd	1215.77 ms	1238.70 ms	22.93 ms
1bbcb9c	1214.25 ms	1230.04 ms	15.79 ms
efb2222	1258.22 ms	1276.78 ms	18.56 ms
326b7eb	1252.86 ms	1259.56 ms	6.70 ms
c021422	1237.12 ms	1263.18 ms	26.06 ms
455619d	1231.40 ms	1237.70 ms	6.30 ms
adcc7d8	1225.90 ms	1245.08 ms	19.18 ms

App size

Revision	Plain	With Sentry	Diff
156e771	20.76 KiB	419.70 KiB	398.94 KiB
17afc4b	20.76 KiB	436.25 KiB	415.49 KiB
e84bc3f	20.76 KiB	434.72 KiB	413.96 KiB
ecd9ecd	20.76 KiB	420.23 KiB	399.47 KiB
1bbcb9c	20.76 KiB	426.10 KiB	405.34 KiB
efb2222	20.76 KiB	424.45 KiB	403.69 KiB
326b7eb	20.76 KiB	432.31 KiB	411.55 KiB
c021422	20.76 KiB	435.64 KiB	414.88 KiB
455619d	20.76 KiB	432.87 KiB	412.11 KiB
adcc7d8	20.76 KiB	426.15 KiB	405.39 KiB

Previous results on branch: fix/writeBinaryImage

Startup times

Revision	Plain	With Sentry	Diff
9d73547	1262.36 ms	1264.82 ms	2.46 ms
e6615d7	1232.69 ms	1250.06 ms	17.37 ms
ba398d9	1242.84 ms	1257.18 ms	14.35 ms
34bd45a	1212.22 ms	1228.80 ms	16.58 ms
c8aefaf	1210.42 ms	1241.44 ms	31.02 ms
d3dd6f3	1250.20 ms	1265.80 ms	15.59 ms
488ea5e	1221.65 ms	1226.44 ms	4.79 ms
61c3a9a	1234.14 ms	1246.17 ms	12.02 ms
6035790	1234.37 ms	1245.76 ms	11.39 ms
6dd069b	1212.65 ms	1222.88 ms	10.23 ms

App size

Revision	Plain	With Sentry	Diff
9d73547	20.76 KiB	435.51 KiB	414.75 KiB
e6615d7	20.76 KiB	433.30 KiB	412.54 KiB
ba398d9	20.76 KiB	435.52 KiB	414.76 KiB
34bd45a	20.76 KiB	435.54 KiB	414.78 KiB
c8aefaf	20.76 KiB	435.52 KiB	414.76 KiB
d3dd6f3	20.76 KiB	435.58 KiB	414.82 KiB
488ea5e	20.76 KiB	433.76 KiB	413.00 KiB
61c3a9a	20.76 KiB	435.52 KiB	414.76 KiB
6035790	20.76 KiB	435.85 KiB	415.09 KiB
6dd069b	20.76 KiB	435.58 KiB	414.82 KiB

[armcknight]

Stoked that you're taking this on, nice work!

[philipphofmann]

We should remove sentrycrashdl_imageCount and sentrycrashdl_getBinaryImage to ensure nobody is calling the async unsafe code anymore.

Thanks for tackling this, @brustolin. This is a tricky one.

[philipphofmann]

@brustolin, I think the general approach looks fine, so I guess you can start adding tests now.

I think I found a few bugs in the code, so I wait until you add tests to give you a detailed review.

[codecov]

Codecov Report

Merging #2939 (13faf37) into main (69d8759) will decrease coverage by 0.029%.
The diff coverage is 91.891%.

❗ Current head 13faf37 differs from pull request most recent head 5d7001e. Consider uploading reports for the commit 5d7001e to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #2939       +/-   ##
=============================================
- Coverage   88.827%   88.799%   -0.029%     
=============================================
  Files          493       495        +2     
  Lines        53141     53462      +321     
  Branches     19035     19141      +106     
=============================================
+ Hits         47204     47474      +270     
- Misses        4975      5028       +53     
+ Partials       962       960        -2     

Impacted Files	Coverage Δ
Sources/SentryCrash/Recording/SentryCrashReport.c	30.881% <5.263%> (-3.387%)	⬇️
...ions/SentryCrash/SentryCrashIntegrationTests.swift	83.333% <76.666%> (-0.607%)	⬇️
...entryCrash/Recording/SentryCrashBinaryImageCache.c	97.777% <97.777%> (ø)
Sources/Sentry/SentryCrashIntegration.m	95.092% <100.000%> (+0.060%)	⬆️
Sources/Sentry/SentryCrashWrapper.m	78.431% <100.000%> (+2.875%)	⬆️
...tryTests/SentryCrash/SentryBinaryImageCacheTests.m	100.000% <100.000%> (ø)
...s/SentryTests/SentryCrash/TestSentryCrashWrapper.m	89.830% <100.000%> (+1.595%)	⬆️

... and 10 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 69d8759...5d7001e. Read the comment docs.

[armcknight]

Looks good, some small suggestions but no blockers as far as I can see. Nice work!

[philipphofmann]

@brustolin, great job 🥇 ; I think we are close to merging this. Apart from my comments, here are a couple of things to address, please

• Add a changelog entry
• Remove sentrycrashdl_imageCount, sentrycrashdl_getBinaryImage, SentryCrashDefaultBinaryImageProvider
• I'm a bit worried, that this could blow up for apps having loads of binary images and maybe use a weird setup. How can we minimize the risk of it blowing up?

[brustolin]

Remove sentrycrashdl_imageCount, sentrycrashdl_getBinaryImage, SentryCrashDefaultBinaryImageProvider

I rather do this in another PR. There still code for non crash captures that uses SentryCrashDefaultBinaryImageProvider.

[brustolin]

I'm a bit worried, that this could blow up for apps having loads of binary images and maybe use a weird setup. How can we minimize the risk of it blowing up?

I believe the amount of library loads that would cause a problem is too high to even be feasible in an App.

[philipphofmann]

I'm a bit worried, that this could blow up for apps having loads of binary images and maybe use a weird setup. How can we minimize the risk of it blowing up?

I believe the amount of library loads that would cause a problem is too high to even be feasible in an App.

I think releasing a beta, would be a good strategy. Furthermore, if it blows up, you should notice it immediately after starting the app.

[philipphofmann]

Almost finished. We just need to adapt the tests a bit, and then LGTM.