Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: Memory Leak, Auto-retry download, Shared Resource Propagation #433

Merged
merged 7 commits into from
Apr 23, 2019
Merged

iOS: Memory Leak, Auto-retry download, Shared Resource Propagation #433

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2deaf1
feat: broadcast a notification when image succeed to download
Mar 20, 2019
4a57513
chore: modernize the syntax and user weak self reference in block
Mar 20, 2019
824eb43
chore: no need to have two lines to set the default value of options …
Mar 20, 2019
721a24f
feat: implement auto-retry download after 1.5, 3 or 4.5 seconds befor…
Mar 20, 2019
86b8b16
chore: replace all RCTBubblingEventBlock by RCTDirectEventBlock
Mar 20, 2019
1f0102d
chore: correct syntax
Mar 20, 2019
1c19f96
feat: remove the auto-retry logic
Apr 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 99 additions & 78 deletions ios/FastImage/FFFastImageView.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
#import "FFFastImageView.h"

@implementation FFFastImageView {
BOOL hasSentOnLoadStart;
BOOL hasCompleted;
BOOL hasErrored;
NSDictionary* onLoadEvent;
}

@interface FFFastImageView()

@property (nonatomic, assign) BOOL hasSentOnLoadStart;
@property (nonatomic, assign) BOOL hasCompleted;
@property (nonatomic, assign) BOOL hasErrored;

@property (nonatomic, strong) NSDictionary* onLoadEvent;

@end

@implementation FFFastImageView

