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

fix: UIImage leaking after Image is disposed #9777

Merged
merged 1 commit into from Feb 16, 2022
Merged

Conversation

rigor789
Copy link
Member

@rigor789 rigor789 commented Feb 16, 2022

PR Checklist

What is the current behavior?

Screenshot 2022-02-16 at 11 12 33 PM

What is the new behavior?

Screenshot 2022-02-16 at 11 14 43 PM

The above screenshots are produced by running the following test code, navigating to the "Show Image" page, and back.

const {
  Application,
  Frame,
  Page,
  Button,
  Image,
} = require("@nativescript/core");

let count = 0;

const makeModalPage = () => {
  const page = new Page();
  page.id = `page${count++}`;

  const image = new Image();

  image.src =
    "https://i.picsum.photos/id/669/5000/5000.jpg?hmac=VlpchW0ODhflKm0SKOYQrc8qysLWbqKmDS1MGT9apAc";

  page.content = image;

  return page;
};

Application.run({
  create() {
    const rootFrame = new Frame();
    rootFrame.navigate({
      create: () => {
        const page = new Page();
        const button = new Button();

        button.text = "Open Image";
        button.on("tap", () => {
          rootFrame.navigate({
            create() {
              return makeModalPage();
            },
          });
        });

        page.content = button;

        return page;
      },
    });

    return rootFrame;
  },
});

@cla-bot cla-bot bot added the cla: yes label Feb 16, 2022
@NathanWalker NathanWalker merged commit 6d7b135 into release/8.2.0 Feb 16, 2022
2 of 3 checks passed
@NathanWalker NathanWalker deleted the fix/image-leak branch Feb 16, 2022
@Archez
Copy link
Contributor

@Archez Archez commented Feb 16, 2022

@rigor789 Should we apply the same fix when updating the image source property? Looks like we just null out any existing image source before creating the replacement.

@rigor789
Copy link
Member Author

@rigor789 rigor789 commented Feb 17, 2022

@Archez good point - yes absolutely, probably makes sense to create a getter/setter setup on imageSource and handle it in the setter, so we can just = null it and it would do the release.

And for clarity, we did discover this change causes some crashes, and will be addressing them before this goes into the final release.

The crashes were related to accessing the freed memory after it's been released, and my hunch is that it's related to the async queue of the image decoding, but need to dig a bit deeper to find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants