JSRefreshRateMonitor memory leak on IOS

[voloshink]

Describe the bug

When startMonitoring is called in JSRefreshRateMonitor, an existing JSRefreshRateListener is not properly invalidated before a new one is created. This means each time startMonitoring is called, more listeners are being created.

Reproduction steps

We use Electrode miniapps meaning it is common for us to create and destroy new miniapps. Each time a new miniapp is created, more refresh monitors are created in memory, that will stay behind even as the miniapps are destroyed.

To give a toy example:

let appCount = 200
        for i in Range(1...appCount) {
            let _ = ElectrodeReactNative
                .sharedInstance()
                .miniApp(
                    withName: "some_name",
                    properties: properties,
                    overlay: false,
                    sizeFlexibility: RCTRootViewSizeFlexibility.none.rawValue,
                    delegate: nil
                )
        }

Here we create 200 miniapps. we do not store them anywhere, so we should expect that no additional memory should be used. However, if we check the memory graph, we will see that 200 additional refresh rate listeners are still in memory.

SDK logs

No response

Expected behavior

No response

Affected SDK versions

2.1.0

Latest working SDK version

N/A

Did you confirm if the latest SDK version fixes the bug?

Yes

Integration Methods

Yarn

React Native Version

No response

Package.json Contents

No response

iOS Setup

No response

Android Setup

No response

Device Information

No response

Other relevant information

No response

[louiszawadzki]

Hi @voloshink, thanks a lot for the clear report and for opening a PR!

There are a few things I'd like to assess before moving on with this as I'm not too familiar with Electrode.
Each JSRefreshRateListener is tied to a jsQueue - that is the thread running the JS code for your mini-app.

In the ideal state you described, we would only keep the last JSRefreshRateListener created, which means we'll only monitor one thread.
This raises the following questions:

• Are all the mini-apps running on the same thread? In which case we could also keep the first JSRefreshRateListener created, which will be a bit more performant.
• If not, is it possible to have multiple mini-apps running at the same time within an Electrode app? If that's the case, the user might go back to a previously monitored mini-app without initializing the SDK once again in it and so we'd end up monitoring the wrong JS thread.

If you don't have all the answers to these questions, no worries we'll dig into it anyway :) But if you do that would save us some time in our research and understanding!

Thanks a lot again!

[LcTwisk]

Hi @louiszawadzki

Thanks for your quick response!
All MiniApps are running on the same JS thread, so using the first JSRefreshRateListener should be fine 👍

[maxep]

Fixed by #633

Thanks again @voloshink for your contribution!

[voloshink]

Thanks for looking into it 👍