[android] use strong references to the offline merge callbacks

(cherry picked from commit b7fafaa42949cbe27a36ab99b21d3c75819dbd0c)

2 files changed, 114 insertions, 103 deletions

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java
index 8684d7c6f1..535107c529 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java
@@ -2,11 +2,12 @@ package com.mapbox.mapboxsdk.offline;
 
 import android.annotation.SuppressLint;
 import android.content.Context;
-import android.os.AsyncTask;
 import android.os.Handler;
 import android.os.Looper;
+import android.support.annotation.AnyThread;
 import android.support.annotation.Keep;
 import android.support.annotation.NonNull;
+import android.support.annotation.UiThread;
 
 import com.mapbox.mapboxsdk.LibraryLoader;
 import com.mapbox.mapboxsdk.Mapbox;
@@ -21,7 +22,6 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
-import java.lang.ref.WeakReference;
 import java.nio.channels.FileChannel;
 
 /**
@@ -32,6 +32,7 @@ import java.nio.channels.FileChannel;
  *
  * @see <a href="https://docs.mapbox.com/help/troubleshooting/mobile-offline/">Offline Maps Information/</a>
  */
+@UiThread
 public class OfflineManager {
 
   private static final String TAG = "Mbgl - OfflineManager";
@@ -156,6 +157,7 @@ public class OfflineManager {
     return instance;
   }
 
+  @AnyThread
   private Handler getHandler() {
     if (handler == null) {
       handler = new Handler(Looper.getMainLooper());
@@ -205,6 +207,8 @@ public class OfflineManager {
    * Merge offline regions from a secondary database into the main offline database.
    * <p>
    * When the merge is completed, or fails, the {@link MergeOfflineRegionsCallback} will be invoked on the main thread.
+   * The callback reference is <b>strongly kept</b> throughout the process,
+   * so it needs to be wrapped in a weak reference or released on the client side if necessary.
    * <p>
    * The secondary database may need to be upgraded to the latest schema.
    * This is done in-place and requires write-access to the provided path.
@@ -225,73 +229,50 @@ public class OfflineManager {
    */
   public void mergeOfflineRegions(@NonNull String path, @NonNull final MergeOfflineRegionsCallback callback) {
     final File src = new File(path);
-    new FileUtils.CheckFileReadPermissionTask(new FileUtils.OnCheckFileReadPermissionListener() {
+    new Thread(new Runnable() {
       @Override
-      public void onReadPermissionGranted() {
-        new FileUtils.CheckFileWritePermissionTask(new FileUtils.OnCheckFileWritePermissionListener() {
-          @Override
-          public void onWritePermissionGranted() {
-            // path writable, merge and update schema in place if necessary
-            mergeOfflineDatabaseFiles(src, callback, false);
-          }
-
-          @Override
-          public void onError() {
-            // path not writable, copy the the file to temp directory, then merge and update schema on a copy if
-            // necessary
-            File dst = new File(FileSource.getInternalCachePath(context), src.getName());
-            new CopyTempDatabaseFileTask(OfflineManager.this, callback).execute(src, dst);
+      public void run() {
+        String errorMessage = null;
+        if (src.canWrite()) {
+          getHandler().post(new Runnable() {
+            @Override
+            public void run() {
+              // path writable, merge and update schema in place if necessary
+              mergeOfflineDatabaseFiles(src, callback, false);
+            }
+          });
+        } else if (src.canRead()) {
+          // path not writable, copy the the file to temp directory
+          final File dst = new File(FileSource.getInternalCachePath(context), src.getName());
+          try {
+            copyTempDatabaseFile(src, dst);
+            getHandler().post(new Runnable() {
+              @Override
+              public void run() {
+                // merge and update schema using the copy
+                OfflineManager.this.mergeOfflineDatabaseFiles(dst, callback, true);
+              }
+            });
+          } catch (IOException ex) {
+            ex.printStackTrace();
+            errorMessage = ex.getMessage();
           }
-        }).execute(src);
-      }
-
-      @Override
-      public void onError() {
-        // path not readable, abort
-        callback.onError("Secondary database needs to be located in a readable path.");
-      }
-    }).execute(src);
-  }
-
-  private static final class CopyTempDatabaseFileTask extends AsyncTask<Object, Void, Object> {
-    @NonNull
-    private final WeakReference<OfflineManager> offlineManagerWeakReference;
-    @NonNull
-    private final WeakReference<MergeOfflineRegionsCallback> callbackWeakReference;
-
-    CopyTempDatabaseFileTask(OfflineManager offlineManager, MergeOfflineRegionsCallback callback) {
-      this.offlineManagerWeakReference = new WeakReference<>(offlineManager);
-      this.callbackWeakReference = new WeakReference<>(callback);
-    }
-
-    @Override
-    protected Object doInBackground(Object... objects) {
-      File src = (File) objects[0];
-      File dst = (File) objects[1];
-
-      try {
-        copyTempDatabaseFile(src, dst);
-        return dst;
-      } catch (IOException ex) {
-        return ex.getMessage();
-      }
-    }
+        } else {
+          // path not readable, abort
+          errorMessage = "Secondary database needs to be located in a readable path.";
+        }
 
-    @Override
-    protected void onPostExecute(Object object) {
-      MergeOfflineRegionsCallback callback = callbackWeakReference.get();
-      if (callback != null) {
-        OfflineManager offlineManager = offlineManagerWeakReference.get();
-        if (object instanceof File && offlineManager != null) {
-          // successfully copied the file, perform merge
-          File dst = (File) object;
-          offlineManager.mergeOfflineDatabaseFiles(dst, callback, true);
-        } else if (object instanceof String) {
-          // error occurred
-          callback.onError((String) object);
+        if (errorMessage != null) {
+          final String finalErrorMessage = errorMessage;
+          getHandler().post(new Runnable() {
+            @Override
+            public void run() {
+              callback.onError(finalErrorMessage);
+            }
+          });
         }
       }
-    }
+    }).start();
   }
 
   private static void copyTempDatabaseFile(@NonNull File sourceFile, File destFile) throws IOException {
@@ -324,13 +305,13 @@ public class OfflineManager {
     mergeOfflineRegions(fileSource, file.getAbsolutePath(), new MergeOfflineRegionsCallback() {
       @Override
       public void onMerge(final OfflineRegion[] offlineRegions) {
+        if (isTemporaryFile) {
+          file.delete();
+        }
         getHandler().post(new Runnable() {
           @Override
           public void run() {
             fileSource.deactivate();
-            if (isTemporaryFile) {
-              file.delete();
-            }
             callback.onMerge(offlineRegions);
           }
         });
@@ -338,13 +319,13 @@ public class OfflineManager {
 
       @Override
       public void onError(final String error) {
+        if (isTemporaryFile) {
+          file.delete();
+        }
         getHandler().post(new Runnable() {
           @Override
           public void run() {
             fileSource.deactivate();
-            if (isTemporaryFile) {
-              file.delete();
-            }
             callback.onError(error);
           }
         });
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt
index 45ff67d688..12169dbe8f 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt
@@ -4,8 +4,8 @@ import android.os.Bundle
 import android.support.v7.app.AppCompatActivity
 import android.widget.Toast
 import com.mapbox.mapboxsdk.Mapbox
-import com.mapbox.mapboxsdk.maps.Style
 import com.mapbox.mapboxsdk.log.Logger
+import com.mapbox.mapboxsdk.maps.Style
 import com.mapbox.mapboxsdk.offline.OfflineManager
 import com.mapbox.mapboxsdk.offline.OfflineRegion
 import com.mapbox.mapboxsdk.storage.FileSource
@@ -20,6 +20,64 @@ class MergeOfflineRegionsActivity : AppCompatActivity() {
     private const val TEST_STYLE = Style.SATELLITE
   }
 
+  private val onFileCopiedListener = object : FileUtils.OnFileCopiedFromAssetsListener {
+    override fun onFileCopiedFromAssets() {
+      Toast.makeText(
+        this@MergeOfflineRegionsActivity,
+        String.format("OnFileCopied."),
+        Toast.LENGTH_LONG).show()
+      mergeDb()
+    }
+
+    override fun onError() {
+      Toast.makeText(
+        this@MergeOfflineRegionsActivity,
+        String.format("Error copying DB file."),
+        Toast.LENGTH_LONG).show()
+    }
+  }
+
+  private val onRegionMergedListener = object : OfflineManager.MergeOfflineRegionsCallback {
+    override fun onMerge(offlineRegions: Array<OfflineRegion>) {
+      mapView.getMapAsync {
+        it.setStyle(Style.Builder().fromUrl(TEST_STYLE))
+      }
+      Toast.makeText(
+        this@MergeOfflineRegionsActivity,
+        String.format("Merged %d regions.", offlineRegions.size),
+        Toast.LENGTH_LONG).show()
+    }
+
+    override fun onError(error: String) {
+      Toast.makeText(
+        this@MergeOfflineRegionsActivity,
+        String.format("Offline DB merge error."),
+        Toast.LENGTH_LONG).show()
+      Logger.e(LOG_TAG, error)
+    }
+  }
+
+  /**
+   * Since we expect from the results of the offline merge callback to interact with the hosting activity,
+   * we need to ensure that we are not interacting with a destroyed activity.
+   */
+  private class MergeCallback(private var activityCallback: OfflineManager.MergeOfflineRegionsCallback?) : OfflineManager.MergeOfflineRegionsCallback {
+
+    override fun onMerge(offlineRegions: Array<out OfflineRegion>?) {
+      activityCallback?.onMerge(offlineRegions)
+    }
+
+    override fun onError(error: String?) {
+      activityCallback?.onError(error)
+    }
+
+    fun onActivityDestroy() {
+      activityCallback = null
+    }
+  }
+
+  private val mergeCallback = MergeCallback(onRegionMergedListener)
+
   override fun onCreate(savedInstanceState: Bundle?) {
     super.onCreate(savedInstanceState)
     setContentView(R.layout.activity_merge_offline_regions)
@@ -39,42 +97,13 @@ class MergeOfflineRegionsActivity : AppCompatActivity() {
 
   private fun copyAsset() {
     // copy db asset to internal memory
-    FileUtils.CopyFileFromAssetsTask(this, object : FileUtils.OnFileCopiedFromAssetsListener {
-      override fun onFileCopiedFromAssets() {
-        mergeDb()
-      }
-
-      override fun onError() {
-        Toast.makeText(
-          this@MergeOfflineRegionsActivity,
-          String.format("Error copying DB file."),
-          Toast.LENGTH_LONG).show()
-      }
-    }).execute(TEST_DB_FILE_NAME, FileSource.getResourcesCachePath(this))
+    FileUtils.CopyFileFromAssetsTask(this, onFileCopiedListener).execute(TEST_DB_FILE_NAME, FileSource.getResourcesCachePath(this))
   }
 
   private fun mergeDb() {
     OfflineManager.getInstance(this).mergeOfflineRegions(
-      FileSource.getResourcesCachePath(this) + "/" + TEST_DB_FILE_NAME,
-      object : OfflineManager.MergeOfflineRegionsCallback {
-        override fun onMerge(offlineRegions: Array<OfflineRegion>) {
-          mapView.getMapAsync{
-              it.setStyle(Style.Builder().fromUrl(TEST_STYLE))
-          }
-          Toast.makeText(
-            this@MergeOfflineRegionsActivity,
-            String.format("Merged %d regions.", offlineRegions.size),
-            Toast.LENGTH_LONG).show()
-        }
-
-        override fun onError(error: String) {
-          Toast.makeText(
-            this@MergeOfflineRegionsActivity,
-            String.format("Offline DB merge error."),
-            Toast.LENGTH_LONG).show()
-          Logger.e(LOG_TAG, error)
-        }
-      })
+      FileSource.getResourcesCachePath(this) + "/" + TEST_DB_FILE_NAME, mergeCallback
+    )
   }
 
   override fun onStart() {
@@ -104,6 +133,7 @@ class MergeOfflineRegionsActivity : AppCompatActivity() {
 
   override fun onDestroy() {
     super.onDestroy()
+    mergeCallback.onActivityDestroy()
     mapView.onDestroy()
 
     // restoring connectivity state