From ad7898be5ab5815900cf7572639e00cff1a3aa9d Mon Sep 17 00:00:00 2001 From: Sarah Lensing Date: Fri, 12 May 2017 11:29:38 -0600 Subject: [PATCH] Fix Memory Leaks (#368) * upgrade to lost 2.3.0 snapshot * dont hold static reference to context in locationfactory * use application context in mapzenmanager * use weak reference in connection callbacks * use mock context that has mock application context in tests * cleanup listener in basic example * checkstyle --- core/build.gradle | 2 +- .../com/mapzen/android/core/MapzenManager.java | 2 +- .../android/graphics/OverlayManager.java | 15 +++++---------- .../android/location/LocationFactory.java | 7 ++----- .../mapzen/android/core/MapzenManagerTest.java | 12 ++++++------ .../android/graphics/MapInitializerTest.java | 6 +++++- .../android/location/LocationFactoryTest.java | 8 ++++---- .../lost/internal/FusedApiServiceUpdater.java | 13 +++++++++++++ .../PlaceAutocompletePresenterTest.java | 18 +++++++++--------- .../sdk/sample/BasicMapzenActivity.java | 5 +++++ 10 files changed, 51 insertions(+), 37 deletions(-) create mode 100644 mapzen-places-api/src/test/java/com/mapzen/android/lost/internal/FusedApiServiceUpdater.java diff --git a/core/build.gradle b/core/build.gradle index ba9234ab..4fd5f9a3 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -78,7 +78,7 @@ dependencies { compile "com.mapzen.tangram:tangram:$tangram_version" compile 'com.mapzen.android:pelias-android-sdk:1.1.0' - compile 'com.mapzen.android:lost:2.2.0' + compile 'com.mapzen.android:lost:2.3.0-SNAPSHOT' compile 'com.google.dagger:dagger:2.0' compile 'javax.annotation:javax.annotation-api:1.2' diff --git a/core/src/main/java/com/mapzen/android/core/MapzenManager.java b/core/src/main/java/com/mapzen/android/core/MapzenManager.java index f20b3901..4aeec53a 100644 --- a/core/src/main/java/com/mapzen/android/core/MapzenManager.java +++ b/core/src/main/java/com/mapzen/android/core/MapzenManager.java @@ -36,7 +36,7 @@ public class MapzenManager { */ public static MapzenManager instance(Context context) { if (instance == null) { - instance = new MapzenManager(context); + instance = new MapzenManager(context.getApplicationContext()); } return instance; diff --git a/core/src/main/java/com/mapzen/android/graphics/OverlayManager.java b/core/src/main/java/com/mapzen/android/graphics/OverlayManager.java index f4df9531..2dec79b1 100644 --- a/core/src/main/java/com/mapzen/android/graphics/OverlayManager.java +++ b/core/src/main/java/com/mapzen/android/graphics/OverlayManager.java @@ -19,6 +19,7 @@ import android.view.View; import android.widget.ImageButton; +import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -86,12 +87,6 @@ public class OverlayManager implements TouchInput.PanResponder, TouchInput.Rotat @Override public void onLocationChanged(Location location) { handleLocationChange(location); } - - @Override public void onProviderEnabled(String provider) { - } - - @Override public void onProviderDisabled(String provider) { - } }; View.OnClickListener compassExternalClickListener; @@ -116,11 +111,11 @@ public class OverlayManager implements TouchInput.PanResponder, TouchInput.Rotat private static class OverlayManagerConnectionCallbacks implements LostApiClient.ConnectionCallbacks { - private OverlayManager overlayManager; + private WeakReference overlayManager; @Override public void onConnected() { - if (overlayManager != null) { - overlayManager.enableLocationLayer(); + if (overlayManager != null && overlayManager.get() != null) { + overlayManager.get().enableLocationLayer(); } } @@ -128,7 +123,7 @@ private static class OverlayManagerConnectionCallbacks } public void setOverlayManager(OverlayManager overlayManager) { - this.overlayManager = overlayManager; + this.overlayManager = new WeakReference(overlayManager); } }; diff --git a/core/src/main/java/com/mapzen/android/location/LocationFactory.java b/core/src/main/java/com/mapzen/android/location/LocationFactory.java index 17993785..24b6eb47 100644 --- a/core/src/main/java/com/mapzen/android/location/LocationFactory.java +++ b/core/src/main/java/com/mapzen/android/location/LocationFactory.java @@ -11,7 +11,6 @@ public class LocationFactory { private static LostApiClient shared; - private static Context context; /** * Returns shared {@link LostApiClient}. @@ -23,9 +22,8 @@ public class LocationFactory { * and future attempts to bind the fused location service would fail. */ @Deprecated public static LostApiClient sharedClient(Context context) { - if (LocationFactory.context != context) { + if (shared == null) { shared = new LostApiClient.Builder(context).build(); - LocationFactory.context = context; } return shared; @@ -42,9 +40,8 @@ public class LocationFactory { */ @Deprecated public static LostApiClient sharedClient(Context context, ConnectionCallbacks callbacks) { - if (LocationFactory.context != context) { + if (shared == null) { shared = new LostApiClient.Builder(context).addConnectionCallbacks(callbacks).build(); - LocationFactory.context = context; } return shared; diff --git a/core/src/test/java/com/mapzen/android/core/MapzenManagerTest.java b/core/src/test/java/com/mapzen/android/core/MapzenManagerTest.java index c0f5b3a4..75bd44d1 100644 --- a/core/src/test/java/com/mapzen/android/core/MapzenManagerTest.java +++ b/core/src/test/java/com/mapzen/android/core/MapzenManagerTest.java @@ -13,10 +13,10 @@ import android.content.res.Resources; import android.support.annotation.NonNull; +import static com.mapzen.TestHelper.getMockContext; import static com.mapzen.android.core.MapzenManager.API_KEY_RES_NAME; import static com.mapzen.android.core.MapzenManager.API_KEY_RES_TYPE; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @RunWith(PowerMockRunner.class) @@ -27,7 +27,7 @@ public class MapzenManagerTest { } @Test public void shouldNotBeNull() throws Exception { - Context context = mock(Context.class); + Context context = getMockContext(); Resources resources = new TestResources(); when(context.getResources()).thenReturn(resources); MapzenManager mapzenManager = MapzenManager.instance(context); @@ -36,7 +36,7 @@ public class MapzenManagerTest { @Test(expected = IllegalStateException.class) public void getApiKey_shouldReturnThrowIfNotSet() throws Exception { - Context context = mock(Context.class); + Context context = getMockContext(); Resources resources = new TestResources(); when(context.getResources()).thenReturn(resources); MapzenManager mapzenManager = MapzenManager.instance(context); @@ -45,7 +45,7 @@ public void getApiKey_shouldReturnThrowIfNotSet() throws Exception { @Test(expected = IllegalStateException.class) public void getApiKey_shouldReturnThrowIfResourceNotFound() throws Exception { - Context context = mock(Context.class); + Context context = getMockContext(); Resources resources = new TestResourcesNotFound(); when(context.getResources()).thenReturn(resources); MapzenManager mapzenManager = MapzenManager.instance(context); @@ -53,7 +53,7 @@ public void getApiKey_shouldReturnThrowIfResourceNotFound() throws Exception { } @Test public void getApiKey_shouldReturnStringResourceValue() throws Exception { - Context context = mock(Context.class); + Context context = getMockContext(); TestResources resources = new TestResources(); resources.testApiKey = "mapzen-fake-api-key"; when(context.getResources()).thenReturn(resources); @@ -62,7 +62,7 @@ public void getApiKey_shouldReturnThrowIfResourceNotFound() throws Exception { } @Test public void setApiKey_shouldOverrideStringResourcesValue() throws Exception { - Context context = mock(Context.class); + Context context = getMockContext(); TestResources resources = new TestResources(); resources.testApiKey = "mapzen-fake-api-key"; when(context.getResources()).thenReturn(resources); diff --git a/core/src/test/java/com/mapzen/android/graphics/MapInitializerTest.java b/core/src/test/java/com/mapzen/android/graphics/MapInitializerTest.java index 18581b13..6fe03dfc 100644 --- a/core/src/test/java/com/mapzen/android/graphics/MapInitializerTest.java +++ b/core/src/test/java/com/mapzen/android/graphics/MapInitializerTest.java @@ -49,7 +49,11 @@ public class MapInitializerTest { @Test public void init_shouldReturnMapzenMap() throws Exception { final TestCallback callback = new TestCallback(); - final TestMapView mapView = new TestMapView(); + final TestMapView mapView = mock(TestMapView.class); + Context context = mock(Context.class); + when(context.getApplicationContext()).thenReturn(mock(Context.class)); + when(mapView.getContext()).thenReturn(context); + when(mapView.getTangramMapView()).thenCallRealMethod(); MapzenManager.instance(getMockContext()).setApiKey("fake-mapzen-api-key"); mapInitializer.init(mapView, callback); assertThat(callback.map).isInstanceOf(MapzenMap.class); diff --git a/core/src/test/java/com/mapzen/android/location/LocationFactoryTest.java b/core/src/test/java/com/mapzen/android/location/LocationFactoryTest.java index edc5d2e8..b6552f0a 100644 --- a/core/src/test/java/com/mapzen/android/location/LocationFactoryTest.java +++ b/core/src/test/java/com/mapzen/android/location/LocationFactoryTest.java @@ -38,16 +38,16 @@ public class LocationFactoryTest { .isEqualTo(LocationFactory.sharedClient(context, callbacks)); } - @Test public void sharedClient_shouldReturnNewClientForNewContext() throws Exception { + @Test public void sharedClient_shouldReturnSameClientForNewContext() throws Exception { assertThat(LocationFactory.sharedClient(Mockito.mock(Context.class))) - .isNotEqualTo(LocationFactory.sharedClient(Mockito.mock(Context.class))); + .isEqualTo(LocationFactory.sharedClient(Mockito.mock(Context.class))); } - @Test public void sharedClientWithCallbacks_shouldReturnNewClientForNewContext() + @Test public void sharedClientWithCallbacks_shouldReturnSameClientForNewContext() throws Exception { LostApiClient.ConnectionCallbacks callbacks = new TestConnectionCallbacks(); assertThat(LocationFactory.sharedClient(Mockito.mock(Context.class), callbacks)) - .isNotEqualTo(LocationFactory.sharedClient(Mockito.mock(Context.class), callbacks)); + .isEqualTo(LocationFactory.sharedClient(Mockito.mock(Context.class), callbacks)); } private class TestConnectionCallbacks implements LostApiClient.ConnectionCallbacks { diff --git a/mapzen-places-api/src/test/java/com/mapzen/android/lost/internal/FusedApiServiceUpdater.java b/mapzen-places-api/src/test/java/com/mapzen/android/lost/internal/FusedApiServiceUpdater.java new file mode 100644 index 00000000..7e81df90 --- /dev/null +++ b/mapzen-places-api/src/test/java/com/mapzen/android/lost/internal/FusedApiServiceUpdater.java @@ -0,0 +1,13 @@ +package com.mapzen.android.lost.internal; + +/** + * Created by sarahlensing on 5/11/17. + */ + +public class FusedApiServiceUpdater { + + public static void updateApiService(FusedLocationProviderApiImpl api, + IFusedLocationProviderService service) { + api.service = service; + } +} diff --git a/mapzen-places-api/src/test/java/com/mapzen/places/api/internal/PlaceAutocompletePresenterTest.java b/mapzen-places-api/src/test/java/com/mapzen/places/api/internal/PlaceAutocompletePresenterTest.java index 9f3fa856..033b6ca7 100644 --- a/mapzen-places-api/src/test/java/com/mapzen/places/api/internal/PlaceAutocompletePresenterTest.java +++ b/mapzen-places-api/src/test/java/com/mapzen/places/api/internal/PlaceAutocompletePresenterTest.java @@ -2,8 +2,9 @@ import com.mapzen.android.lost.api.LocationServices; import com.mapzen.android.lost.api.LostApiClient; +import com.mapzen.android.lost.internal.FusedApiServiceUpdater; import com.mapzen.android.lost.internal.FusedLocationProviderApiImpl; -import com.mapzen.android.lost.internal.FusedLocationProviderService; +import com.mapzen.android.lost.internal.IFusedLocationProviderService; import com.mapzen.pelias.BoundingBox; import com.mapzen.pelias.gson.Feature; import com.mapzen.pelias.gson.Properties; @@ -56,21 +57,20 @@ public void getBoundingBox_shouldReturnCorrectBox() throws Exception { @Test public void getBoundingBox_lostClient_shouldReturnCorrectBox() throws Exception { - FusedLocationProviderService.FusedLocationProviderBinder stubBinder = - mock(FusedLocationProviderService.FusedLocationProviderBinder.class); - FusedLocationProviderService mockService = mock(FusedLocationProviderService.class); - when(stubBinder.getService()).thenReturn(mockService); FusedLocationProviderApiImpl impl = (FusedLocationProviderApiImpl) LocationServices.FusedLocationApi; - impl.onServiceConnected(stubBinder); + IFusedLocationProviderService service = mock(IFusedLocationProviderService.class); + FusedApiServiceUpdater.updateApiService(impl, service); LostApiClient client = mock(LostApiClient.class); + when(client.isConnected()).thenReturn(true); + presenter.setLostClient(client); + Location location = mock(Location.class); when(location.getLatitude()).thenReturn(40.0); when(location.getLongitude()).thenReturn(70.0); - when(mockService.getLastLocation(client)).thenReturn(location); - presenter.setLostClient(client); - when(client.isConnected()).thenReturn(true); + when(service.getLastLocation()).thenReturn(location); + BoundingBox boundingBox = presenter.getBoundingBox(); double boundsRadius = 0.02; assertThat(boundingBox.getMinLat() + boundsRadius).isEqualTo(location.getLatitude()); diff --git a/samples/mapzen-android-sdk-sample/src/main/java/com/mapzen/android/sdk/sample/BasicMapzenActivity.java b/samples/mapzen-android-sdk-sample/src/main/java/com/mapzen/android/sdk/sample/BasicMapzenActivity.java index a054c6ff..3c0d1685 100644 --- a/samples/mapzen-android-sdk-sample/src/main/java/com/mapzen/android/sdk/sample/BasicMapzenActivity.java +++ b/samples/mapzen-android-sdk-sample/src/main/java/com/mapzen/android/sdk/sample/BasicMapzenActivity.java @@ -87,6 +87,11 @@ private void configureMap(boolean enabled) { } } + @Override protected void onDestroy() { + super.onDestroy(); + map.setFindMeOnClickListener(null); + } + @Override public void onItemSelected(AdapterView parent, View view, int position, long id) { if (map == null) { return;