fix: Add swizzling for NSFileManager.createFileAtPath #4634
Conversation
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4634 +/- ##
=============================================
+ Coverage 90.584% 90.659% +0.074%
=============================================
Files 621 623 +2
Lines 71120 71441 +321
Branches 25306 26003 +697
=============================================
+ Hits 64424 64768 +344
+ Misses 6602 6576 -26
- Partials 94 97 +3
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9a9ffd | 1201.61 ms | 1215.06 ms | 13.45 ms |
| 282cc99 | 1238.27 ms | 1253.46 ms | 15.19 ms |
| 984eb2d | 1220.62 ms | 1235.24 ms | 14.62 ms |
| 4350d44 | 1228.75 ms | 1246.75 ms | 18.00 ms |
| 279841c | 1237.29 ms | 1254.12 ms | 16.83 ms |
| e145ca1 | 1207.19 ms | 1230.67 ms | 23.48 ms |
| 9b9f2a0 | 1233.36 ms | 1244.04 ms | 10.68 ms |
| 7b022df | 1220.53 ms | 1227.56 ms | 7.03 ms |
| aa225e2 | 1218.00 ms | 1236.45 ms | 18.45 ms |
| 9f0d9e0 | 1226.31 ms | 1242.70 ms | 16.40 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9a9ffd | 21.58 KiB | 418.43 KiB | 396.85 KiB |
| 282cc99 | 22.85 KiB | 414.09 KiB | 391.24 KiB |
| 984eb2d | 20.76 KiB | 425.77 KiB | 405.01 KiB |
| 4350d44 | 21.58 KiB | 629.82 KiB | 608.24 KiB |
| 279841c | 22.84 KiB | 403.19 KiB | 380.34 KiB |
| e145ca1 | 21.58 KiB | 669.70 KiB | 648.12 KiB |
| 9b9f2a0 | 21.58 KiB | 707.42 KiB | 685.84 KiB |
| 7b022df | 22.85 KiB | 412.95 KiB | 390.10 KiB |
| aa225e2 | 21.58 KiB | 694.47 KiB | 672.89 KiB |
| 9f0d9e0 | 21.58 KiB | 424.28 KiB | 402.70 KiB |
Previous results on branch: philprime/nsfilemanager-swizzling
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0274a46 | 1243.38 ms | 1262.28 ms | 18.89 ms |
| ded7094 | 1241.53 ms | 1260.89 ms | 19.36 ms |
| 6285002 | 1232.50 ms | 1246.02 ms | 13.52 ms |
| b6e09ac | 1241.63 ms | 1263.66 ms | 22.03 ms |
| c035d04 | 1234.16 ms | 1250.73 ms | 16.57 ms |
| ea7187f | 1243.06 ms | 1253.69 ms | 10.63 ms |
| e474b38 | 1232.46 ms | 1246.29 ms | 13.83 ms |
| be0667c | 1238.42 ms | 1266.65 ms | 28.24 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0274a46 | 22.30 KiB | 761.98 KiB | 739.68 KiB |
| ded7094 | 22.31 KiB | 760.40 KiB | 738.09 KiB |
| 6285002 | 22.31 KiB | 758.89 KiB | 736.59 KiB |
| b6e09ac | 22.31 KiB | 758.58 KiB | 736.28 KiB |
| c035d04 | 22.31 KiB | 761.99 KiB | 739.68 KiB |
| ea7187f | 22.30 KiB | 762.61 KiB | 740.31 KiB |
| e474b38 | 22.31 KiB | 760.40 KiB | 738.10 KiB |
| be0667c | 22.31 KiB | 758.05 KiB | 735.74 KiB |
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
philprime
requested review from
philipphofmann,
brustolin and
armcknight
as code owners
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
|
So right now after adding the unit tests for enabling and disabling the swizzling via the experimental feature flag, the tests in I currently believe we need to unswizzle methods in the |
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
That sounds like side effects which you won't be able to solve with unswizzling. Let's have a chat about this, as this sounds like something that will bite us later. |
|
@philipphofmann I already narrowed the issue down to the swizzling. I went ahead and created a local Xcode scheme only running tests in these files:
Without unswizzling, if I run the tests individually, they all pass. If I run all the tests, they fail. I added breakpoints and manually tested, and in test cases where swizzling should not happen, the methods were still swizzled from previous tests. Maybe the whole unswizzling/uninstall should only be implemented in the tests, but that's probably better to be discussed in #4647 |
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
📜 Description
This PR fixes the automatic tracking of
file.writespans when usingNSFileManager. createFileAtPath.The changes are split of from #4605.
💡 Motivation and Context
Before macOS 15, iOS 18 and tvOS 18 the implementation would use the already swizzled methods of
NSData. As the internal implementation has changed, theNSFileManagermust be swizzled directly.See #4546 for full discussion.
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.🔮 Next steps