Skip to content

fix: Swizzling crash on iOS 13 #1297

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

Merged
merged 1 commit into from
Aug 31, 2021
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Aug 31, 2021

📜 Description

iOS 13 doesn't have a method for HTTPAdditionalHeaders. Instead, it only
has a property. Therefore, the swizzling crashes on iOS 13. This is fixed now
by checking if NSURLSessionConfiguration has a method HTTPAdditionalHeaders.
The downside is that we don't add a trace header on iOS 13 anymore. This
is better than crashing though.

I will add a test matrix to run the tests on older iOS versions soon.

💡 Motivation and Context

Fixes GH-1296

💚 How did you test it?

Running the tests on an iOS 13 simulator.

📝 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

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
philipphofmann Philipp Hofmann
iOS 13 doesn't have a method for HTTPAdditionalHeaders. Instead, it only
has a property. Therefore, the swizzling crashes on iOS 13. This is fixed now
by checking if NSURLSessionConfiguration has a method HTTPAdditionalHeaders.
The downside is that we don't add a trace header on iOS 13 anymore. This
is better than crashing though.

Fixes GH-1296
@philipphofmann philipphofmann requested a review from a team August 31, 2021 06:40
Comment on lines +38 to +39
// iOS 13 doesn't have a method for HTTPAdditionalHeaders. Instead, it only has a property.
// Therefore, we need to make sure that NSURLSessionConfiguration has this method to be able to
Copy link
Contributor

Choose a reason for hiding this comment

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

M: cant we swizzle a property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not right now at the moment. We need to add the functionality or think of a different approach.

@codecov-commenter
Copy link

Codecov Report

Merging #1297 (c52c74b) into master (ae0aed2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1297      +/-   ##
==========================================
+ Coverage   95.59%   95.61%   +0.02%     
==========================================
  Files         149      149              
  Lines        5314     5317       +3     
==========================================
+ Hits         5080     5084       +4     
+ Misses        234      233       -1     
Impacted Files Coverage Δ
Sources/Sentry/SentryNetworkSwizzling.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryTracer.m 98.52% <0.00%> (+0.49%) ⬆️

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 ae0aed2...c52c74b. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7.2.4 app crash on launch on iOS 13.5
3 participants