Skip to content

Commit da27549

Browse files
committed
review-comment: Simplify NSURLSession swizzling and add assumption tests
- remove run-time discovery for swizzle targets; directly swizzle [NSURLSession class] - add unit test to do run-time discovery and report if assumptions invalid - add test to validate that dataTaskWithURL: and dataTaskWithRequest: are independent (both can be swizzled w/out calling the other) - Extract SentryWrapCompletionHandler to reduce duplication
1 parent b67dbab commit da27549

5 files changed

Lines changed: 194 additions & 146 deletions

File tree

Sources/Sentry/SentryNSURLSessionSwizzleTargetDiscovery.m

Lines changed: 0 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -70,65 +70,4 @@ @implementation SentryNSURLSessionSwizzleTargetDiscovery
7070
return [result copy];
7171
}
7272

73-
/**
74-
* Discovers which NSURLSession classes need swizzling for completion handler methods.
75-
* Uses the same proven discovery pattern as urlSessionTaskClassesToTrack but for session methods.
76-
*/
77-
+ (NSArray<Class> *)urlSessionCompletionHandlerClassesToSwizzle:(SEL)selector
78-
{
79-
NSMutableArray<Class> *classesToSwizzle = [NSMutableArray array];
80-
81-
Class baseClass = [NSURLSession class];
82-
if (class_getInstanceMethod(baseClass, selector)) {
83-
[classesToSwizzle addObject:baseClass];
84-
}
85-
86-
NSArray *configs = @[
87-
[NSURLSessionConfiguration defaultSessionConfiguration],
88-
[NSURLSessionConfiguration ephemeralSessionConfiguration]
89-
// backgroundSessionConfiguration not relevant for session replay
90-
];
91-
92-
for (NSURLSessionConfiguration *config in configs) {
93-
NSURLSession *session = [NSURLSession sessionWithConfiguration:config];
94-
Class sessionClass = [session class];
95-
96-
if (sessionClass == baseClass) {
97-
[session invalidateAndCancel];
98-
continue;
99-
}
100-
101-
if ([self classHasUniqueImplementation:sessionClass selector:selector]) {
102-
[classesToSwizzle addObject:sessionClass];
103-
}
104-
105-
[session invalidateAndCancel];
106-
}
107-
108-
return [classesToSwizzle copy];
109-
}
110-
111-
/**
112-
* Checks if a class provides its own implementation of a method (not just inheriting).
113-
*/
114-
+ (BOOL)classHasUniqueImplementation:(Class)cls selector:(SEL)selector
115-
{
116-
Method method = class_getInstanceMethod(cls, selector);
117-
if (!method)
118-
return NO;
119-
120-
Class superClass = [cls superclass];
121-
if (!superClass)
122-
return YES;
123-
124-
Method superMethod = class_getInstanceMethod(superClass, selector);
125-
if (!superMethod)
126-
return YES;
127-
128-
IMP classIMP = method_getImplementation(method);
129-
IMP superIMP = method_getImplementation(superMethod);
130-
131-
return classIMP != superIMP;
132-
}
133-
13473
@end

Sources/Sentry/SentryNetworkTracker.m

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ - (void)captureResponseDetails:(NSData *)data
568568
task:(NSURLSessionTask *)task
569569
{
570570
// TODO: Implementation
571-
// 2. Create SentryNetworkBody from response data
571+
// 2. Parse response body data
572572
// 3. Store in appropriate location for session replay
573573
// 4. Handle size limits and truncation if needed
574574
}

Sources/Sentry/SentrySwizzleWrapperHelper.m

Lines changed: 51 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#import "SentrySwizzleWrapperHelper.h"
2-
#import "SentryLogC.h"
32
#import "SentryNSURLSessionSwizzleTargetDiscovery.h"
43
#import "SentryNetworkTracker.h"
54
#import "SentrySwizzle.h"
@@ -99,74 +98,68 @@ + (void)swizzleURLSessionTask:(SentryNetworkTracker *)networkTracker
9998
#pragma clang diagnostic pop
10099
}
101100

