airbnb / mavericks

Mavericks: Android on Autopilot
https://airbnb.io/mavericks/
Apache License 2.0
5.85k stars 499 forks source link

MockableMavericksView Memory Leak #640

Closed safa007 closed 2 years ago

safa007 commented 2 years ago

My team has been noticing that any screen that implements MockableMavericksView has been leaking. So I replaced MockableMavericksView with MavericksView and the memory leaks stopped happening.

All of the leaks look the exact same, pointing to MockStateHolder.delegateInfoMap. And the leaks are happening with all screens - even those that have no-op implementations of provideMocks.

Is this a known issue?

elihart commented 2 years ago

Thanks for bringing this up.

Note first that this is the MockStateHolder used by MockableMavericks, which should only be used in dev, so no leaks will be in prod.

The MockStateHolder requires the mock to be cleared once you're done using it, via functions like clearAllMocks or clearMock.

When calling getMockVariants the returned MockedViewProvider provides a MockedView which in turn as a lambda cleanupMockState that should be used to clean up the mock reference once the mock is done being used.

Depending on how you're using the mocks its up to your set up to clear the references properly. but if you don't its only a leak in dev builds.

safa007 commented 2 years ago

Hmm, getMockVariants is only being used by our IntegrationTestActivity to launch the Fragment with various mock variants. We have our BaseFragment implementing MockableMavericksView because we assumed that provideMocks would just provide EmptyMocks if a fragment had no mocks and there would be no issues. Is there a way to clearAllMocks() from our BaseFragment in this case?

elihart commented 2 years ago

In OnDestroy of your base fragment you could call MockableMavericks.mockStateHolder#clearMock(fragment) to prevent that fragment from leaking.

probably a better long term solution, it looks like MockStateHolder.addViewModelDelegate could use the lifecycle of the MockableMavericksView to register a listener for destroy to know when to automatically remove the references.

safa007 commented 2 years ago

That seems to work 👍 Thanks so much!