Skip to content

Commit

Permalink
Add RCTWeakProxy to properly deallocate RCTUIImageViewAnimated
Browse files Browse the repository at this point in the history
Summary:
@public
CADisplayLink strongly holds onto its target, so you have to use a weak proxy object to pass the target into the CADisplayLink.

Previously we passed a weak-self point (i.e. weakSelf) but this did not have the intended effect, since the pointer to self would still be passed to CADisplayLink, and thus it would hold onto the RCTUIImageViewAnimated strongly.

So is weakSelf doing anything other than using self?
It is but it's very minor and not useful. In the case that the object got de-allocated between assigning self to weakSelf and creating the CADisplayLink, then we would pass a nil target. This is actually impossible though because we are running an instance method, so self is implicitly retained! So semantically it is something different but in practice it is the same as passing self through.

Notes:
* This system was added originally in #24822
* #25636 then "enabled" this system by deprecating existing approach

Reviewed By: fkgozali

Differential Revision: D16939869

fbshipit-source-id: 7a0e947896f23aa30ad074d1dcb4d4db7543e00a
  • Loading branch information
Mehdi Mulani authored and facebook-github-bot committed Aug 22, 2019
1 parent d2213c7 commit 947e71a
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 2 deletions.
3 changes: 3 additions & 0 deletions Libraries/Image/RCTUIImageViewAnimated.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
*/

#import <React/RCTAnimatedImage.h>
#import <React/RCTDefines.h>

RCT_EXTERN void RCTUIImageViewEnableWeakProxy(BOOL enabled);

@interface RCTUIImageViewAnimated : UIImageView

Expand Down
11 changes: 9 additions & 2 deletions Libraries/Image/RCTUIImageViewAnimated.m
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@
*/

#import <React/RCTUIImageViewAnimated.h>
#import <React/RCTWeakProxy.h>

#import <mach/mach.h>
#import <objc/runtime.h>

static BOOL weakProxyEnabled = YES;
void RCTUIImageViewEnableWeakProxy(BOOL enabled) {
weakProxyEnabled = enabled;
}

static NSUInteger RCTDeviceTotalMemory() {
return (NSUInteger)[[NSProcessInfo processInfo] physicalMemory];
}
Expand Down Expand Up @@ -146,8 +152,9 @@ - (NSOperationQueue *)fetchQueue
- (CADisplayLink *)displayLink
{
if (!_displayLink) {
__weak __typeof(self) weakSelf = self;
_displayLink = [CADisplayLink displayLinkWithTarget:weakSelf selector:@selector(displayDidRefresh:)];
__weak typeof(self) weakSelf = self;
id target = weakProxyEnabled ? [RCTWeakProxy weakProxyWithTarget:self] : weakSelf;
_displayLink = [CADisplayLink displayLinkWithTarget:target selector:@selector(displayDidRefresh:)];
NSString *runLoopMode = [NSProcessInfo processInfo].activeProcessorCount > 1 ? NSRunLoopCommonModes : NSDefaultRunLoopMode;
[_displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:runLoopMode];
}
Expand Down
16 changes: 16 additions & 0 deletions React/Base/RCTWeakProxy.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import <Foundation/Foundation.h>

@interface RCTWeakProxy : NSObject

@property (nonatomic, weak, readonly) id target;

+ (instancetype)weakProxyWithTarget:(id)target;

@end
30 changes: 30 additions & 0 deletions React/Base/RCTWeakProxy.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import "RCTWeakProxy.h"

@implementation RCTWeakProxy

- (instancetype)initWithTarget:(id)target
{
if (self = [super init]) {
_target = target;
}
return self;
}

+ (instancetype)weakProxyWithTarget:(id)target
{
return [[RCTWeakProxy alloc] initWithTarget:target];
}

- (id)forwardingTargetForSelector:(SEL)aSelector
{
return _target;
}

@end

1 comment on commit 947e71a

@fkgozali
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ericlewis - fyi

Please sign in to comment.