Skip to content

Commit d944afb

Browse files
maksymmalyhinmikehaney24
maksymmalyhin
and
mikehaney24
authored
Don't leak upload packages or deadlock on lifecycle events (#5382) (#5384)
* Don't leak upload packages or deadlock on lifecycle events GDTCORUploadPackages would call targetToStorage in -initWithTarget. When this method was running during app terminate, this would cause a deadlock because calling targetToStorage is re-entrant while GDTCORRegistrar is sending lifecycle events to all the instances it's tracking. NSTimer's invalidate method is also called whenever a package is completed, retried, or expired, without regard to the presence of a package handler. A unit test is added to ensure that GDTCORUploadPackage's are not leaked by a retain TSAN caught no issues * Don't mark GDTCORUploadPackage -init as unavailable Co-authored-by: Michael Haney <[email protected]>
1 parent 9815ac9 commit d944afb

File tree

5 files changed

+104
-53
lines changed

5 files changed

+104
-53
lines changed

‎GoogleDataTransport/GDTCORLibrary/GDTCORRegistrar.m

Lines changed: 45 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -122,63 +122,66 @@ - (void)registerPrioritizer:(id<GDTCORPrioritizer>)prioritizer target:(GDTCORTar
122122
#pragma mark - GDTCORLifecycleProtocol
123123

124124
- (void)appWillBackground:(nonnull GDTCORApplication *)app {
125-
dispatch_async(_registrarQueue, ^{
126-
for (id<GDTCORUploader> uploader in [self->_targetToUploader allValues]) {
127-
if ([uploader respondsToSelector:@selector(appWillBackground:)]) {
128-
[uploader appWillBackground:app];
129-
}
125+
NSArray<id<GDTCORUploader>> *uploaders = [self.targetToUploader allValues];
126+
for (id<GDTCORUploader> uploader in uploaders) {
127+
if ([uploader respondsToSelector:@selector(appWillBackground:)]) {
128+
[uploader appWillBackground:app];
130129
}
131-
for (id<GDTCORPrioritizer> prioritizer in [self->_targetToPrioritizer allValues]) {
132-
if ([prioritizer respondsToSelector:@selector(appWillBackground:)]) {
133-
[prioritizer appWillBackground:app];
134-
}
130+
}
131+
NSArray<id<GDTCORPrioritizer>> *prioritizers = [self.targetToPrioritizer allValues];
132+
for (id<GDTCORPrioritizer> prioritizer in prioritizers) {
133+
if ([prioritizer respondsToSelector:@selector(appWillBackground:)]) {
134+
[prioritizer appWillBackground:app];
135135
}
136-
for (id<GDTCORStorageProtocol> storage in [self->_targetToStorage allValues]) {
137-
if ([storage respondsToSelector:@selector(appWillBackground:)]) {
138-
[storage appWillBackground:app];
139-
}
136+
}
137+
NSArray<id<GDTCORStorageProtocol>> *storages = [self.targetToStorage allValues];
138+
for (id<GDTCORStorageProtocol> storage in storages) {
139+
if ([storage respondsToSelector:@selector(appWillBackground:)]) {
140+
[storage appWillBackground:app];
140141
}
141-
});
142+
}
142143
}
143144

144145
- (void)appWillForeground:(nonnull GDTCORApplication *)app {
145-
dispatch_async(_registrarQueue, ^{
146-
for (id<GDTCORUploader> uploader in [self->_targetToUploader allValues]) {
147-
if ([uploader respondsToSelector:@selector(appWillForeground:)]) {
148-
[uploader appWillForeground:app];
149-
}
146+
NSArray<id<GDTCORUploader>> *uploaders = [self.targetToUploader allValues];
147+
for (id<GDTCORUploader> uploader in uploaders) {
148+
if ([uploader respondsToSelector:@selector(appWillForeground:)]) {
149+
[uploader appWillForeground:app];
150150
}
151-
for (id<GDTCORPrioritizer> prioritizer in [self->_targetToPrioritizer allValues]) {
152-
if ([prioritizer respondsToSelector:@selector(appWillForeground:)]) {
153-
[prioritizer appWillForeground:app];
154-
}
151+
}
152+
NSArray<id<GDTCORPrioritizer>> *prioritizers = [self.targetToPrioritizer allValues];
153+
for (id<GDTCORPrioritizer> prioritizer in prioritizers) {
154+
if ([prioritizer respondsToSelector:@selector(appWillForeground:)]) {
155+
[prioritizer appWillForeground:app];
155156
}
156-
for (id<GDTCORStorageProtocol> storage in [self->_targetToStorage allValues]) {
157-
if ([storage respondsToSelector:@selector(appWillForeground:)]) {
158-
[storage appWillForeground:app];
159-
}
157+
}
158+
NSArray<id<GDTCORStorageProtocol>> *storages = [self.targetToStorage allValues];
159+
for (id<GDTCORStorageProtocol> storage in storages) {
160+
if ([storage respondsToSelector:@selector(appWillForeground:)]) {
161+
[storage appWillForeground:app];
160162
}
161-
});
163+
}
162164
}
163165

164166
- (void)appWillTerminate:(nonnull GDTCORApplication *)app {
165-
dispatch_sync(_registrarQueue, ^{
166-
for (id<GDTCORUploader> uploader in [self->_targetToUploader allValues]) {
167-
if ([uploader respondsToSelector:@selector(appWillTerminate:)]) {
168-
[uploader appWillTerminate:app];
169-
}
167+
NSArray<id<GDTCORUploader>> *uploaders = [self.targetToUploader allValues];
168+
for (id<GDTCORUploader> uploader in uploaders) {
169+
if ([uploader respondsToSelector:@selector(appWillTerminate:)]) {
170+
[uploader appWillTerminate:app];
170171
}
171-
for (id<GDTCORPrioritizer> prioritizer in [self->_targetToPrioritizer allValues]) {
172-
if ([prioritizer respondsToSelector:@selector(appWillTerminate:)]) {
173-
[prioritizer appWillTerminate:app];
174-
}
172+
}
173+
NSArray<id<GDTCORPrioritizer>> *prioritizers = [self.targetToPrioritizer allValues];
174+
for (id<GDTCORPrioritizer> prioritizer in prioritizers) {
175+
if ([prioritizer respondsToSelector:@selector(appWillTerminate:)]) {
176+
[prioritizer appWillTerminate:app];
175177
}
176-
for (id<GDTCORStorageProtocol> storage in [self->_targetToStorage allValues]) {
177-
if ([storage respondsToSelector:@selector(appWillTerminate:)]) {
178-
[storage appWillTerminate:app];
179-
}
178+
}
179+
NSArray<id<GDTCORStorageProtocol>> *storages = [self.targetToStorage allValues];
180+
for (id<GDTCORStorageProtocol> storage in storages) {
181+
if ([storage respondsToSelector:@selector(appWillTerminate:)]) {
182+
[storage appWillTerminate:app];
180183
}
181-
});
184+
}
182185
}
183186

184187
@end

‎GoogleDataTransport/GDTCORLibrary/GDTCORUploadPackage.m

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,30 @@
2323
#import "GDTCORLibrary/Private/GDTCORUploadCoordinator.h"
2424
#import "GDTCORLibrary/Private/GDTCORUploadPackage_Private.h"
2525

26+
/** A class that holds a weak reference to an upload package, for use by the package's NSTimer. */
27+
@interface GDTCORUploadPackageTimerHolder : NSObject
28+
29+
/** The upload package. */
30+
@property(weak, nonatomic) GDTCORUploadPackage *package;
31+
32+
@end
33+
34+
@implementation GDTCORUploadPackageTimerHolder
35+
36+
/** Calls checkIfPackageIsExpired on the package if non-nil. Invalidates if the package is nil.
37+
*
38+
* @param timer The timer instance calling this method.
39+
*/
40+
- (void)timerFired:(NSTimer *)timer {
41+
if (_package) {
42+
[_package checkIfPackageIsExpired];
43+
} else {
44+
[timer invalidate];
45+
}
46+
}
47+
48+
@end
49+
2650
@implementation GDTCORUploadPackage {
2751
/** If YES, the package's -completeDelivery method has been called. */
2852
BOOL _isDelivered;
@@ -41,9 +65,11 @@ - (instancetype)initWithTarget:(GDTCORTarget)target {
4165
_storage = [GDTCORRegistrar sharedInstance].targetToStorage[@(target)];
4266
_deliverByTime = [GDTCORClock clockSnapshotInTheFuture:180000];
4367
_handler = [GDTCORUploadCoordinator sharedInstance];
68+
GDTCORUploadPackageTimerHolder *holder = [[GDTCORUploadPackageTimerHolder alloc] init];
69+
holder.package = self;
4470
_expirationTimer = [NSTimer scheduledTimerWithTimeInterval:5.0
45-
target:self
46-
selector:@selector(checkIfPackageIsExpired:)
71+
target:holder
72+
selector:@selector(timerFired:)
4773
userInfo:nil
4874
repeats:YES];
4975
}
@@ -52,7 +78,18 @@ - (instancetype)initWithTarget:(GDTCORTarget)target {
5278
}
5379

5480
- (instancetype)copy {
55-
GDTCORUploadPackage *newPackage = [[GDTCORUploadPackage alloc] initWithTarget:_target];
81+
GDTCORUploadPackage *newPackage = [[GDTCORUploadPackage alloc] init];
82+
newPackage->_target = _target;
83+
newPackage->_storage = _storage;
84+
newPackage->_deliverByTime = _deliverByTime;
85+
newPackage->_handler = _handler;
86+
GDTCORUploadPackageTimerHolder *holder = [[GDTCORUploadPackageTimerHolder alloc] init];
87+
holder.package = newPackage;
88+
newPackage->_expirationTimer = [NSTimer scheduledTimerWithTimeInterval:5.0
89+
target:holder
90+
selector:@selector(timerFired:)
91+
userInfo:nil
92+
repeats:YES];
5693
newPackage->_events = [_events copy];
5794
GDTCORLogDebug(@"Copying UploadPackage %@ to %@", self, newPackage);
5895
return newPackage;
@@ -76,30 +113,30 @@ - (void)completeDelivery {
76113
@"It's an API violation to call -completeDelivery twice.");
77114
}
78115
_isDelivered = YES;
116+
[_expirationTimer invalidate];
79117
if (!_isHandled && _handler &&
80118
[_handler respondsToSelector:@selector(packageDelivered:successful:)]) {
81-
[_expirationTimer invalidate];
82119
_isHandled = YES;
83120
[_handler packageDelivered:[self copy] successful:YES];
84121
}
85122
GDTCORLogDebug(@"Upload package delivered: %@", self);
86123
}
87124

88125
- (void)retryDeliveryInTheFuture {
126+
[_expirationTimer invalidate];
89127
if (!_isHandled && _handler &&
90128
[_handler respondsToSelector:@selector(packageDelivered:successful:)]) {
91-
[_expirationTimer invalidate];
92129
_isHandled = YES;
93130
[_handler packageDelivered:[self copy] successful:NO];
94131
}
95132
GDTCORLogDebug(@"Upload package will retry in the future: %@", self);
96133
}
97134

98-
- (void)checkIfPackageIsExpired:(NSTimer *)timer {
135+
- (void)checkIfPackageIsExpired {
99136
if ([[GDTCORClock snapshot] isAfter:_deliverByTime]) {
137+
[_expirationTimer invalidate];
100138
if (_handler && [_handler respondsToSelector:@selector(packageExpired:)]) {
101139
_isHandled = YES;
102-
[_expirationTimer invalidate];
103140
GDTCORLogDebug(@"Upload package expired: %@", self);
104141
[_handler packageExpired:self];
105142
}

‎GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadPackage_Private.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,7 @@
2424
/** A handler that will receive callbacks for certain events. */
2525
@property(nonatomic) id<NSSecureCoding, GDTCORUploadPackageProtocol> handler;
2626

27+
/** Checks if the package is expired and calls -packageExpired: on the handler if necessary. */
28+
- (void)checkIfPackageIsExpired;
29+
2730
@end

‎GoogleDataTransport/GDTCORLibrary/Public/GDTCORUploadPackage.h

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,7 @@
6262
* @param target The target/destination of this package.
6363
* @return An instance of this class.
6464
*/
65-
- (instancetype)initWithTarget:(GDTCORTarget)target NS_DESIGNATED_INITIALIZER;
66-
67-
// Please use the designated initializer.
68-
- (instancetype)init NS_UNAVAILABLE;
65+
- (instancetype)initWithTarget:(GDTCORTarget)target;
6966

7067
/** Completes delivery of the package.
7168
*

‎GoogleDataTransport/GDTCORTests/Unit/GDTCORUploadPackageTest.m

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,4 +142,15 @@ - (void)testExpiration {
142142
[self waitForExpectations:@[ expectation ] timeout:30];
143143
}
144144

145+
/** Tests that the upload package is not leaked by using an NSTimer. */
146+
- (void)testNoMemoryLeak {
147+
__weak GDTCORUploadPackage *weakPackage;
148+
@autoreleasepool {
149+
GDTCORUploadPackage *package = [[GDTCORUploadPackage alloc] initWithTarget:kGDTCORTargetTest];
150+
weakPackage = package;
151+
package = nil;
152+
}
153+
XCTAssertNil(weakPackage);
154+
}
155+
145156
@end

0 commit comments

Comments
 (0)