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

[StevenMasini]

Changelogs

1- Use weak self reference in progression and completion block to avoid memory leaks

It's recommended to never retain the strong reference to self into blocks.

Blocks maintain strong references to any captured objects, including self, which means that it’s easy to end up with a strong reference cycle

Avoid Strong Reference Cycles when Capturing self

2- Broadcast a notification when different instance share the same source succeed to download an image

As mentioned in #432, when two instance of FFFastImageView share the same source, but one of them fail to download while the second succeed, the result of the second won't be shared with the first one. Which result with instance of FFFastImageView staying empty.

To remedy this issue I used NSNotificationCenter to attach an observer and post a notification whenever an instance of FFFastImageView succeed to download. Works very well on my own project, it's a game changer for me.

3- Implement auto-retry failed download logic

As mentioned in #432, we can't only rely on SDWebImageRetryFailed to auto-retry when a download failed.

Also apparently this feature has been requested on the SDWebImage repository and is still waiting for implementation.
SDWebImage/SDWebImage#2587

So I implemented my own auto-retry mechanism which will retry after 1.5s, 3s and 4.5s before failing. In total it makes the timeout for the image download to 9s.

Let me know if you want to bring more flexibility to this feature.

4- Update Objective-C syntax to highlight instance properties

Being a former Objective-C iOS engineer myself, I updated the code to the modern Objective-C syntax.

Adopting Modern Objective-C
Migrating to Modern Objective-C See slides 47 and 48

Updating the syntax helped me to realized that their were more invocation to strong self in blocks that I initially thought. See #398

5- Use RCTDirectEventBlock instead of RCTBubblingEventBlock

From what I understand from the React Native documentation.

RCTBubblingEventBlock are used for UI related events (e.g: the user pinch the map)
RCTDirectEventBlock are used for more straight events (e.g web page failed to load callbacks).

So I replaced all the RCTBubblingEventBlock types by RCTDirectEventBlock as we don't handle user interaction.

Also found this gist about the subject.

[codecov]

Codecov Report

Merging #433 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #433   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files           1        1           
  Lines          19       19           
=======================================
  Hits           18       18           
  Misses          1        1

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4849f2b...1c19f96. Read the comment docs.

[StevenMasini]

Just in case someone wants to try this branch on their own project, I made a npm module.

In your package.json use

"@stevenmasini/react-native-fast-image": "^5.2.0",

For iOS, you will need to update your XCode project to use the
./node_modules/@stevenmasini/react-native-fast-image/ios/FastImage.xcodeproj.

For Android, in you settings.gradle

include ':react-native-fast-image'
project(':react-native-fast-image').projectDir = new File(rootProject.projectDir, '../node_modules/@stevenmasini/react-native-fast-image/android')

[evanjmg]

@DylanVann any feedback on this?

[evanjmg]

I tried this out and I'm getting repeated images in cells. Is there anything changed in this that could cause this issue? @StevenMasini any thoughts? I'm using .map((item=> <FastImage source={{ uri }} />) - UPDATE: ah this happens in 5.2.0 as well. Anyway to set a key?

[StevenMasini]

@evanjmg

Do you means the warning that <FlatList /> will trigger if you don't specify a key in the renderItem method ?

Then if that the case you just need to add a prop key to your element.

Something like

.map( (item, index) => {
     <FastImage key={`0x${index}`} source={{ uri: item.uri }} />
})

If not can you give me more detail ?

[StevenMasini]

@DylanVann I am asking once again to review this PR 🙏

I am already using it in Production in my own app via @steven/react-native-fast-image module.

Also this PR is a follow up and aim to fix these issues #371 #421 #394

[evanjmg]

I'm using it in production too 👍

[evanjmg]

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

[evanjmg]

perhaps check for source.url?

[StevenMasini]

perhaps check for source.url?

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

[StevenMasini]

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.

[evanjmg]

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?

[evanjmg]

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.

[StevenMasini]

@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

[pribeh]

This issue is severe. Can we get this reviewed.

[DylanVann]

@StevenMasini This looks good. Turning on some warning checks confirms that you are right about the usage of self inside blocks.

I would merge these changes except I'm unsure about the retry handling code. Whatever we provide for retrying should be consistent for Android and iOS.

If you remove the retry related changes I can merge this and we could handle them in another PR.

[StevenMasini]

@DylanVann I will extract the auto-retry logic from this PR and submit a new PR specific to it.

I also need to fix the issue found by @evanjmg.

[StevenMasini]

@DylanVann I removed the auto-retry logic from this branch.

If @evanjmg come back to me about his crash, I can fix it in another PR.

Also I am willing to contribute more on the native iOS part if you need me 👍

[evanjmg]

I had to revert to 5.2.0 due to the images not appearing sometimes (perhaps due to retry logic a can’t seem to figure it out) and also I can’t easily reproduce any crashes but yes they still happen with the old code. For the crash, I’ve added a hacky try catch and check if the method exists before executing and that patches it. Right now, I’ll pull your latest and see if the images not appearing issue goes away.

…

On Apr 22, 2019, at 8:30 AM, Steven Masini ***@***.***> wrote: @DylanVann I removed the auto-retry logic from this branch. If @evanjmg come back to me about his crash, I can fix it in another PR. Also I am willing to contribute more on the native iOS part if you need me 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[StevenMasini]

@evanjmg I haven't updated @steven/react-native-fast-image as I need the auto-retry logic in my own project.

Where did you put your try/catch ?

[evanjmg]

- (void)imageDidLoadObserver:(NSNotification *)notification {
    FFFastImageSource *source = notification.object;
    if (source != nil) {
        BOOL responds = [self respondsToSelector:@selector(sd_setImageWithURL:)];
        if (responds == YES) {
            @try {
                [self sd_setImageWithURL:source.url];
            } @catch (NSException * e) {
                NSLog(@"Exception: %@", e);
            }
        }
    }
}

[DylanVann]

This looks good!

I may end up also removing the notification center changes before publishing though. I'm trying to keep things between Android and iOS in sync, and trying to primarily leverage the native libraries SDWebImage and Glide. So I think a more correct place to add this functionality might be in SDWebImage

Alternatively there's this issue if you know of another iOS library that supports this:

Consider switching to a different iOS image loading library.

[StevenMasini]

@DylanVann Yeah I was thinking about it yesterday while removing the auto-retry logic. I will submit a PR directly to SDWebImage for this feature.

But for the notification center, I would still advise you to keep this change as it's mostly RN specific issue. I can't suggest something like this directly to SDWebImage.

The problem with RN is that the component will re-render only if the props have changed. So if a similar image has finish loading in another instance of FastImageView, then previously loaded FastImageView won't be refreshed.

I checked Android don't have this problem for some reason.