101+
/**
102+
* Wraps a completion handler to capture response details for session replay.
103+
* Returns nil if the original handler is nil (no wrapping needed).
104+
*/
105+
static void (^SentryWrapCompletionHandler(SentryNetworkTracker *tracker, NSURLRequest *request,
106+
NSMutableArray<NSURLSessionDataTask *> *taskHolder,
107+
void (^original)(NSData *, NSURLResponse *, NSError *)))(NSData *, NSURLResponse *, NSError *)
108+
{
109+
if (!original)
110+
return nil;
111+
return ^(NSData *data, NSURLResponse *response, NSError *error) {
112+
NSURLSessionDataTask *task = taskHolder.firstObject;
113+
if (!error && data && task) {
114+
[tracker captureResponseDetails:data response:response request:request task:task];
115+
}
116+
original(data, response, error);
117+
};
118+
}
119+
102120
/**
103121
* Swizzles NSURLSession data task creation methods that use completion handlers
104122
* to enable request/response body capture for session replay.
123+
*
124+
* Both dataTaskWithRequest: and dataTaskWithURL: are independent implementations
125+
* (neither calls through to the other), so both need swizzling.
126+
*
127+
* See SentryNSURLSessionSwizzleTargetDiscoveryTests that verifies these assumptions still hold.
105128
*/
106129
+ (void)swizzleURLSessionDataTasksForResponseCapture:(SentryNetworkTracker *)networkTracker
107130
{
108131
#pragma clang diagnostic push
109132
#pragma clang diagnostic ignored "-Wshadow"
110133
SEL dataTaskWithRequestSelector = @selector(dataTaskWithRequest:completionHandler:);
111-
NSArray<Class> *requestClasses = [SentryNSURLSessionSwizzleTargetDiscovery
112-
urlSessionCompletionHandlerClassesToSwizzle:dataTaskWithRequestSelector];
113-
for (Class sessionClass in requestClasses) {
114-
SentrySwizzleInstanceMethod(sessionClass, dataTaskWithRequestSelector,
115-
SentrySWReturnType(NSURLSessionDataTask *),
116-
SentrySWArguments(NSURLRequest * request,
117-
void (^completionHandler)(NSData *, NSURLResponse *, NSError *)),
118-
SentrySWReplacement({
119-
__block NSURLSessionDataTask *task = nil;
120-
121-
void (^wrappedHandler)(NSData *, NSURLResponse *, NSError *) = nil;
122-
if (completionHandler) {
123-
wrappedHandler = ^(NSData *data, NSURLResponse *response, NSError *error) {
124-
if (!error && data && task) {
125-
[networkTracker captureResponseDetails:data
126-
response:response
127-
request:request
128-
task:task];
129-
}
130-
completionHandler(data, response, error);
131-
};
132-
}
133-
134-
task = SentrySWCallOriginal(request, wrappedHandler ?: completionHandler);
135-
return task;
136-
}),
137-
SentrySwizzleModeOncePerClassAndSuperclasses, (void *)dataTaskWithRequestSelector);
138-
}
134+
SentrySwizzleInstanceMethod([NSURLSession class], dataTaskWithRequestSelector,
135+
SentrySWReturnType(NSURLSessionDataTask *),
136+
SentrySWArguments(NSURLRequest * request,
137+
void (^completionHandler)(NSData *, NSURLResponse *, NSError *)),
138+
SentrySWReplacement({
139+
NSMutableArray<NSURLSessionDataTask *> *taskHolder =
140+
[NSMutableArray arrayWithCapacity:1];
141+
NSURLSessionDataTask *task = SentrySWCallOriginal(request,
142+
SentryWrapCompletionHandler(networkTracker, request, taskHolder, completionHandler));
143+
[taskHolder addObject:task];
144+
return task;
145+
}),
146+
SentrySwizzleModeOncePerClassAndSuperclasses, (void *)dataTaskWithRequestSelector);
139147

140148
SEL dataTaskWithURLSelector = @selector(dataTaskWithURL:completionHandler:);
141-
NSArray<Class> *urlClasses = [SentryNSURLSessionSwizzleTargetDiscovery
142-
urlSessionCompletionHandlerClassesToSwizzle:dataTaskWithURLSelector];
143-
for (Class sessionClass in urlClasses) {
144-
SentrySwizzleInstanceMethod(sessionClass, dataTaskWithURLSelector,
145-
SentrySWReturnType(NSURLSessionDataTask *),
146-
SentrySWArguments(
147-
NSURL * url, void (^completionHandler)(NSData *, NSURLResponse *, NSError *)),
148-
SentrySWReplacement({
149-
NSURLRequest *request = [NSURLRequest requestWithURL:url];
150-
__block NSURLSessionDataTask *task = nil;
151-
152-
void (^wrappedHandler)(NSData *, NSURLResponse *, NSError *) = nil;
153-
if (completionHandler) {
154-
wrappedHandler = ^(NSData *data, NSURLResponse *response, NSError *error) {
155-
if (!error && data && task) {
156-
[networkTracker captureResponseDetails:data
157-
response:response
158-
request:request
159-
task:task];
160-
}
161-
completionHandler(data, response, error);
162-
};
163-
}
164-
165-
task = SentrySWCallOriginal(url, wrappedHandler ?: completionHandler);
166-
return task;
167-
}),
168-
SentrySwizzleModeOncePerClassAndSuperclasses, (void *)dataTaskWithURLSelector);
169-
}
149+
SentrySwizzleInstanceMethod([NSURLSession class], dataTaskWithURLSelector,
150+
SentrySWReturnType(NSURLSessionDataTask *),
151+
SentrySWArguments(
152+
NSURL * url, void (^completionHandler)(NSData *, NSURLResponse *, NSError *)),
153+
SentrySWReplacement({
154+
NSURLRequest *request = [NSURLRequest requestWithURL:url];
155+
NSMutableArray<NSURLSessionDataTask *> *taskHolder =
156+
[NSMutableArray arrayWithCapacity:1];
157+
NSURLSessionDataTask *task = SentrySWCallOriginal(url,
158+
SentryWrapCompletionHandler(networkTracker, request, taskHolder, completionHandler));
159+
[taskHolder addObject:task];
160+
return task;
161+
}),
162+
SentrySwizzleModeOncePerClassAndSuperclasses, (void *)dataTaskWithURLSelector);
170163
#pragma clang diagnostic pop
171164
}
172165

