Skip to content

Fix memory leak in location component. #1093

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.

Sign up for GitHub

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

Merged
Chaoba merged 2 commits into from
Feb 21, 2022
Merged

Fix memory leak in location component. #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a62cc7e
Fix memory leak in location component.
Jan 28, 2022
af258e8
Remove location compass
Feb 7, 2022
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

Mapbox welcomes participation and contributions from everyone.

## main
## Bug fixes 🐞
* Fix memory leak in location component. ([#1093](https://github.com/mapbox/mapbox-maps-android/pull/1093))
Copy link
Contributor

@yunikkk yunikkk Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chaoba could you explain how was this reproduced?

Copy link
Contributor Author

@Chaoba Chaoba Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open and close LocationComponentActivity several times could reproduce it.

Copy link
Contributor

@yunikkk yunikkk Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I mean - aren't we hiding the actual problem here? Why isn't unRegisterLocationConsumer called here? Shouldn't lifecycle call it always?

Copy link
Contributor Author

@Chaoba Chaoba Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we do call unRegisterLocationConsumer, but still have this issue.

Copy link
Contributor

@yunikkk yunikkk Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how is this possible at all?

Copy link
Contributor Author

@Chaoba Chaoba Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can only be reproduced with compass engine, guess locationEngine holds the reference of location plugin as well as sensorManager.

Copy link
Contributor

@yunikkk yunikkk Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't unsubscribe from compass engine somehow? I'd say compass is also very common component so if memory leaks were reproducible it would leak in many apps out there

Copy link
Contributor Author

@Chaoba Chaoba Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause should be locationEngine holds the reference the callback during request location update even we remove it. And then the location compass is kept and causes the leak.

Only request location update or have location compass can't reproduce this issue, must combine these two together.

Copy link
Contributor

@yunikkk yunikkk Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've digged it a bit and it seems found the root cause. Thing is - we use Activity context to create SensorManager instance at CompassEngine.

And callbacks, that we add / remove to the LocationEngine indeed are retained, even after remove is called.

So the issue became visible with the CompassEngine addition, since it holds SensorManager, which holds Context, which holds (and is) Activity.
It seems to me that we should use application context everywhere, so that in future such problems won't be possible at all.

PS
Context for the issue is interesting as well. It seems that our implementation of LocationEngine in mapbox-android-core lacks certain logic cleaning the callback or otherwise such issues wouldn't be possible as well.

The issue with LocationEngine keeping its listeners even after removeLocationUpdates has very long story, raised here, reworked here and followed with the documented recommendation to avoid leaks using WeakReferences here published here.
I'll cut a ticket in the lib repo to apply the fix as well.

Copy link
Contributor

@yunikkk yunikkk Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapbox/mapbox-events-android#562


# 10.4.0-beta.1

## Features ✨ and improvements 🏁
Expand Down
47 changes: 23 additions & 24 deletions ...mponent/src/main/java/com/mapbox/maps/plugin/locationcomponent/DefaultLocationProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,30 +42,8 @@ class DefaultLocationProvider @VisibleForTesting(otherwise = PRIVATE) internal c
private lateinit var runnable: Runnable
private var updateDelay = INIT_UPDATE_DELAY

private val locationEngineCallback = object : LocationEngineCallback<LocationEngineResult> {
/**
* Invoked when new data available.
*
* @param result updated data.
*/
override fun onSuccess(result: LocationEngineResult) {
result.lastLocation?.let {
notifyLocationUpdates(it)
}
}

/**
* Invoked when engine exception occurs.
*
* @param exception
*/
override fun onFailure(exception: Exception) {
Logger.e(
TAG,
"Failed to obtain location update: $exception"
)
}
}
private val locationEngineCallback: LocationEngineCallback<LocationEngineResult> =
CurrentLocationEngineCallback(this)

@VisibleForTesting(otherwise = PRIVATE)
internal val locationCompassListener =
Expand Down Expand Up @@ -181,6 +159,27 @@ class DefaultLocationProvider @VisibleForTesting(otherwise = PRIVATE) internal c
}
}

private class CurrentLocationEngineCallback(locationProvider: DefaultLocationProvider) :
LocationEngineCallback<LocationEngineResult> {
private val locationProviderWeakReference: WeakReference<DefaultLocationProvider> =
WeakReference(locationProvider)

override fun onSuccess(result: LocationEngineResult) {
result.lastLocation?.let { location ->
locationProviderWeakReference.get()?.let {
it.notifyLocationUpdates(location)
}
}
}

override fun onFailure(exception: Exception) {
Logger.e(
TAG,
"Failed to obtain location update: $exception"
)
}
}

private companion object {
private const val TAG = "MapboxLocationProvider"
private const val INIT_UPDATE_DELAY = 100L
Expand Down