Fix memory leaks by clearing view bindings and references in fragments #6347
Conversation
|
Hi @nicolas-raoul, kindly review this PR which fixes memory leaks by properly clearing bindings and references in multiple fragments. All checks have passed. Thanks! 🙌 |
There was a problem hiding this comment.
Thanks! 🙂
These steps lead to a crash, would you mind checking? On main this triggers no crash.
screen-20250613-222524.mp4
USER_COMMENT=
APP_VERSION_CODE=1052
APP_VERSION_NAME=5.4.1-debug-devjadiya-verify-clean
ANDROID_VERSION=16
PHONE_MODEL=Pixel 9 Pro
STACK_TRACE=java.lang.NullPointerException
at fr.free.nrw.commons.media.MediaDetailFragment.getBinding(MediaDetailFragment.kt:194)
at fr.free.nrw.commons.media.MediaDetailFragment.onCreateView$lambda$15(MediaDetailFragment.kt:409)
at fr.free.nrw.commons.media.MediaDetailFragment.$r8$lambda$w5izUOUtPup1CyA_YKiR7Kxd4xE(Unknown Source:0)
at fr.free.nrw.commons.media.MediaDetailFragment$$ExternalSyntheticLambda2.run(D8$$SyntheticClass:0)
at android.os.Handler.handleCallback(Handler.java:1041)
at android.os.Handler.dispatchMessage(Handler.java:103)
at android.os.Looper.dispatchMessage(Looper.java:315)
at android.os.Looper.loopOnce(Looper.java:251)
at android.os.Looper.loop(Looper.java:349)
at android.app.ActivityThread.main(ActivityThread.java:9052)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:593)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:929)
IS_SILENT=false
REPORT_ID=68294b83-1de8-4cf0-8c79-cc7908d782e6
USER_CRASH_DATE=2025-06-13T22:22:40.218+09:00
USER_EMAIL=
|
Apart from the crash above, the app is working fine, I used it for 3 days. :-) |
… delayed callbacks
|
Hi @nicolas-raoul 😀, I've tested the changes on both debug and production flavors, including the Explore → Featured → Media navigation flow. The crash no longer occurs, and the app remains stable. Please feel free to review the latest commit — I'm happy to make any further improvements if needed. |
There was a problem hiding this comment.
Using the latest commit, still crashing after viewing a few pictures:
APP_VERSION_CODE=1052
APP_VERSION_NAME=5.4.1-debug-devjadiya-verify-clean
ANDROID_VERSION=16
PHONE_MODEL=Pixel 9 Pro
STACK_TRACE=java.lang.NullPointerException
at fr.free.nrw.commons.media.MediaDetailFragment.getBinding(MediaDetailFragment.kt:198)
at fr.free.nrw.commons.media.MediaDetailFragment.access$getBinding(MediaDetailFragment.kt:136)
at fr.free.nrw.commons.media.MediaDetailFragment$aspectRatioListener$1.onIntermediateImageSet(MediaDetailFragment.kt:739)
at fr.free.nrw.commons.media.MediaDetailFragment$aspectRatioListener$1.onIntermediateImageSet(MediaDetailFragment.kt:736)
at com.facebook.drawee.controller.AbstractDraweeController.onNewResultInternal(AbstractDraweeController.java:578)
at com.facebook.drawee.controller.AbstractDraweeController.access$000(AbstractDraweeController.java:44)
at com.facebook.drawee.controller.AbstractDraweeController$1.onNewResultImpl(AbstractDraweeController.java:503)
at com.facebook.datasource.BaseDataSubscriber.onNewResult(BaseDataSubscriber.java:46)
at com.facebook.datasource.AbstractDataSource$1.run(AbstractDataSource.java:176)
at android.os.Handler.handleCallback(Handler.java:1041)
at android.os.Handler.dispatchMessage(Handler.java:103)
at android.os.Looper.dispatchMessage(Looper.java:315)
at android.os.Looper.loopOnce(Looper.java:251)
at android.os.Looper.loop(Looper.java:349)
at android.app.ActivityThread.main(ActivityThread.java:9041)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:593)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:929)
Please fix and test by opening a dozen pictures in Explore. Thanks!
Done with the changes, please check. Retested Explore > Featured, mobile uploads. Images load fine now, and no crashes on open/swipe. |
There was a problem hiding this comment.
Much less frequent, but I am still getting this crash after viewing around 30 pictures in Explore>Featured:
APP_VERSION_NAME=5.4.1-debug-devjadiya-verify-clean
ANDROID_VERSION=16
PHONE_MODEL=Pixel 9 Pro
STACK_TRACE=java.lang.NullPointerException: Attempt to read from field 'androidx.viewpager.widget.ViewPager fr.free.nrw.commons.databinding.FragmentMediaDetailPagerBinding.mediaDetailsPager' on a null object reference in method 'void fr.free.nrw.commons.media.MediaDetailPagerFragment.onSaveInstanceState(android.os.Bundle)'
at fr.free.nrw.commons.media.MediaDetailPagerFragment.onSaveInstanceState(MediaDetailPagerFragment.java:172)
at androidx.fragment.app.Fragment.performSaveInstanceState(Fragment.java:3151)
at androidx.fragment.app.FragmentStateManager.saveBasicState(FragmentStateManager.java:683)
at androidx.fragment.app.FragmentStateManager.saveState(FragmentStateManager.java:649)
at androidx.fragment.app.FragmentStore.saveActiveFragments(FragmentStore.java:177)
at androidx.fragment.app.FragmentManager.saveAllState(FragmentManager.java:2655)
at androidx.fragment.app.Fragment.performSaveInstanceState(Fragment.java:3153)
at androidx.fragment.app.FragmentStateManager.saveBasicState(FragmentStateManager.java:683)
at androidx.fragment.app.FragmentStateManager.saveState(FragmentStateManager.java:649)
at androidx.fragment.app.FragmentStore.saveActiveFragments(FragmentStore.java:177)
at androidx.fragment.app.FragmentManager.saveAllState(FragmentManager.java:2655)
at androidx.fragment.app.Fragment.performSaveInstanceState(Fragment.java:3153)
at androidx.fragment.app.FragmentStateManager.saveBasicState(FragmentStateManager.java:683)
at androidx.fragment.app.FragmentStateManager.saveState(FragmentStateManager.java:649)
at androidx.fragment.app.FragmentStore.saveActiveFragments(FragmentStore.java:177)
at androidx.fragment.app.FragmentManager.saveAllState(FragmentManager.java:2655)
at androidx.fragment.app.FragmentController.saveAllState(FragmentController.java:152)
at androidx.fragment.app.FragmentActivity$1.saveState(FragmentActivity.java:133)
at androidx.savedstate.SavedStateRegistry.performSave(SavedStateRegistry.kt:247)
at androidx.savedstate.SavedStateRegistryController.performSave(SavedStateRegistryController.kt:81)
at androidx.activity.ComponentActivity.onSaveInstanceState(ComponentActivity.kt:345)
at fr.free.nrw.commons.contributions.MainActivity.onSaveInstanceState(MainActivity.kt:360)
at android.app.Activity.performSaveInstanceState(Activity.java:2501)
at android.app.Instrumentation.callActivityOnSaveInstanceState(Instrumentation.java:1752)
at android.app.ActivityThread.callActivityOnSaveInstanceState(ActivityThread.java:6678)
at android.app.ActivityThread.callActivityOnStop(ActivityThread.java:6087)
at android.app.ActivityThread.performStopActivityInner(ActivityThread.java:6054)
at android.app.ActivityThread.handleStopActivity(ActivityThread.java:6117)
at android.app.servertransaction.StopActivityItem.execute(StopActivityItem.java:43)
at android.app.servertransaction.ActivityTransactionItem.execute(ActivityTransactionItem.java:63)
at android.app.servertransaction.TransactionExecutor.executeLifecycleItem(TransactionExecutor.java:169)
at android.app.servertransaction.TransactionExecutor.executeTransactionItems(TransactionExecutor.java:101)
at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:80)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2830)
at android.os.Handler.dispatchMessage(Handler.java:110)
at android.os.Looper.dispatchMessage(Looper.java:315)
at android.os.Looper.loopOnce(Looper.java:251)
at android.os.Looper.loop(Looper.java:349)
at android.app.ActivityThread.main(ActivityThread.java:9041)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:593)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:929)
Before the crash, I saw LeakCanary getting info from leaks, not sure if related.
Added null check in onSaveInstanceState() to prevent rare crash. |
|
I tried to reduce leakcanary errors related to media fragments. I tested Explore>Feature. On scrolls works fine but in swipe there is crash after 50-60 photos. I tried to investigate on the playstore variant; it crash too but after 100 photos. Could you please guide for possible fix as leak canary is not detecting anything now. Thanks. |
|
✅ Generated APK variants! |
|
I just tried the latest commit here. 🙂
|
|
Hey @devjadiya, Because I also once started working on memory leaks, but found that some references were intentionally leaked to fix some previous issue. However, some leaks worth fixing. There are long classes in the project, and we are using MVP architecture in some places. So, start small and target small isolated logic one by one. |
|
Thanks @rohit9625 for the thoughtful suggestion. You're absolutely right. I've also noticed that some older leaks seem to be workarounds for other lifecycle issues, which makes blindly nullifying everything risky. I’ve been targeting safe cleanup (bindings, listeners, adapters) without breaking core logic. But yes, after touching 10+ files and testing 500+ image flows, I realize many fragments are tightly coupled or doing too much. I’ll start taking a modular refactoring approach as you suggested — beginning with MediaDetailFragment.kt and slowly isolating logic where possible. It’ll take time, but it seems like the only sustainable path for both performance and stability. Appreciate your experience and direction here - helps me stay focused and not over-patch things. |
Fixes memory leaks in multiple fragments. These leaks were caused by improper cleanup of view bindings and references during fragment lifecycle events.
Changes made: