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 grabbou committed Aug 26, 2019
1 parent 1dd1152 commit 6d6b532
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

4 comments on commit 6d6b532

@vanhungoz
Copy link

@vanhungoz vanhungoz commented on 6d6b532 Dec 20, 2019

Choose a reason for hiding this comment

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

i find out this commit makes a crash in my app.
related issue: #27463

@vanhungoz
Copy link

Choose a reason for hiding this comment

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

{
"LocallySymbolicatedSourceFile": "",
"UserData": {
"Resolved": false,
"Name": "QuartzCore: CA::Display::DisplayLink::dispatch_items(unsigned long long, unsigned long long, unsigned long long) + 636",
"Description": ""
},
"LocallySymbolicatedDefaultName": "",
"DefaultName": "QuartzCore: CA::Display::DisplayLink::dispatch_items(unsigned long long, unsigned long long, unsigned long long) + 636",
"LocallySymbolicatedSourceLine": 0,
"Identifier": {
"AdamId": "1321343906",
"Version": "2.1.8",
"IsBeta": true,
"Build": "3",
"PlatformSDKIdentifier": "com.apple.platform.iphoneos",
"RootBuild": "3",
"RootVersion": "2.1.8",
"CrashPointId": "x32HH37qJKL-pe7wQzVaF"
},
"SourceLine": 2332,
"SourceFile": "CADisplay.mm"
}
Json log crash file

@vanhungoz
Copy link

Choose a reason for hiding this comment

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

Incident Identifier: 0D30CBED-7A3C-4FDA-94B5-D974D396DE29
Beta Identifier: 450624D1-F701-44C6-B2EB-00C2055DD2E2
Hardware Model: iPhone8,1
Version: 3 (2.1.8)
AppStoreTools: 11B48b
AppVariant: 1:iPhone8,1:12.2
Beta: YES
Code Type: ARM-64 (Native)
Role: Non UI
Parent Process: launchd [1]

Date/Time: 2019-12-06 09:56:16.6807 +0700
Launch Time: 2019-12-06 09:56:10.4703 +0700
OS Version: iPhone OS 12.2 (16E227)
Baseband Version: 5.55.00
Report Version: 104

Exception Type: EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000
Exception Note: EXC_CORPSE_NOTIFY
Triggered by Thread: 0

Last Exception Backtrace:
0 CoreFoundation 0x219f58518 __exceptionPreprocess + 228 (NSException.m:172)
1 libobjc.A.dylib 0x2191339f8 objc_exception_throw + 56 (objc-exception.mm:557)
2 CoreFoundation 0x219e75278 -[NSObject(NSObject) doesNotRecognizeSelector:] + 140 (NSObject.m:323)
3 CoreFoundation 0x219f5dd60 forwarding + 1408 (NSForwarding.m:3224)
4 CoreFoundation 0x219f5f9fc _CF_forwarding_prep_0 + 92
5 QuartzCore 0x21e2ba194 CA::Display::DisplayLink::dispatch_items(unsigned long long, unsigned long long, unsigned long long) + 636 (CADisplay.mm:2332)
6 IOKit 0x21a1b4718 IODispatchCalloutFromCFMessage + 488 (IOKitLib.c:1216)
7 CoreFoundation 0x219ec3d30 __CFMachPortPerform + 188 (CFMachPort.c:522)
8 CoreFoundation 0x219eea934 CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION + 56 (CFRunLoop.c:1996)
9 CoreFoundation 0x219eea080 __CFRunLoopDoSource1 + 440 (CFRunLoop.c:2133)
10 CoreFoundation 0x219ee4ea4 __CFRunLoopRun + 2096 (CFRunLoop.c:3152)
11 CoreFoundation 0x219ee4354 CFRunLoopRunSpecific + 436 (CFRunLoop.c:3247)
12 GraphicsServices 0x21c0e479c GSEventRunModal + 104 (GSEvent.c:2245)
13 UIKitCore 0x246357b68 UIApplicationMain + 212 (UIApplication.m:4353)
15 libdyld.dylib 0x2199aa8e0 start + 4

@vanhungoz
Copy link

Choose a reason for hiding this comment

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

i used react-native-fast-image to display the images

Please sign in to comment.