[0.67] HermesRuntimeHolder and DevSettings leaked on calling ReactNativeHost::UnloadInstance

[nichamp]

Problem Description

If one attempts to destruct a ReactRootView and/or call ReactNativeHost::UnloadInstance, a significant amount of memory (several megabytes with either Debug or Release RNW) is not freed and accumulate each time one repeats this. This is indicative of memory leaks and can be proven by adding breakpoints to destructors in components like HermesRuntimeHolder.

In my preliminary investigation I can see the problem is that DevSettings holds std::shared_ptr<jsi::RuntimeHolderLazyInit> jsiRuntimeHolder and HermesRuntimeHolder unlike ChakraRuntimeHolder has a std::shared_ptr<facebook::react::DevSettings> m_devSettings. With a simple fix to change the latter to std::weak_ptr, it will get freed properly on calling UnloadInstance and memory usage appears to stop increasing significantly on reload.

However, there is a second issue: If one doesn't explicit call ReactNativeHost::UnloadInstance and merely frees the last pointer to ReactRootView, neither it or ReactNativeHost (or the JS runtime of course) will destruct either. So there is also a circular references with those objects. This is likely because UIManager::removeRootView doesn't get called to free the strong ref to ReactRootView held be a shadow node and consequently, its ReactNative::ReactNativeHost m_reactNativeHost doesn't get cleared. I am not certain where the best blace to break the circular reference chain with a winrt::weak_ref or etc. would be. If is isn't possible, documentation and examples should make it very clear that the expectation is to call UnloadInstance.

Steps To Reproduce

1. Add a HermesRuntimeHolder destructor and add a breakpoint.
2. Modify code to attempt to call ReactNativeHost::UnloadInstance and remove ReactRootView from the XAML tree.
3. Observe the destructor is not called.

Expected Results

All RNW and JSI resources should be freed on destructing the last references to the WinRT COM pointers (e.g. by removing from XAML tree) or by explicitly attempting to destruct with UnloadInstance

If possible, RNW should have automated tests to try to catch memory leaks by consecutively reloading one or more different bundles.

CLI version

6.3.1

Environment

System:
    OS: Windows 10 10.0.19042
    CPU: (24) x64 AMD Ryzen Threadripper PRO 3945WX 12-Cores
    Memory: 39.46 GB / 63.86 GB
  Binaries:
    Node: 16.14.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.15 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 8.3.1 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK:
      AllowDevelopmentWithoutDevLicense: Enabled
      AllowAllTrustedApps: Enabled
      Versions: 10.0.19041.0, 10.0.22000.0
  IDEs:
    Android Studio: Not Found
    Visual Studio: 16.11.32126.315 (Visual Studio Enterprise 2019)
  Languages:
    Java: Not Found
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2
    react-native: 0.67.1
    react-native-windows: 0.67.0
  npmGlobalPackages:
    *react-native*: Not Found

Target Platform Version

10.0.19041

Target Device(s)

Desktop

Visual Studio Version

Visual Studio 2019

Build Configuration

Release

Snack, code example, screenshot, or link to a repository

No response

[chrisglein]

Potential candidate for backport to 0.68 depending on the fix. But definitely a high priority fix.

[jonthysell]

@vmoroz Is this something you could look into?

[jonthysell]

@vmoroz Any ideas here?

[FahadMohammedFiroz]

Is this bug resolved?

[ghmous-github]

is this fixed?

[TatianaKapos]

This has been bumped a few times, I don't think this needs to be tied to a specific release but seems like a high priority bug. Do we want to move this to Next?