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.

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

Memory leak when programmatically changing 'source' prop of Image #12220

Open
tomchambers2 opened this issue Feb 5, 2017 · 15 comments
Open

Memory leak when programmatically changing 'source' prop of Image #12220

tomchambers2 opened this issue Feb 5, 2017 · 15 comments
Labels
Bug Component: Image Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Platform: iOS iOS applications.

Comments

@tomchambers2
Copy link

tomchambers2 commented Feb 5, 2017

Description

If you use an Image component in render and use the parent component's state to manage the source prop, each time the source is changed the previous source is not deallocated. Even when the parent component is unmounted, the previous images still use memory.

imagetest_memory_leak

Reproduction

I've made a simple ImageTest project that demonstrates the bug. (IMPORTANT: run app in release mode to experience the issue)

Code:

/**
 * Sample React Native App
 * https://github.com/facebook/react-native
 * @flow
 */

import React, { Component } from 'react';
import {
  AppRegistry,
  StyleSheet,
  Text,
  View,
  Image,
  TouchableHighlight,
  Dimensions,
} from 'react-native';

import image0 from './images/0.png'
import image1 from './images/1.png'
import image2 from './images/2.png'
import image3 from './images/3.png'
import image4 from './images/4.png'

const images = [
  image0,
  image1,
  image2,
  image3,
  image4,
]

export default class ImageTest extends Component {
  constructor() {
    super()

    this.state = ({
      counter: 0,
    })
  }

  increment = () => {
    this.setState({
      counter: this.state.counter + 1
    })
  }

  render() {
    const imageSource = images[this.state.counter]

    return (
      <View>
        <Image
          source={imageSource}
          style={styles.image}
        />
        <TouchableHighlight onPress={this.increment} style={styles.highlight}>
          <Text style={styles.text}>NEXT IMAGE</Text>
        </TouchableHighlight>
      </View>
    );
  }
}

const window = Dimensions.get('window')

const styles = StyleSheet.create({
  highlight: {
    backgroundColor: 'yellow',
    position: 'absolute',
    bottom: 0,
    left: 0,
  },
  text: {
    fontSize: 50,
  },
  image: {
    width: window.width,
    height: window.height,
  },
});

AppRegistry.registerComponent('ImageTest', () => ImageTest);

Solution

Presumably when the source is changed the underlying UIImage/UIImageView needs to be destroyed.

Additional Information

  • React Native version: 0.41.0
  • Platform: iOS
  • Operating System: MacOS 10.12.3 / xcode 8.2
@hramos
Copy link
Contributor

hramos commented May 25, 2017

We're cutting down on the number of outstanding issues, in order to allow us to focus. I'm closing this issue because it has been open for over 60 days with no activity. If you think it should still be opened let us know why. PRs are always welcome.

@hramos hramos closed this as completed May 25, 2017
@hramos hramos added the Icebox label May 26, 2017
@weepy
Copy link

weepy commented Sep 5, 2017

I have this same issue. I'm trying to create an animated image sequence, but previous frames don't get deallocated resulting in massive memory issues.

Additional Information:

React Native version: 0.47.0
Platform: iOS

@shergin shergin added the Good first issue Interested in collaborating? Take a stab at fixing one of these issues. label Sep 5, 2017
@shergin shergin reopened this Sep 5, 2017
@jdmunro
Copy link
Contributor

jdmunro commented Oct 1, 2017

I spent some time investigating this. The issue seems to be related to React Native's usage of [UIImage imageNamed:] to load the file.

I came to this conclusion by modifying RCTImageFromLocalAssetURL by commenting out the following lines:

//  if (bundle) {
//    image = [UIImage imageNamed:imageName inBundle:bundle compatibleWithTraitCollection:nil];
//  } else {
//    image = [UIImage imageNamed:imageName];
//  }

This way, the image loading falls back to [UIImage imageWithContentsOfFile:]. Using this mechanism, the memory profile of cycling through the images is mostly flat as the previous image is deallocated when the next image is loaded:

screen shot 2017-10-01 at 18 13 08

The difference here is that [UIImage imageNamed:] has an internal, system managed cache of images. You can verify that you're seeing a cached image by cycling back through to the first image: the memory usage no longer increases.

The cache should release in low memory conditions. I tried simulating this using the Simulate Memory Warning in the simulator but this did not free up the memory.

According to the official Apple documentation:

The system may purge cached image data at any time to free up memory. Purging occurs only for images that are in the cache but are not currently being used.

