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

feat: add method unswizzling #4647

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: add method unswizzling #4647

wants to merge 4 commits into from

Conversation

philprime
Copy link
Contributor

@philprime philprime commented Dec 19, 2024

📜 Description

I adapted the method swizzling helper to keep a reference to the original implementation, so that the unswizzle methods can revert swizzling.

💡 Motivation and Context

We are introducing a new experimental flag in #4634 to conditionally swizzle Objective-C methods. Therefore some unit tests (e.g testFoo) swizzle methods, while others (e.g. testBar) do not. If we do not offer a functionality to unswizzle methods, they are leaking isolation, i.e. testFoo runs before testBar therefore methods are swizzled, even tough we don't want that.

This is required for #4634 to pass tests.

💚 How did you test it?

I added a unit test to test the unswizzling.

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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

  • Change unswizzling to be wrapped by #if TEST to not be used in production

Copy link

🚨 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:

  • Sources/Sentry/SentrySwizzle.m

Copy link

github-actions bot commented Dec 19, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 4b2a0d5

Copy link

🚨 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:

  • Sources/Sentry/SentrySwizzle.m

@philprime philprime marked this pull request as ready for review December 19, 2024 09:59
@philprime philprime self-assigned this Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 94.93671% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.633%. Comparing base (0cb72fd) to head (4b2a0d5).

Files with missing lines Patch % Lines
Sources/Sentry/SentrySwizzle.m 93.548% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4647       +/-   ##
=============================================
+ Coverage   90.584%   90.633%   +0.048%     
=============================================
  Files          621       621               
  Lines        71120     71201       +81     
  Branches     25306     25936      +630     
=============================================
+ Hits         64424     64532      +108     
+ Misses        6602      6573       -29     
- Partials        94        96        +2     
Files with missing lines Coverage Δ
...ources/Sentry/include/HybridPublic/SentrySwizzle.h 97.142% <ø> (ø)
Tests/SentryTests/Helper/SentrySwizzleTests.m 92.417% <100.000%> (+0.664%) ⬆️
Sources/Sentry/SentrySwizzle.m 94.193% <93.548%> (-0.488%) ⬇️

... and 26 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 0cb72fd...4b2a0d5. Read the comment docs.

Copy link

github-actions bot commented Dec 19, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1234.60 ms 1254.16 ms 19.56 ms
Size 22.32 KiB 761.25 KiB 738.94 KiB

Baseline results on branch: main

Startup times

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/unswizzling

Startup times

Revision Plain With Sentry Diff
1f239e8 1233.94 ms 1253.76 ms 19.82 ms

App size

Revision Plain With Sentry Diff
1f239e8 22.30 KiB 760.63 KiB 738.32 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

FYI, we already tried that in the past and it always blew up the tests. We instead chose a different approach: We don't unswizzle, but we make sure the operations are NoOps when closing the SDK. I mean we can try again, but we need some good reasons. What drove you to open this PR?

@philprime
Copy link
Contributor Author

@philipphofmann in #4634 I added unit tests initializing the SentrySDK with an experimental feature flag enabled or disabled. If the SentrySDK is enabled, and swizzling happens, the swizzling remains even after closing and re-enabling the SentrySDK without swizzling.

We could also change this PR to only unswizzle in unit tests, i.e. using #if TEST


[SentryTestsLog clear];

[SentrySwizzle unswizzleInstanceMethod:methodForUnswizzling
Copy link
Member

@armcknight armcknight Dec 19, 2024

Choose a reason for hiding this comment

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

What happens if something swizzles after us, and then we call this method? Do we break the call chain? I'd be interested to see a test case for that.

  • some object A has a method foo
  • swizzle foo and replace it with bar, which calls through to foo
  • swizzle foo again and replace it with baz, which calls through to bar
  • run unswizzle. does that replace baz with foo? then whatever was relying on baz is broken

If this is the case, then this is risky to ship to prod, I'd agree with @philipphofmann that at the most this should only live in test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

I will have to further look into that and add handling for cascading swizzling. But as the main point of this PR is isolation of unit tests, I would focus on fixing that.

@brustolin
Copy link
Contributor

We could also change this PR to only unswizzle in unit tests, i.e. using #if TEST

I agree with this!!

Copy link

🚨 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:

  • Sources/Sentry/SentrySwizzle.m

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.

4 participants