Skip to content

Commit c5509f8

Browse files
mkaretafacebook-github-bot
authored andcommitted
fixed cache leak when context contains non-recreatable/invalid context
Summary: This change allows reuse pool to "ask" controller if context is still valid and can be recreated for future reuse. Goal is to fix cache leak issue, when view is stored in reuse pool but would be never reused, since context contains piece of data that can't be recreated anymore. Reviewed By: kevin0571 Differential Revision: D26914733 fbshipit-source-id: de4247b78328631b9407a9f5a844a3d535e4972c
1 parent 6d5b49e commit c5509f8

File tree

4 files changed

+155
-0
lines changed

4 files changed

+155
-0
lines changed

ComponentKit/StatefulViews/CKStatefulViewComponentController.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,17 @@
6161
*/
6262
+ (NSInteger)maximumPoolSize:(ContextType)context;
6363

64+
/**
65+
Optionally override this to validate if context is still valid and related view can be reused.
66+
If context becames invalid, it will be eventually removed from reuse pool.
67+
Default implementaion returns YES.
68+
This method should return NO, only when context can't be recreated anymore,
69+
and view will be never reused.
70+
WARNING: context invalidation must be one way process. If this function
71+
returns YES for context that was previously invalid then behaviour is undefined.
72+
*/
73+
+ (BOOL)isContextValid:(ContextType)context;
74+
6475
/**
6576
The current stateful view owned by this controller, if any.
6677

ComponentKit/StatefulViews/CKStatefulViewComponentController.mm

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ + (NSInteger)maximumPoolSize:(id)context
4747
return -1;
4848
}
4949

50+
+ (BOOL)isContextValid:(id)context {
51+
return YES;
52+
}
53+
5054
- (UIView *)statefulView
5155
{
5256
return _statefulView;

ComponentKit/StatefulViews/CKStatefulViewReusePool.mm

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,16 @@ - (UIView *)dequeueStatefulViewForControllerClass:(Class)controllerClass
123123
if (it == _pool.end()) { // Avoid overhead of creating the item unless it already exists
124124
return nil;
125125
}
126+
if (![it->first.first isContextValid:it->first.second]) {
127+
// Context is invalid. This generally should not happen
128+
// since invalid context == non reacreatable context
129+
// But we still handle it to make sure that behaviour is
130+
// well defined
131+
132+
// Remove views with invalid context from reuse pool
133+
_pool.erase(it);
134+
return nil;
135+
}
126136
UIView *candidate = it->second.viewWithPreferredSuperview(preferredSuperview);
127137
if (candidate) {
128138
return candidate;
@@ -144,6 +154,13 @@ - (void)enqueueStatefulView:(UIView *)view
144154
RCAssertNotNil(controllerClass, @"Must provide a controller class");
145155
RCAssertNotNil(mayRelinquishBlock, @"Must provide a relinquish block");
146156

157+
// Shortcut for invalid contexts. If context is invalid
158+
// by the time when it is being added to the pool, we should
159+
// drop it immediately
160+
if (![controllerClass isContextValid:context]) {
161+
return;
162+
}
163+
147164
auto const addEntry = ^{
148165
auto &poolItem = _pendingPool[std::make_pair(controllerClass, context)];
149166
poolItem.addEntry({view, mayRelinquishBlock});
@@ -165,13 +182,20 @@ - (void)enqueueStatefulView:(UIView *)view
165182
RCDispatchMainDefaultMode(^{
166183
self->_enqueuedPendingPurge = NO;
167184
[self purgePendingPool];
185+
[self dropViewsWithInvalidContext];
168186
});
169187
}
170188

171189
- (void)purgePendingPool
172190
{
173191
RCAssertMainThread();
174192
for (const auto &it : _pendingPool) {
193+
// Ignore items that already can't be reused to save some cycles
194+
BOOL isContextValid = [it.first.first isContextValid:it.first.second];
195+
if (!isContextValid) {
196+
continue;
197+
}
198+
175199
// maximumPoolSize will be -1 by default
176200
NSInteger maximumPoolSize = [it.first.first maximumPoolSize:it.first.second];
177201

@@ -183,4 +207,15 @@ - (void)purgePendingPool
183207
_clearingPendingPool = NO;
184208
}
185209

210+
- (void)dropViewsWithInvalidContext {
211+
RCAssertMainThread();
212+
for (auto it = _pool.begin(); it != _pool.end();) {
213+
if (![it->first.first isContextValid:it->first.second]) {
214+
it = _pool.erase(it);
215+
} else {
216+
++it;
217+
}
218+
}
219+
}
220+
186221
@end

ComponentKitTests/StatefulViews/CKStatefulViewReusePoolTests.mm

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,22 @@ + (NSInteger)maximumPoolSize:(id)context
3434

3535
@end
3636

37+
@interface CKStatefulViewComponentInvalidatableContext: NSObject
38+
@property (assign, nonatomic) BOOL isValid;
39+
@end
40+
@implementation CKStatefulViewComponentInvalidatableContext
41+
@end
42+
43+
@interface CKStatefulViewComponentWithInvalidContextController : CKStatefulViewComponentController
44+
@end
45+
@implementation CKStatefulViewComponentWithInvalidContextController
46+
47+
+ (BOOL)isContextValid:(CKStatefulViewComponentInvalidatableContext *)context {
48+
return context.isValid;
49+
}
50+
51+
@end
52+
3753
@interface EnqueueOnDealloc: NSObject
3854
+ (instancetype)newWithPool:(CKStatefulViewReusePool *)pool;
3955
@end
@@ -272,6 +288,95 @@ - (void)testMaximumPoolSizeOfOneByEnqueueingTwoViewsThenDequeueingTwoViewsReturn
272288
XCTAssertTrue(dequeuedView2 != view2, @"Didn't expect view in container2 to be returned");
273289
}
274290

291+
- (void)testInvalidContextEnqueue
292+
{
293+
CKStatefulViewReusePool *pool = [[CKStatefulViewReusePool alloc] init];
294+
CKStatefulViewComponentInvalidatableContext *context = [CKStatefulViewComponentInvalidatableContext new];
295+
296+
__block int calledBlockCount = 0;
297+
298+
UIView *container1 = [[UIView alloc] init];
299+
CKTestStatefulView *view1 = [[CKTestStatefulView alloc] init];
300+
[container1 addSubview:view1];
301+
[pool enqueueStatefulView:view1
302+
forControllerClass:[CKStatefulViewComponentWithInvalidContextController class]
303+
context:context
304+
mayRelinquishBlock:^BOOL{
305+
return YES;
306+
}];
307+
308+
UIView *container2 = [[UIView alloc] init];
309+
CKTestStatefulView *view2 = [[CKTestStatefulView alloc] init];
310+
[container2 addSubview:view2];
311+
[pool enqueueStatefulView:view2
312+
forControllerClass:[CKOtherStatefulViewComponentController class]
313+
context:nil
314+
mayRelinquishBlock:^BOOL{
315+
calledBlockCount++;
316+
return YES;
317+
}];
318+
319+
CKRunRunLoopUntilBlockIsTrue(^BOOL{
320+
return calledBlockCount == 1;
321+
});
322+
323+
UIView *dequeuedView1 = [pool dequeueStatefulViewForControllerClass:[CKStatefulViewComponentWithInvalidContextController class]
324+
preferredSuperview:container1
325+
context:nil];
326+
XCTAssertTrue(dequeuedView1 == nil, @"Expected nil to be returned, since context is invalid");
327+
328+
UIView *dequeuedView2 = [pool dequeueStatefulViewForControllerClass:[CKOtherStatefulViewComponentController class]
329+
preferredSuperview:container2
330+
context:nil];
331+
XCTAssertTrue(dequeuedView2 == view2, @"View with valid context should be returned");
332+
}
333+
334+
335+
- (void)testInvalidContextAfterEnqueue
336+
{
337+
CKStatefulViewReusePool *pool = [[CKStatefulViewReusePool alloc] init];
338+
CKStatefulViewComponentInvalidatableContext *context = [CKStatefulViewComponentInvalidatableContext new];
339+
context.isValid = YES;
340+
341+
__block int calledBlockCount = 0;
342+
343+
UIView *container1 = [[UIView alloc] init];
344+
CKTestStatefulView *view1 = [[CKTestStatefulView alloc] init];
345+
[container1 addSubview:view1];
346+
[pool enqueueStatefulView:view1
347+
forControllerClass:[CKStatefulViewComponentWithInvalidContextController class]
348+
context:context
349+
mayRelinquishBlock:^BOOL{
350+
return YES;
351+
}];
352+
context.isValid = NO;
353+
354+
UIView *container2 = [[UIView alloc] init];
355+
CKTestStatefulView *view2 = [[CKTestStatefulView alloc] init];
356+
[container2 addSubview:view2];
357+
[pool enqueueStatefulView:view2
358+
forControllerClass:[CKOtherStatefulViewComponentController class]
359+
context:nil
360+
mayRelinquishBlock:^BOOL{
361+
calledBlockCount++;
362+
return YES;
363+
}];
364+
365+
CKRunRunLoopUntilBlockIsTrue(^BOOL{
366+
return calledBlockCount == 1;
367+
});
368+
369+
UIView *dequeuedView1 = [pool dequeueStatefulViewForControllerClass:[CKStatefulViewComponentWithInvalidContextController class]
370+
preferredSuperview:container1
371+
context:nil];
372+
XCTAssertTrue(dequeuedView1 == nil, @"Expected nil to be returned, since context is invalid");
373+
374+
UIView *dequeuedView2 = [pool dequeueStatefulViewForControllerClass:[CKOtherStatefulViewComponentController class]
375+
preferredSuperview:container2
376+
context:nil];
377+
XCTAssertTrue(dequeuedView2 == view2, @"View with valid context should be returned");
378+
}
379+
275380
#pragma mark - Pending pool tests
276381

277382
- (void)testEnqueueingViewThenDequeueingWithPendingEnabledReturnsSameViewImmediately

0 commit comments

Comments
 (0)