Keep strong references to the FileSource/OfflineManager operations callbacks

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

@@ -2,11 +2,12 @@
 
 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.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
-import java.lang.ref.WeakReference;
 import java.nio.channels.FileChannel;
 
 /**
@@ -32,6 +32,7 @@
  *
  * @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 static synchronized OfflineManager getInstance(@NonNull Context context)
     return instance;
   }
 
+  @AnyThread
   private Handler getHandler() {
     if (handler == null) {
       handler = new Handler(Looper.getMainLooper());
@@ -205,6 +207,8 @@ public void run() {
    * 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 void run() {
    */
   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,27 +305,27 @@ private void mergeOfflineDatabaseFiles(@NonNull final File file, @NonNull final
     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);
           }
         });
       }
 
       @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);
           }
         });

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java

@@ -20,7 +20,6 @@
 import com.mapbox.mapboxsdk.utils.ThreadUtils;
 
 import java.io.File;
-import java.lang.ref.WeakReference;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
@@ -69,14 +68,14 @@ public interface ResourcesCachePathChangeCallback {
      *
      * @param path the path of the current resources cache database
      */
-    void onSuccess(String path);
+    void onSuccess(@NonNull String path);
 
     /**
      * Receives an error message if setting the path was not successful
      *
      * @param message the error message
      */
-    void onError(String message);
+    void onError(@NonNull String message);
 
   }
 
@@ -265,63 +264,63 @@ public static String getInternalCachePath(@NonNull Context context) {
   /**
    * Changes the path of the resources cache database.
    * Note that the external storage setting needs to be activated in the manifest.
+   * <p>
+   * 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.
    *
    * @param context  the context of the path
    * @param path     the new database path
    * @param callback the callback to obtain the result
+   * @deprecated Use {@link #setResourcesCachePath(String, ResourcesCachePathChangeCallback)}
    */
-  public static void setResourcesCachePath(@NonNull Context context,
+  @Deprecated
+  public static void setResourcesCachePath(@NonNull final Context context,
                                            @NonNull final String path,
-                                           @NonNull ResourcesCachePathChangeCallback callback) {
-    final String fileSourceActivatedMessage = "Cannot set path, file source is activated."
-      + " Make sure that the map or a resources download is not running.";
-    if (getInstance(context).isActivated()) {
+                                           @NonNull final ResourcesCachePathChangeCallback callback) {
+    setResourcesCachePath(path, callback);
+  }
+
+  /**
+   * Changes the path of the resources cache database.
+   * Note that the external storage setting needs to be activated in the manifest.
+   * <p>
+   * 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.
+   *
+   * @param path     the new database path
+   * @param callback the callback to obtain the result
+   */
+  public static void setResourcesCachePath(@NonNull final String path,
+                                           @NonNull final ResourcesCachePathChangeCallback callback) {
+    final Context applicationContext = Mapbox.getApplicationContext();
+    final FileSource fileSource = FileSource.getInstance(applicationContext);
+
+    if (fileSource.isActivated()) {
+      String fileSourceActivatedMessage = "Cannot set path, file source is activated."
+        + " Make sure that the map or a resources download is not running.";
       Logger.w(TAG, fileSourceActivatedMessage);
       callback.onError(fileSourceActivatedMessage);
-    } else if (path.equals(resourcesCachePath)) {
+    } else if (path.equals(getResourcesCachePath(applicationContext))) {
       // no need to change the path
       callback.onSuccess(path);
     } else {
-      final WeakReference<Context> contextWeakReference = new WeakReference<>(context);
-      final WeakReference<ResourcesCachePathChangeCallback> callbackWeakReference = new WeakReference<>(callback);
       new FileUtils.CheckFileWritePermissionTask(new FileUtils.OnCheckFileWritePermissionListener() {
         @Override
         public void onWritePermissionGranted() {
-          final Context context = contextWeakReference.get();
-          final ResourcesCachePathChangeCallback callback = callbackWeakReference.get();
-
-          if (callback == null) {
-            Logger.w(TAG, "Lost callback reference.");
-            return;
-          } else if (context == null) {
-            String lostContextMessage = "Lost context reference.";
-            Logger.w(TAG, lostContextMessage);
-            callback.onError(lostContextMessage);
-            return;
-          }
-
-          // verify fileSource's activation again after the async task returns
-          if (getInstance(context).isActivated()) {
-            Logger.w(TAG, fileSourceActivatedMessage);
-            callback.onError(fileSourceActivatedMessage);
-          } else {
-            final SharedPreferences.Editor editor =
-              context.getSharedPreferences(MapboxConstants.MAPBOX_SHARED_PREFERENCES, Context.MODE_PRIVATE).edit();
-            editor.putString(MAPBOX_SHARED_PREFERENCE_RESOURCES_CACHE_PATH, path);
-            editor.apply();
-            setResourcesCachePath(context, path);
-            callback.onSuccess(path);
-          }
+          final SharedPreferences.Editor editor =
+            applicationContext.getSharedPreferences(MapboxConstants.MAPBOX_SHARED_PREFERENCES,
+              Context.MODE_PRIVATE).edit();
+          editor.putString(MAPBOX_SHARED_PREFERENCE_RESOURCES_CACHE_PATH, path);
+          editor.apply();
+          setResourcesCachePath(applicationContext, path);
+          callback.onSuccess(path);
         }
 
         @Override
         public void onError() {
-          final ResourcesCachePathChangeCallback callback = callbackWeakReference.get();
-          if (callback != null) {
-            String message = "Path is not writable: " + path;
-            Logger.e(TAG, message);
-            callback.onError(message);
-          }
+          String message = "Path is not writable: " + path;
+          Logger.e(TAG, message);
+          callback.onError(message);
         }
       }).execute(new File(path));
     }