Sources/Sentry/include/SentryNSURLSessionSwizzleTargetDiscovery.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@ NS_ASSUME_NONNULL_BEGIN
66

77
+ (NSArray<Class> *)urlSessionTaskClassesToTrack;
88

9-
+ (NSArray<Class> *)urlSessionCompletionHandlerClassesToSwizzle:(SEL)selector;
10-
119
@end
1210

1311
NS_ASSUME_NONNULL_END

Tests/SentryTests/Integrations/Performance/Network/SentryNSURLSessionSwizzleTargetDiscoveryTests.swift

Lines changed: 142 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,36 +3,154 @@ import XCTest
33
// We need to know whether Apple changes the NSURLSessionTask implementation.
44
class SentryNSURLSessionSwizzleTargetDiscoveryTests: XCTestCase {
55

6-
func test_URLSessionTask_ByIosVersion() {
6+
func test_URLSessionTask_ByIosVersion() {
77
let classes = SentryNSURLSessionSwizzleTargetDiscovery.urlSessionTaskClassesToTrack()
8-
8+
99
XCTAssertEqual(classes.count, 1)
1010
XCTAssertTrue(classes.first === URLSessionTask.self)
1111
}
12-
13-
func test_URLSessionCompletionHandler_DataTaskWithRequest() {
14-
let selector = #selector(URLSession.dataTask(with:completionHandler:) as (URLSession) -> (URLRequest, @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask)
15-
assertCompletionHandlerDiscovery(selector: selector, selectorName: "dataTaskWithRequest:completionHandler:")
12+
13+
// MARK: - NSURLSession class hierarchy validation tests
14+
//
15+
// Based on testing, NSURLSession implements dataTaskWithRequest:completionHandler:
16+
// and dataTaskWithURL:completionHandler: directly on the base class.
17+
//
18+
// The swizzling code relies on this by swizzling [NSURLSession class] directly
19+
// rather than doing runtime discovery. These tests verify that assumption
20+
// still holds — if Apple ever moves these methods to a subclass, these tests
21+
// will fail and we'll know to update the swizzling approach.
22+
23+
func test_URLSession_isNotClassCluster_dataTaskWithRequest() {
24+
let selector = #selector(URLSession.dataTask(with:completionHandler:)
25+
as (URLSession) -> (URLRequest, @escaping @Sendable (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask)
26+
assertNSURLSessionImplementsDirectly(selector: selector, selectorName: "dataTaskWithRequest:completionHandler:")
1627
}
17-
18-
func test_URLSessionCompletionHandler_DataTaskWithURL() {
19-
let selector = #selector(URLSession.dataTask(with:completionHandler:) as (URLSession) -> (URL, @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask)
20-
assertCompletionHandlerDiscovery(selector: selector, selectorName: "dataTaskWithURL:completionHandler:")
28+
29+
func test_URLSession_isNotClassCluster_dataTaskWithURL() {
30+
let selector = #selector(URLSession.dataTask(with:completionHandler:)
31+
as (URLSession) -> (URL, @escaping @Sendable (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask)
32+
assertNSURLSessionImplementsDirectly(selector: selector, selectorName: "dataTaskWithURL:completionHandler:")
33+
}
34+
35+
// MARK: - dataTaskWithURL: / dataTaskWithRequest: independence
36+
//
37+
// We swizzle both dataTaskWithRequest:completionHandler: and
38+
// dataTaskWithURL:completionHandler: because they are independent
39+
// implementations — dataTaskWithURL: does NOT dispatch to
40+
// dataTaskWithRequest: via objc_msgSend.
41+
//
42+
// If this test ever fails, Apple has changed the internal dispatch so
43+
// one calls through to the other. In that case, remove the redundant
44+
// swizzle and add a deduplication guard to avoid double-capture.
45+
46+
func test_dataTaskWithURL_doesNotCallThrough_dataTaskWithRequest() {
47+
assertNoCallThrough(
48+
from: NSSelectorFromString("dataTaskWithURL:completionHandler:"),
49+
to: NSSelectorFromString("dataTaskWithRequest:completionHandler:"),
50+
call: { session in
51+
let url = URL(string: "https://example.com")!
52+
let task = session.dataTask(with: url) { _, _, _ in }
53+
task.cancel()
54+
}
55+
)
2156
}
22-
23-
// MARK: - Helper Methods
24-
25-
private func assertCompletionHandlerDiscovery(selector: Selector, selectorName: String) {
26-
let classes = SentryNSURLSessionSwizzleTargetDiscovery.urlSessionCompletionHandlerClasses(toSwizzle: selector)
27-
28-
// At the time of writing, internal session subclasses don't override these data task factory methods
29-
// If we ever see more than 1 class that isn't URLSession itself - flag it to be checked out.
30-
XCTAssertEqual(classes.count, 1, "Should have exactly URLSession to swizzle for \(selectorName)")
31-
XCTAssertTrue(classes.first === URLSession.self, "Should be URLSession class for \(selectorName)")
32-
33-
// Double-check the method that we need actually exists.
34-
let hasMethod = class_getInstanceMethod(classes.first!, selector) != nil
35-
XCTAssertTrue(hasMethod, "URLSession should have \(selectorName)")
57+
58+
func test_dataTaskWithRequest_doesNotCallThrough_dataTaskWithURL() {
59+
assertNoCallThrough(
60+
from: NSSelectorFromString("dataTaskWithRequest:completionHandler:"),
61+
to: NSSelectorFromString("dataTaskWithURL:completionHandler:"),
62+
call: { session in
63+
let request = URLRequest(url: URL(string: "https://example.com")!)
64+
let task = session.dataTask(with: request) { _, _, _ in }
65+
task.cancel()
66+
}
67+
)
68+
}
69+
70+
/// Temporarily replaces the IMP of `targetSelector` with one that increments
71+
/// a counter, then invokes `call` (which should trigger `sourceSelector`).
72+
/// Asserts the counter stays at 0 — meaning `sourceSelector` does not
73+
/// internally dispatch to `targetSelector` via objc_msgSend.
74+
private func assertNoCallThrough(
75+
from sourceSelector: Selector,
76+
to targetSelector: Selector,
77+
call: (URLSession) -> Void
78+
) {
79+
guard let method = class_getInstanceMethod(URLSession.self, targetSelector) else {
80+
XCTFail("URLSession should implement \(targetSelector)")
81+
return
82+
}
83+
84+
let originalIMP = method_getImplementation(method)
85+
defer { method_setImplementation(method, originalIMP) }
86+
87+
var hitCount = 0
88+
89+
let replacementBlock: @convention(block) (NSObject, AnyObject, Any?) -> AnyObject = { obj, arg, handler in
90+
hitCount += 1
91+
typealias Fn = @convention(c) (NSObject, Selector, AnyObject, Any?) -> AnyObject
92+
let original = unsafeBitCast(originalIMP, to: Fn.self)
93+
return original(obj, targetSelector, arg, handler)
94+
}
95+
96+
method_setImplementation(method, imp_implementationWithBlock(replacementBlock))
97+
98+
let session = URLSession(configuration: .ephemeral)
99+
defer { session.invalidateAndCancel() }
100+
101+
call(session)
102+
103+
XCTAssertEqual(
104+
hitCount, 0,
105+
"\(sourceSelector) called through to \(targetSelector). "
106+
+ "These methods are no longer independent — remove the redundant swizzle "
107+
+ "in SentrySwizzleWrapperHelper and add a deduplication guard."
108+
)
36109
}
37110

111+
// MARK: - Helper
112+
113+
/// Walks the class hierarchy for sessions created with default and ephemeral
114+
/// configurations and asserts that no subclass overrides `selector`.
115+
private func assertNSURLSessionImplementsDirectly(selector: Selector, selectorName: String) {
116+
let baseClass: AnyClass = URLSession.self
117+
118+
// The base class must implement the method.
119+
XCTAssertNotNil(
120+
class_getInstanceMethod(baseClass, selector),
121+
"URLSession should implement \(selectorName)"
122+
)
123+
124+
// Check sessions created with each relevant configuration.
125+
let configs: [URLSessionConfiguration] = [
126+
.default,
127+
.ephemeral
128+
]
129+
130+
for config in configs {
131+
let session = URLSession(configuration: config)
132+
let sessionClass: AnyClass = type(of: session)
133+
134+
defer { session.invalidateAndCancel() }
135+
136+
if sessionClass === baseClass {
137+
continue
138+
}
139+
140+
// If Apple returns a subclass, it must NOT provide its own
141+
// implementation — it should inherit from URLSession.
142+
let subMethod = class_getInstanceMethod(sessionClass, selector)
143+
let baseMethod = class_getInstanceMethod(baseClass, selector)
144+
145+
if let subMethod, let baseMethod {
146+
let subIMP = method_getImplementation(subMethod)
147+
let baseIMP = method_getImplementation(baseMethod)
148+
XCTAssertEqual(
149+
subIMP, baseIMP,
150+
"\(NSStringFromClass(sessionClass)) overrides \(selectorName) with an unexpected IMP — "
151+
+ "Verify swizzling in SentrySwizzleWrapperHelper is correct for dataTasks."
152+
)
153+
}
154+
}
155+
}
38156
}

0 commit comments

Comments
 (0)