Skip to content

Fix memory leaks by clearing view bindings and references in fragments #6347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,12 @@ public void onBackStackChanged() {

}

@Override
public void onDestroyView() {
super.onDestroyView();
binding = null;
}

@Override
public void onDestroy() {
super.onDestroy();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ class BookmarkLocationsFragment : DaggerFragment() {
}
}

override fun onDestroyView() {
super.onDestroyView()
binding = null
}

override fun onDestroy() {
super.onDestroy()
// Make sure to null out the binding to avoid memory leaks
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ public void onStop() {
controller.stop();
}

@Override
public void onDestroyView() {
super.onDestroyView();
binding = null;
}

@Override
public void onDestroy() {
super.onDestroy();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,14 +685,23 @@ class ContributionsFragment : CommonsDaggerSupportFragment(), FragmentManager.On
override fun onDestroy() {
try {
compositeDisposable.clear()
// Remove child fragment safely
contributionsListFragment?.let {
childFragmentManager.beginTransaction()
.remove(it)
.commitAllowingStateLoss()
}
childFragmentManager.removeOnBackStackChangedListener(this)
locationManager!!.unregisterLocationManager()
locationManager!!.removeLocationListener(this)
super.onDestroy()
locationManager?.unregisterLocationManager()
locationManager?.removeLocationListener(this)
// Nullify locationManager to prevent leaks
locationManager = null
} catch (exception: IllegalArgumentException) {
Timber.e(exception)
} catch (exception: IllegalStateException) {
Timber.e(exception)
} finally {
super.onDestroy()
}
}

Expand Down Expand Up @@ -757,7 +766,9 @@ class ContributionsFragment : CommonsDaggerSupportFragment(), FragmentManager.On

override fun onDestroyView() {
super.onDestroyView()
presenter!!.onDetachView()
presenter?.onDetachView()
binding = null
contributionsListFragment = null
}

override fun notifyDataSetChanged() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,11 @@ after opening the app.
quizChecker!!.cleanup()
locationManager!!.unregisterLocationManager()
// Remove ourself from hashmap to prevent memory leaks
try {
locationManager?.unregisterLocationManager()
} catch (e: Exception) {
Timber.e(e, "Error while cleaning up locationManager")
}
locationManager = null
super.onDestroy()
}
Expand Down
134 changes: 81 additions & 53 deletions app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,13 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C

private var initialListTop: Int = 0

/**
* Holds the view binding reference for this fragment.
* Use bindingOrNull for safe access after view destruction.
*/
private var _binding: FragmentMediaDetailBinding? = null
private val binding get() = _binding!!
private val bindingOrNull get() = _binding

private var descriptionHtmlCode: String? = null

Expand Down Expand Up @@ -340,25 +345,27 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C

handleBackEvent(view)

//set onCLick listeners
binding.mediaDetailLicense.setOnClickListener { onMediaDetailLicenceClicked() }
binding.mediaDetailCoordinates.setOnClickListener { onMediaDetailCoordinatesClicked() }
binding.sendThanks.setOnClickListener { sendThanksToAuthor() }
binding.dummyCaptionDescriptionContainer.setOnClickListener { showCaptionAndDescription() }
binding.mediaDetailImageView.setOnClickListener {
launchZoomActivity(
binding.mediaDetailImageView
)
/**
* Safely sets click listeners on media detail UI elements using bindingOrNull.
* Prevents potential crashes caused by view being destroyed during delayed callbacks.
*/
bindingOrNull?.let { binding ->
binding.mediaDetailLicense.setOnClickListener { onMediaDetailLicenceClicked() }
binding.mediaDetailCoordinates.setOnClickListener { onMediaDetailCoordinatesClicked() }
binding.sendThanks.setOnClickListener { sendThanksToAuthor() }
binding.dummyCaptionDescriptionContainer.setOnClickListener { showCaptionAndDescription() }
binding.mediaDetailImageView.setOnClickListener {
launchZoomActivity(binding.mediaDetailImageView)
}
binding.categoryEditButton.setOnClickListener { onCategoryEditButtonClicked() }
binding.depictionsEditButton.setOnClickListener { onDepictionsEditButtonClicked() }
binding.seeMore.setOnClickListener { onSeeMoreClicked() }
binding.mediaDetailAuthor.setOnClickListener { onAuthorViewClicked() }
binding.nominateDeletion.setOnClickListener { onDeleteButtonClicked() }
binding.descriptionEdit.setOnClickListener { onDescriptionEditClicked() }
binding.coordinateEdit.setOnClickListener { onUpdateCoordinatesClicked() }
binding.copyWikicode.setOnClickListener { onCopyWikicodeClicked() }
}
binding.categoryEditButton.setOnClickListener { onCategoryEditButtonClicked() }
binding.depictionsEditButton.setOnClickListener { onDepictionsEditButtonClicked() }
binding.seeMore.setOnClickListener { onSeeMoreClicked() }
binding.mediaDetailAuthor.setOnClickListener { onAuthorViewClicked() }
binding.nominateDeletion.setOnClickListener { onDeleteButtonClicked() }
binding.descriptionEdit.setOnClickListener { onDescriptionEditClicked() }
binding.coordinateEdit.setOnClickListener { onUpdateCoordinatesClicked() }
binding.copyWikicode.setOnClickListener { onCopyWikicodeClicked() }

binding.fileUsagesComposeView.apply {
setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed)
setContent {
Expand Down Expand Up @@ -404,14 +411,19 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
/**
* Gets the height of the frame layout as soon as the view is ready and updates aspect ratio
* of the picture.
* Updated to use bindingOrNull inside delayed callbacks to prevent crashes
* when view is destroyed before post/postDelayed is executed.
*/
view.post{
val width = binding.mediaDetailScrollView.width
view.post {
val safeBinding = bindingOrNull ?: return@post
val width = safeBinding.mediaDetailScrollView.width
if (width > 0) {
frameLayoutHeight = binding.mediaDetailFrameLayout.measuredHeight
frameLayoutHeight = safeBinding.mediaDetailFrameLayout.measuredHeight
updateAspectRatio(width)
} else {
view.postDelayed({ updateAspectRatio(binding.root.width) }, 1)
view.postDelayed({
bindingOrNull?.let { updateAspectRatio(it.root.width) }
}, 1)
}
}

Expand Down Expand Up @@ -526,45 +538,56 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
binding.sendThanks.visibility = View.VISIBLE
}

binding.mediaDetailScrollView.viewTreeObserver.addOnGlobalLayoutListener(
/**
* Sets a one-time global layout listener to safely access view dimensions.
* Uses bindingOrNull to avoid crash if the fragment view is destroyed
* before the callback is executed.
*/
bindingOrNull?.mediaDetailScrollView?.viewTreeObserver?.addOnGlobalLayoutListener(
object : OnGlobalLayoutListener {
override fun onGlobalLayout() {
val safeBinding = bindingOrNull ?: return
if (context == null) {
return
}
binding.mediaDetailScrollView.viewTreeObserver.removeOnGlobalLayoutListener(
safeBinding.mediaDetailScrollView.viewTreeObserver.removeOnGlobalLayoutListener(
this
)
oldWidthOfImageView = binding.mediaDetailScrollView.width
if (media != null) {
oldWidthOfImageView = safeBinding.mediaDetailScrollView.width
media?.filename?.let {
displayMediaDetails()
fetchFileUsages(media?.filename!!)
fetchFileUsages(it)
}
}
}
)
binding.progressBarEdit.visibility = View.GONE
bindingOrNull?.progressBarEdit?.visibility = View.GONE
}

override fun onConfigurationChanged(newConfig: Configuration) {
super.onConfigurationChanged(newConfig)
binding.mediaDetailScrollView.viewTreeObserver.addOnGlobalLayoutListener(
/**
* Sets a global layout listener to update the aspect ratio when device configuration changes.
* Uses bindingOrNull to avoid crashes in case the view is destroyed before callback.
*/
bindingOrNull?.mediaDetailScrollView?.viewTreeObserver?.addOnGlobalLayoutListener(
object : OnGlobalLayoutListener {
override fun onGlobalLayout() {
val safeBinding = bindingOrNull ?: return
/**
* We update the height of the frame layout as the configuration changes.
*/
binding.mediaDetailFrameLayout.post {
frameLayoutHeight = binding.mediaDetailFrameLayout.measuredHeight
updateAspectRatio(binding.mediaDetailScrollView.width)
safeBinding.mediaDetailFrameLayout.post {
frameLayoutHeight = safeBinding.mediaDetailFrameLayout.measuredHeight
updateAspectRatio(safeBinding.mediaDetailScrollView.width)
}
if (binding.mediaDetailScrollView.width != oldWidthOfImageView) {
if (safeBinding.mediaDetailScrollView.width != oldWidthOfImageView) {
if (newWidthOfImageView == 0) {
newWidthOfImageView = binding.mediaDetailScrollView.width
newWidthOfImageView = safeBinding.mediaDetailScrollView.width
updateAspectRatio(newWidthOfImageView)
}
binding.mediaDetailScrollView.viewTreeObserver.removeOnGlobalLayoutListener(
this
safeBinding.mediaDetailScrollView.viewTreeObserver.removeOnGlobalLayoutListener(
this
)
}
}
Expand Down Expand Up @@ -613,7 +636,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
}

private fun onMediaRefreshed(media: Media) {
media.categories = this.media!!.categories
media.categories = this.media?.categories
this.media = media
setTextFields(media)
compositeDisposable.addAll(
Expand All @@ -627,29 +650,30 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
}

private fun onDiscussionLoaded(discussion: String) {
binding.mediaDetailDisc.text = prettyDiscussion(discussion.trim { it <= ' ' })
bindingOrNull?.mediaDetailDisc?.text = prettyDiscussion(discussion.trim { it <= ' ' })
}

private fun onDeletionPageExists(deletionPageExists: Boolean) {
if (getUserName(requireContext()) == null && getUserName(requireContext()) != media!!.author) {
binding.nominateDeletion.visibility = View.GONE
binding.nominatedDeletionBanner.visibility = View.GONE
val safeBinding = bindingOrNull ?: return

if (getUserName(requireContext()) == null && getUserName(requireContext()) != media?.author) {
safeBinding.nominateDeletion.visibility = View.GONE
safeBinding.nominatedDeletionBanner.visibility = View.GONE
} else if (deletionPageExists) {
if (applicationKvStore.getBoolean(
String.format(NOMINATING_FOR_DELETION_MEDIA, media!!.imageUrl), false
String.format(NOMINATING_FOR_DELETION_MEDIA, media?.imageUrl), false
)
) {
applicationKvStore.remove(
String.format(NOMINATING_FOR_DELETION_MEDIA, media!!.imageUrl)
String.format(NOMINATING_FOR_DELETION_MEDIA, media?.imageUrl)
)
binding.progressBarDeletion.visibility = View.GONE
safeBinding.progressBarDeletion.visibility = View.GONE
}
binding.nominateDeletion.visibility = View.GONE

binding.nominatedDeletionBanner.visibility = View.VISIBLE
safeBinding.nominateDeletion.visibility = View.GONE
safeBinding.nominatedDeletionBanner.visibility = View.VISIBLE
} else if (!isCategoryImage) {
binding.nominateDeletion.visibility = View.VISIBLE
binding.nominatedDeletionBanner.visibility = View.GONE
safeBinding.nominateDeletion.visibility = View.VISIBLE
safeBinding.nominatedDeletionBanner.visibility = View.GONE
}
}

Expand Down Expand Up @@ -784,14 +808,18 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C
}
}

/**
* Clears binding and removes layout listener to prevent memory leaks
* or crashes from delayed UI callbacks after the view is destroyed.
*/
override fun onDestroyView() {
if (layoutListener != null && view != null) {
requireView().viewTreeObserver.removeGlobalOnLayoutListener(layoutListener) // old Android was on crack. CRACK IS WHACK
layoutListener?.let {
view?.viewTreeObserver?.removeOnGlobalLayoutListener(it)
layoutListener = null
}

compositeDisposable.clear()

_binding = null
super.onDestroyView()
}

Expand Down Expand Up @@ -2263,4 +2291,4 @@ fun FileUsagesContainer(
}

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ class ProfileActivity : BaseActivity() {
public override fun onDestroy() {
super.onDestroy()
compositeDisposable.clear()
// Nullify fragment references to avoid memory leaks
if (::achievementsFragment.isInitialized) achievementsFragment.setHasOptionsMenu(false)
if (::leaderboardFragment.isInitialized) leaderboardFragment.setHasOptionsMenu(false)
contributionsFragment = null
}

override fun onCreateOptionsMenu(menu: Menu): Boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,8 @@ class UploadActivity : BaseActivity(), UploadContract.View, UploadBaseFragment.C
uploadCategoriesFragment!!.callback = null
}
onBackPressedCallback.remove()
locationManager?.unregisterLocationManager()
UploadMediaPresenter.presenterCallback = null // Clearing reference
}

/**
Expand Down
10 changes: 0 additions & 10 deletions app/src/main/res/values-x-invalidLanguageCode/error.xml

This file was deleted.

4 changes: 2 additions & 2 deletions app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@
<string name="statistics_thanks">Thanks Received</string>
<string name="statistics_featured">Featured Images</string>
<string name="statistics_wikidata_edits">Images via \"Nearby Places\"</string>
<string name="level">Level %d</string>
<string name="profileLevel">%s (Level %s)</string>
<string name="level" formatted="false">Level %d</string>
<string name="profileLevel" formatted="false">%s (Level %s)</string>
<string name="images_uploaded">Images Uploaded</string>
<string name="image_reverts">Images Not Reverted</string>
<string name="images_used_by_wiki">Images Used</string>
Expand Down