Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Avoid leaking compassView context through animator #13743

Merged
merged 1 commit into from
Jan 17, 2019
Merged

Avoid leaking compassView context through animator #13743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,11 @@ public void onDestroy() {
mapCallback.onDestroy();
initialRenderCallback.onDestroy();

if (compassView != null) {
// avoid leaking context through animator #13742
compassView.resetAnimation();
Copy link
Member

Choose a reason for hiding this comment

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

Thinking in terms of https://github.com/mapbox/mapbox-gl-native/issues/13152, maybe we can use compassView.setEnabled(false) here to limit the amount of publicly exposed methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

the enabled flag is also used for saving instance state:

    outState.putBoolean(MapboxConstants.STATE_COMPASS_ENABLED, isCompassEnabled());

Since we do not control the execution order for OnDestroy vs OnSaveInstance state. We could end up in a situation where the compass remains disabled when recreating the underlying context while it initially was enabled.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that onSaveInstanceState is always going to be called before onDestroy, this is the takeaway I get from the docs. That said, I don't have strong feelings about this, so :shipit:.

Copy link
Member Author

Choose a reason for hiding this comment

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

For normal cases as an Activity yes, but since our MapView#OnDestroy is called from Fragment#onDestroyView it can occur that MapView#OnSaveInstanceState is called after MapView#OnDestroy. For example destroying an Activity while you have a MapView fragment in the backstack.

Copy link
Member

Choose a reason for hiding this comment

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

Good point 👍

}

if (mapboxMap != null) {
mapboxMap.onDestroy();
}
Expand Down
2 changes: 1 addition & 1 deletion ...droid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/widgets/CompassView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public void isAnimating(boolean isAnimating) {
this.isAnimating = isAnimating;
}

private void resetAnimation() {
public void resetAnimation() {
if (fadeAnimator != null) {
fadeAnimator.cancel();
}
Expand Down