Further investigation is required to determine if this counts as 'not currently being used' or not and whether these images (now off-screen) should be purged.

Also interesting to consider:

If you have an image file that will only be displayed once and wish to ensure that it does not get added to the system’s cache, you should instead create your image using imageWithContentsOfFile:. This will keep your single-use image out of the system image cache, potentially improving the memory use characteristics of your app.

This suggests that images that will be displayed once, such as the background slides of an app-intro, which may well be high-resolution, should not be loaded using [UIImage imageNamed:] because we do not want to cache them. I'm unsure how or if React Native should be able to make a decision like this and whether it makes sense for the developer to give a hint as to the purpose of the image?

@joshjhargreaves
Copy link
Contributor

joshjhargreaves commented Oct 6, 2017

Thank you @jdmunro for the excellent, informative investigation, and @tomchambers2 for the demo project.

IMHO I think it should be up to the developer to decide if an image should be cached or not as I think, simply put, there's no way for React Native to know if application is likely to use an <img> with the same source again.

For the high-res 'app-intro' example @jdmunro mentions, perhaps you could pass a property to img like so: <img noSystemCache={true} />, as we specifically know that we (as the developer) will never show that image again outside of the app-into screen in the app again.

@shergin
Copy link
Contributor

shergin commented Oct 6, 2017

I agree that we should not introduce any kind of heuristic algorithm here because [UIImage imageNamed:] already has something like that. (because it leads to unpredictable (bad) developer experience.)

@jdmunro
Copy link
Contributor

jdmunro commented Oct 9, 2017

Something such as @joshyhargreaves suggested could work, if we can agree on an API. I'd be happy to give this a go. I haven't confirmed whether this is an issue on Android and so this may end up being an iOS-only prop.

Perhaps this could be part of the image source prop types as these already have more advanced options relating to network caching behaviours for remote images: https://github.com/facebook/react-native/blob/1e8f3b11027fe0a7514b4fc97d0798d3c64bc895/Libraries/Image/ImageSourcePropType.js

@ignacioola
Copy link
Contributor

Maybe the native iOS caching @jdmunro mentions should be enabled when the user explicitly declares the <Image> should be cached by using the cache control mechanism, setting the source.cache prop to 'only-if-cached' or 'force-cache'.

@raphaelrk
Copy link

raphaelrk commented Dec 12, 2018

Was also running into this issue with remote images saved locally (to reduce Image flickering on mount/unmount). Made a reproducible demo here: snack link

@troZee
Copy link
Contributor

troZee commented Jun 15, 2020

Hey,
I was trying to reproduce this issue on my fork, but I could not find any leak here.

I run app in release mode and run profiler to check, if any leak occurs. Unfortunately I did not notice any unexpected behaviour. To reproduce it, I used images from RN assets, so maybe that's why I could not see any leak here.

Can you provide an images, which you used to reproduce this issue ?

@peterzanetti
Copy link

Still reproducible for me in RN 0.61.3. No way to clear the cache or prevent caching once loaded means inevitable crash of the device after loading a handful of images. It is pretty shocking that this is still an issue.

@huseyin39
Copy link

huseyin39 commented Apr 26, 2021

RN 0.64 and still happening.
Edit: I think it's more related to components than image only. The same issue happens with audio file.

@github-actions
Copy link

github-actions bot commented Mar 1, 2023

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added Stale There has been a lack of activity on this issue and it may be closed soon. and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Mar 1, 2023
@jake-stewart
Copy link

jake-stewart commented Dec 28, 2023

this is still an issue.

EDIT: for anyone facing this issue in the future, react-native-fast-image does not have this memory leak.

@shubhamguptadream11
Copy link
Collaborator

Can anyone confirm if this is still an issue on latest RN version 0.75.4?

@coolsoftwaretyler
Copy link

Looks like it. I've got it reproducing with this React Native 0.76.4 app: https://github.com/coolsoftwaretyler/react-native-image-memory-leak-repro

Screenshot 2024-12-08 at 8 03 36 PM

Seems there's been some discussion here about the right way to handle this, but not a good resolution on a path forward.

I'd be happy to write up a PR with a proposed prop on the Image component to allow developers to opt out of [UIImage imageNamed:] if they want to deallocate transient images. If it's still a non-starter, no worries, I am just interested in hacking around on it a little.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Component: Image Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Platform: iOS iOS applications.
Projects
None yet
Development

No branches or pull requests