Tackle the block editor's memory leaks on WPAndroid

[hypest]

Promoting it to a ticket, @malinajirka has found evidence of memory leaks (see next comment as well) on WPAndroid related to the block editor and it'd be best to tackle them.

[mchowning]

:wave: @hypest . Do we need this ticket since https://github.com/wordpress-mobile/gutenberg-mobile/issues/2200 was transferred from WPAndroid to this repository? I'm thinking we can close this issue and keep that one. WDYT?

[hypest]

Hey @mchowning, 2200 seems to be about a crash that might be related to OOM or not, it's not clear yet, right? This new ticket is a more general one, to address the various memory leaks detected while investigating.

[mchowning]

This new ticket is a more general one, to address the various memory leaks detected while investigating.

That makes sense. Thanks for clearing that up for me!

[mkevins]

While investigating https://github.com/wordpress-mobile/gutenberg-mobile/issues/2200, I dug a little bit into the memory leak(s) reported there. I repeated the described steps (opening and closing the editor repeatedly) while profiling the app, and I took a heap dump after a garbage collection event. I confirmed that we are indeed leaking a few instances.

Suspected leaked classes

In the instance view, I observed a pattern which may be helpful. The depth of most instances (the ones likely to have been leaked) is 3 higher than the depth of a single instance which has a lower depth (likely the non-leaked instance, since the editor remained open while I took the heap dump). I first observed this pattern for EditPostActivity, but also found that it was present for other most of the other classes as well:

EditPostActivity instance view: depth 6 vs. 3

EditPostSettingsFragment: depth 7 vs. 4

HistoryListFragment: depth 11 vs. 8

SupportRequestManagerFragment: depth 12 vs. 9

EditPostPublishSettingsFragment: depth 11 vs. 8

ReportFragment: depth 12 vs. 9

Interestingly, GutenbergEditorFragment and GutenbergContainerFragment instances showed a difference of depth of 2 instead of 3:

GutenbergEditorFragment: depth 6 vs. 4

GutenbergContainerFragment: depth 7 vs. 5

Starting with EditPostActivity, I examined the reference graph to search for long-living references that would outlive the lifecycle of the activity. The first few in the list were implicit references (all of the first few - those with depth 5 - are coming from WPAndroidGlueCode), so probably good candidates for a leak (btw, there are many references to the activity at many depths). Picking mOnMediaEditorListener somewhat arbitrarily, I drilled further towards the GC root, and it seems that at depths 0 and 1, we have React Native code holding internal references to our RNReactNativeGutenbergBridgeModule, at depth 2 our module holds a reference to mGutenbergBridgeJS2Parent, and at depth 3, the inner anonymous implementation of this holds a reference to mOnMediaEditorListener (depth 4) from the enclosing outer scope within WPAndroidGlueCode.

References

https://github.com/wordpress-mobile/gutenberg-mobile/blob/a3a155d0053c75985960ae7ac8ecfaa2e3732efa/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java#L186

So, it seems that at least part of the problem is that all the listeners referenced within the instantiation of the anonymous inner class (new GutenbergBridgeJS2Parent) are kept alive by the implicit reference to the enclosing class (WPAndroidGlueCode), which in turn hold a reference to and outlive the lifecycle of GutenbergContainerFragment and GutenbergEditorFragment via their implicit reference to their enclosing scopes within the method call to attachToContainer.

https://github.com/wordpress-mobile/WordPress-Android/blob/dc41909f75cde15273d366494135ffb918e9a5d1/libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java#L348

from:

https://github.com/wordpress-mobile/WordPress-Android/blob/dc41909f75cde15273d366494135ffb918e9a5d1/WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java#L1723

One solution that comes to mind is to implement a counterpart method to attachToContainer (perhaps a detachFromContainer) to ensure these implicit references do not outlive their platform intended lifecycle. I don't yet have a great sense of how much effort that will require, but I do believe it is a cause of at least a large proportion of these leaks.

[geriux]

This is still relevant as I recently checked the memory usage increases when opening and closing the editor repeatedly and it doesn't free the memory back. I'll keep the High priority label.