- (id) init {
self = [super init];
Expand All @@ -14,82 +20,95 @@ - (id) init {
return self;
}

- (void)setResizeMode:(RCTResizeMode)resizeMode
{
- (void)dealloc {
[NSNotificationCenter.defaultCenter removeObserver:self];
}

- (void)setResizeMode:(RCTResizeMode)resizeMode {
if (_resizeMode != resizeMode) {
_resizeMode = resizeMode;
self.contentMode = (UIViewContentMode)resizeMode;
}
}

- (void)setOnFastImageLoadEnd:(RCTBubblingEventBlock)onFastImageLoadEnd {
- (void)setOnFastImageLoadEnd:(RCTDirectEventBlock)onFastImageLoadEnd {
_onFastImageLoadEnd = onFastImageLoadEnd;
if (hasCompleted) {
if (self.hasCompleted) {
_onFastImageLoadEnd(@{});
}
}

- (void)setOnFastImageLoad:(RCTBubblingEventBlock)onFastImageLoad {
- (void)setOnFastImageLoad:(RCTDirectEventBlock)onFastImageLoad {
_onFastImageLoad = onFastImageLoad;
if (hasCompleted) {
_onFastImageLoad(onLoadEvent);
if (self.hasCompleted) {
_onFastImageLoad(self.onLoadEvent);
}
}

- (void)setOnFastImageError:(RCTDirectEventBlock)onFastImageError {
_onFastImageError = onFastImageError;
if (hasErrored) {
if (self.hasErrored) {
_onFastImageError(@{});
}
}

- (void)setOnFastImageLoadStart:(RCTBubblingEventBlock)onFastImageLoadStart {
if (_source && !hasSentOnLoadStart) {
- (void)setOnFastImageLoadStart:(RCTDirectEventBlock)onFastImageLoadStart {
if (_source && !self.hasSentOnLoadStart) {
_onFastImageLoadStart = onFastImageLoadStart;
onFastImageLoadStart(@{});
hasSentOnLoadStart = YES;
self.hasSentOnLoadStart = YES;
} else {
_onFastImageLoadStart = onFastImageLoadStart;
hasSentOnLoadStart = NO;
self.hasSentOnLoadStart = NO;
}
}

- (void)sendOnLoad:(UIImage *)image {
onLoadEvent = @{
@"width":[NSNumber numberWithDouble:image.size.width],
@"height":[NSNumber numberWithDouble:image.size.height]
};
if (_onFastImageLoad) {
_onFastImageLoad(onLoadEvent);
self.onLoadEvent = @{
@"width":[NSNumber numberWithDouble:image.size.width],
@"height":[NSNumber numberWithDouble:image.size.height]
};
if (self.onFastImageLoad) {
self.onFastImageLoad(self.onLoadEvent);
}
}

- (void)imageDidLoadObserver:(NSNotification *)notification {
Copy link

@evanjmg evanjmg Apr 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is the line that is crashing for me. @StevenMasini ? any ideas?

Fatal Exception: NSInvalidArgumentException
0  CoreFoundation                 0x208d19eb8 __exceptionPreprocess
1  libobjc.A.dylib                0x207f13f18 objc_exception_throw
2  CoreFoundation                 0x208c2ef10 -[NSOrderedSet initWithSet:copyItems:]
3  CoreFoundation                 0x208d1f924 ___forwarding___
4  CoreFoundation                 0x208d216f0 _CF_forwarding_prep_0
5  App                      0x102ddae0c -[FFFastImageView imageDidLoadObserver:] (FFFastImageView.m:79)
6  Foundation                     0x2097b7618 __57-[NSNotificationCenter addObserver:selector:name:object:]_block_invoke_2
7  CoreFoundation                 0x208c85fcc __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__
8  CoreFoundation                 0x208c85f8c ___CFXRegistrationPost_block_invoke
9  CoreFoundation                 0x208c85434 _CFXRegistrationPost
10 CoreFoundation                 0x208c850c0 ___CFXNotificationPost_block_invoke
11 CoreFoundation                 0x208bfaae4 -[_CFXNotificationRegistrar find:object:observer:enumerator:]
12 CoreFoundation                 0x208c84b44 _CFXNotificationPost
13 Foundation                     0x2096a4b24 -[NSNotificationCenter postNotificationName:object:userInfo:]
14 UIKitCore                      0x236e74ca4 -[UIViewAnimationState sendDelegateAnimationDidStop:finished:]
15 UIKitCore                      0x236e75024 -[UIViewAnimationState animationDidStop:finished:]
16 QuartzCore                     0x20d3d2338 CA::Layer::run_animation_callbacks(void*)
``` - -[UIViewAnimationState url]: unrecognized selector sent to instance 0x1120ad720

Copy link

Choose a reason for hiding this comment

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

perhaps check for source.url?

Copy link
Author

Choose a reason for hiding this comment

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

perhaps check for source.url?

Any way to reproduce the crash ? What was the source.url ?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this crash is link with the observer that I attached to the event.

[FFFastImageView imageDidLoadObserver:

I will investigate this. Would be helpful if you could tell me how do you call <FastImage>, which props to you give to it.

Copy link

Choose a reason for hiding this comment

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

I still I’m having trouble reproducing as my app has hundreds of images in it and I need to add more metadata to crashalytics. The only props I use are onLoad, source.headers, source.uri and priority. Perhaps it’s an image resp error?

Copy link

Choose a reason for hiding this comment

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

For now I've reverted my production build to prevent crashes; hopefully I won't get memory leaks with 5.2.0. I'll try to reproduce on staging as I develop new features.

Copy link
Author

@StevenMasini StevenMasini Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@evanjmg Could you use XCode to intercept the crash and then print the value of source in the console.

I don't know if you are familiar with XCode debugging tools.

Debugging with XCode

In the breakpoint panel, enable All Exceptions to be caught.
Then once the crash occur, in the debug console type po (stand for print object) then the value you want to prospect.

po source

breakpoint-panel

FFFastImageSource *source = notification.object;
if (source != nil && source.url != nil) {
[self sd_setImageWithURL:source.url];
}
}

- (void)setSource:(FFFastImageSource *)source {
if (_source != source) {
_source = source;

// Attach a observer to refresh other FFFastImageView instance sharing the same source
[NSNotificationCenter.defaultCenter addObserver:self selector:@selector(imageDidLoadObserver:) name:source.url.absoluteString object:nil];

// Load base64 images.
NSString* url = [_source.url absoluteString];
if (url && [url hasPrefix:@"data:image"]) {
if (_onFastImageLoadStart) {
_onFastImageLoadStart(@{});
hasSentOnLoadStart = YES;
if (self.onFastImageLoadStart) {
self.onFastImageLoadStart(@{});
self.hasSentOnLoadStart = YES;
} {
hasSentOnLoadStart = NO;
self.hasSentOnLoadStart = NO;
}
UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:_source.url]];
[self setImage:image];
if (_onFastImageProgress) {
_onFastImageProgress(@{
@"loaded": @(1),
@"total": @(1)
});
if (self.onFastImageProgress) {
self.onFastImageProgress(@{
@"loaded": @(1),
@"total": @(1)
});
}
hasCompleted = YES;
self.hasCompleted = YES;
[self sendOnLoad:image];

if (_onFastImageLoadEnd) {
_onFastImageLoadEnd(@{});
if (self.onFastImageLoadEnd) {
self.onFastImageLoadEnd(@{});
}
return;
}
Expand All @@ -100,8 +119,7 @@ - (void)setSource:(FFFastImageSource *)source {
}];

// Set priority.
SDWebImageOptions options = 0;
options |= SDWebImageRetryFailed;
SDWebImageOptions options = SDWebImageRetryFailed;
switch (_source.priority) {
case FFFPriorityLow:
options |= SDWebImageLowPriority;
Expand All @@ -125,52 +143,55 @@ - (void)setSource:(FFFastImageSource *)source {
break;
}

if (_onFastImageLoadStart) {
_onFastImageLoadStart(@{});
hasSentOnLoadStart = YES;
if (self.onFastImageLoadStart) {
self.onFastImageLoadStart(@{});
self.hasSentOnLoadStart = YES;
} {
hasSentOnLoadStart = NO;
self.hasSentOnLoadStart = NO;
}
hasCompleted = NO;
hasErrored = NO;
self.hasCompleted = NO;
self.hasErrored = NO;

// Load the new source.
// This will work for:
// - https://
// - file:///var/containers/Bundle/Application/50953EA3-CDA8-4367-A595-DE863A012336/ReactNativeFastImageExample.app/assets/src/images/fields.jpg
// - file:///var/containers/Bundle/Application/545685CB-777E-4B07-A956-2D25043BC6EE/ReactNativeFastImageExample.app/assets/src/images/plankton.gif
// - file:///Users/dylan/Library/Developer/CoreSimulator/Devices/61DC182B-3E72-4A18-8908-8A947A63A67F/data/Containers/Data/Application/AFC2A0D2-A1E5-48C1-8447-C42DA9E5299D/Documents/images/E1F1D5FC-88DB-492F-AD33-B35A045D626A.jpg"
[self sd_setImageWithURL:_source.url
placeholderImage:nil
options:options
progress:^(NSInteger receivedSize, NSInteger expectedSize, NSURL * _Nullable targetURL) {
if (_onFastImageProgress) {
_onFastImageProgress(@{
@"loaded": @(receivedSize),
@"total": @(expectedSize)
});
}
} completed:^(UIImage * _Nullable image,
NSError * _Nullable error,
SDImageCacheType cacheType,
NSURL * _Nullable imageURL) {
if (error) {
hasErrored = YES;
if (_onFastImageError) {
_onFastImageError(@{});
}
if (_onFastImageLoadEnd) {
_onFastImageLoadEnd(@{});
[self downloadImage:_source options:options];
}
}

- (void)downloadImage:(FFFastImageSource *) source options:(SDWebImageOptions) options {
__weak typeof(self) weakSelf = self; // Always use a weak reference to self in blocks
[self sd_setImageWithURL:_source.url
placeholderImage:nil
options:options
progress:^(NSInteger receivedSize, NSInteger expectedSize, NSURL * _Nullable targetURL) {
if (weakSelf.onFastImageProgress) {
weakSelf.onFastImageProgress(@{
@"loaded": @(receivedSize),
@"total": @(expectedSize)
});
}
} completed:^(UIImage * _Nullable image,
NSError * _Nullable error,
SDImageCacheType cacheType,
NSURL * _Nullable imageURL) {
if (error) {
weakSelf.hasErrored = YES;
if (weakSelf.onFastImageError) {
weakSelf.onFastImageError(@{});
}
} else {
hasCompleted = YES;
[self sendOnLoad:image];
if (_onFastImageLoadEnd) {
_onFastImageLoadEnd(@{});
if (weakSelf.onFastImageLoadEnd) {
weakSelf.onFastImageLoadEnd(@{});
}
} else {
weakSelf.hasCompleted = YES;
[weakSelf sendOnLoad:image];

// Alert other FFFastImageView component sharing the same URL
[NSNotificationCenter.defaultCenter postNotificationName:source.url.absoluteString object:source];

if (weakSelf.onFastImageLoadEnd) {
weakSelf.onFastImageLoadEnd(@{});
}
}];
}
}
}];
}

@end
Expand Down