rainbow icon indicating copy to clipboard operation
rainbow copied to clipboard

Published 20 hours ago • verified publisher icon rainbow-me

Fix memory leak in cool modals

Open osdnk opened this issue 1 year ago • 3 comments

Fixes RNBW-#### Figma link (if any):

This was a regression caused by https://github.com/rainbow-me/rainbow/pull/1142

However, the original bug was already fixed in reanimated so this fix is not needed. Also, I expect that this particular line was unintentional anyway

See the bug here (sorry for the image quality, this is the screenshot for the video): before https://streamable.com/em0d2t image

after: https://streamable.com/nvgoxe image

Observe the memory chart changing:

Before: https://streamable.com/e0kxpe After:https://streamable.com/0vkpty

Before after 1 minute image

After fix after almost 2 minutes image

(see recording for the documentation of the more precise experiments)

There is still something broken (maybe this is just JS logic), but definitely, the derivative of the growth decreased so it's an improvement.

What changed (plus any additional context for devs)

I restored commented out line cause setting controller to null. This is important because in this structure (that is a bit incorrect, but ok) controller is holding view (RNCMScreen and RNCMScreeView) and vice versa. This is not java GC, but ARC (not having GC to be precise), which means that as long as the memory holds the reference for the object, it doesn't get deallocated. Here, one object keeps ref to another and the other way.

I fixed this in a way this was supposed to be fixed originally.

This was previously not discovered vie leak tool because of the fact that this is not an alienated object not being deallocated but the whole structure that was most likely too big to be noticed to be separated.

osdnk avatar Jul 27 '22 07:07 osdnk

/rebase

terrysahaidak avatar Aug 01 '22 09:08 terrysahaidak

/rebase

terrysahaidak avatar Aug 08 '22 18:08 terrysahaidak

/rebase

tchayen avatar Aug 24 '22 11:08 tchayen

@estrattonbailey Did you have any intentions with closing and reopening? I will assign myself to that to see if we can merge it

jkadamczyk avatar Oct 17 '22 20:10 jkadamczyk

APP-1 Take Memory leak in cool modals PR to the finish line

It was opened by Michał and needs some action to get it closed

linear[bot] avatar Oct 17 '22 20:10 linear[bot]

It has some issues, we shouldn't merge it in this state. Gonna create linear tickets for that to address the memory leaks and link this PR

The issues are presented below, it happens for send flow of all assets

https://user-images.githubusercontent.com/16062886/196479668-2ba7e3ad-b193-4837-a906-a97b85370121.mov

jkadamczyk avatar Oct 18 '22 15:10 jkadamczyk