[android] Reuse Java Source objects by holding on to a strong reference in the C++ peer.

7 files changed, 95 insertions, 31 deletions

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
index e8eb7e8718..6c1827a679 100755
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
@@ -733,7 +733,7 @@ final class NativeMapView {
     if (isDestroyedOn("addSource")) {
       return;
     }
-    nativeAddSource(source.getNativePtr());
+    nativeAddSource(source, source.getNativePtr());
   }
 
   @Nullable
@@ -748,7 +748,7 @@ final class NativeMapView {
     if (isDestroyedOn("removeSource")) {
       return null;
     }
-    nativeRemoveSource(source.getNativePtr());
+    nativeRemoveSource(source, source.getNativePtr());
     return source;
   }
 
@@ -1027,11 +1027,11 @@ final class NativeMapView {
 
   private native Source nativeGetSource(String sourceId);
 
-  private native void nativeAddSource(long nativeSourcePtr) throws CannotAddSourceException;
+  private native void nativeAddSource(Source source, long sourcePtr) throws CannotAddSourceException;
 
   private native Source nativeRemoveSourceById(String sourceId);
 
-  private native void nativeRemoveSource(long sourcePtr);
+  private native void nativeRemoveSource(Source source, long sourcePtr);
 
   private native void nativeAddImage(String name, int width, int height, float pixelRatio,
                                      byte[] array);
diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp
index 04cbb21927..381b41fab5 100755
--- a/platform/android/src/native_map_view.cpp
+++ b/platform/android/src/native_map_view.cpp
@@ -858,7 +858,6 @@ jni::Array<jni::Object<Source>> NativeMapView::getSources(JNIEnv& env) {
     for (auto source : sources) {
         auto jSource = jni::Object<Source>(createJavaSourcePeer(env, *rendererFrontend, *source));
         jSources.Set(env, index, jSource);
-        jni::DeleteLocalRef(env, jSource);
         index++;
     }
 
@@ -877,12 +876,12 @@ jni::Object<Source> NativeMapView::getSource(JNIEnv& env, jni::String sourceId)
     return jni::Object<Source>(createJavaSourcePeer(env, *rendererFrontend, *coreSource));
 }
 
-void NativeMapView::addSource(JNIEnv& env, jni::jlong sourcePtr) {
+void NativeMapView::addSource(JNIEnv& env, jni::Object<Source> obj, jlong sourcePtr) {
     assert(sourcePtr != 0);
 
     Source *source = reinterpret_cast<Source *>(sourcePtr);
     try {
-        source->addToMap(*map);
+        source->addToMap(env, obj, *map);
         source->setRendererFrontend(*rendererFrontend);
     } catch (const std::runtime_error& error) {
         jni::ThrowNew(env, jni::FindClass(env, "com/mapbox/mapboxsdk/style/sources/CannotAddSourceException"), error.what());
@@ -892,20 +891,17 @@ void NativeMapView::addSource(JNIEnv& env, jni::jlong sourcePtr) {
 jni::Object<Source> NativeMapView::removeSourceById(JNIEnv& env, jni::String id) {
     std::unique_ptr<mbgl::style::Source> coreSource = map->getStyle().removeSource(jni::Make<std::string>(env, id));
     if (coreSource) {
-        return jni::Object<Source>(createJavaSourcePeer(env, *rendererFrontend, *coreSource));
+        return jni::Object<Source>(removeSourceFromMap(std::move(coreSource)));
     } else {
         return jni::Object<Source>();
     }
 }
 
-void NativeMapView::removeSource(JNIEnv&, jlong sourcePtr) {
+void NativeMapView::removeSource(JNIEnv& env, jni::Object<Source> obj, jlong sourcePtr) {
     assert(sourcePtr != 0);
 
     mbgl::android::Source *source = reinterpret_cast<mbgl::android::Source *>(sourcePtr);
-    std::unique_ptr<mbgl::style::Source> coreSource = map->getStyle().removeSource(source->get().getID());
-    if (coreSource) {
-        source->setSource(std::move(coreSource));
-    }
+    source->removeFromMap(env, obj, *map);
 }
 
 void NativeMapView::addImage(JNIEnv& env, jni::String name, jni::jint w, jni::jint h, jni::jfloat scale, jni::Array<jbyte> pixels) {
diff --git a/platform/android/src/native_map_view.hpp b/platform/android/src/native_map_view.hpp
index d7e3b17b99..99d047ff29 100755
--- a/platform/android/src/native_map_view.hpp
+++ b/platform/android/src/native_map_view.hpp
@@ -230,11 +230,11 @@ public:
 
     jni::Object<Source> getSource(JNIEnv&, jni::String);
 
-    void addSource(JNIEnv&, jni::jlong);
+    void addSource(JNIEnv&, jni::Object<Source>, jlong nativePtr);
 
     jni::Object<Source> removeSourceById(JNIEnv&, jni::String);
 
-    void removeSource(JNIEnv&, jlong);
+    void removeSource(JNIEnv&, jni::Object<Source>, jlong nativePtr);
 
     void addImage(JNIEnv&, jni::String, jni::jint, jni::jint, jni::jfloat, jni::Array<jbyte>);
 
diff --git a/platform/android/src/style/sources/source.cpp b/platform/android/src/style/sources/source.cpp
index 447b13019d..8a872f753d 100644
--- a/platform/android/src/style/sources/source.cpp
+++ b/platform/android/src/style/sources/source.cpp
@@ -30,16 +30,27 @@ namespace android {
             : source(coreSource) {
     }
 
-    Source::~Source() = default;
+    Source::~Source() {
+        // Before being added to a map, the Java peer owns this C++ peer and cleans
+        //  up after itself correctly through the jni native peer bindings.
+        // After being added to the map, the ownership is flipped and the C++ peer has a strong reference
+        //  to it's Java peer, preventing the Java peer from being GC'ed.
+        //  In this case, the core source initiates the destruction, which requires releasing the Java peer,
+        //  while also resetting it's nativePtr to 0 to prevent the subsequent GC of the Java peer from
+        // re-entering this dtor.
+        if (ownedSource.get() == nullptr && javaPeer.get() != nullptr) {
+            //Manually clear the java peer
+            android::UniqueEnv env = android::AttachEnv();
+            static auto nativePtrField = javaClass.GetField<jlong>(*env, "nativePtr");
+            javaPeer->Set(*env, nativePtrField, (jlong) 0);
+            javaPeer.reset();
+        }
+    }
 
     style::Source& Source::get() {
         return source;
     }
 
-    void Source::setSource(std::unique_ptr<style::Source> coreSource) {
-        this->ownedSource = std::move(coreSource);
-    }
-
     jni::String Source::getId(jni::JNIEnv& env) {
         return jni::Make<jni::String>(env, source.getID());
     }
@@ -49,14 +60,47 @@ namespace android {
         return attribution ? jni::Make<jni::String>(env, attribution.value()) : jni::Make<jni::String>(env,"");
     }
 
-    void Source::addToMap(mbgl::Map& _map) {
+    void Source::addToMap(JNIEnv& env, jni::Object<Source> obj, mbgl::Map& map) {
         // Check to see if we own the source first
         if (!ownedSource) {
             throw std::runtime_error("Cannot add source twice");
         }
 
-        // Add source to map
-        _map.getStyle().addSource(releaseCoreSource());
+        // Add source to map and release ownership
+        map.getStyle().addSource(releaseCoreSource());
+        attachJavaSource(env, obj);
+    }
+
+    void Source::removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map& map) {
+        //Cannot remove if not attached yet
+        if (ownedSource) {
+            throw std::runtime_error("Cannot remove source detached source");
+        }
+
+        // Remove the source from the map
+        std::unique_ptr<mbgl::style::Source> coreSource = map.getStyle().removeSource(source.getID());
+        attachCoreSource(std::move(coreSource));
+    }
+
+    jni::jobject* Source::attachCoreSource(std::unique_ptr<style::Source> coreSource) {
+        // Take over the source
+        ownedSource = std::move(coreSource);
+
+        // Release the peer relationships. These will be re-established when the source is added to a map
+        if (ownedSource->peer.has_value()) {
+            util::any_cast<std::unique_ptr<Source>>(&(ownedSource->peer))->release();
+            ownedSource->peer = nullptr;
+        }
+
+        return (jni::jobject *) javaPeer.release()->Get();
+    }
+
+    void Source::attachJavaSource(JNIEnv& env, jni::Object<Source> obj) {
+        // Add peer to core source
+        source.peer = std::unique_ptr<Source>(this);
+
+        // Add strong reference to java source
+        javaPeer = obj.NewGlobalRef(env);
     }
 
     void Source::setRendererFrontend(AndroidRendererFrontend& frontend_) {
@@ -68,6 +112,16 @@ namespace android {
         return std::move(ownedSource);
     }
 
+    jni::jobject* Source::getJavaPeer(jni::JNIEnv& env) {
+        if(!javaPeer) {
+            // Create Java peer for sources already in the map
+            auto jp = jni::Object<Source>(createJavaPeer(env));
+            attachJavaSource(env, jp);
+            jni::DeleteLocalRef(env, jp);
+        }
+        return (jni::jobject *) javaPeer->Get();
+    }
+
     jni::Class<Source> Source::javaClass;
 
     void Source::registerNative(jni::JNIEnv& env) {
diff --git a/platform/android/src/style/sources/source.hpp b/platform/android/src/style/sources/source.hpp
index 383017b66f..d6dde17983 100644
--- a/platform/android/src/style/sources/source.hpp
+++ b/platform/android/src/style/sources/source.hpp
@@ -33,17 +33,19 @@ public:
 
     virtual ~Source();
 
-    /**
-     * Set core source (ie return ownership after remove)
-     */
-    void setSource(std::unique_ptr<style::Source>);
-
     style::Source& get();
 
-    void addToMap(mbgl::Map&);
+    void addToMap(JNIEnv&, jni::Object<Source>, mbgl::Map&);
+
+    void removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map&);
+
+    void attachJavaSource(JNIEnv& env, jni::Object<Source>);
+    jni::jobject* attachCoreSource(std::unique_ptr<style::Source>);
 
     void setRendererFrontend(AndroidRendererFrontend&);
 
+    jni::jobject* getJavaPeer(jni::JNIEnv&);
+
     virtual jni::jobject* createJavaPeer(jni::JNIEnv&) = 0;
 
     jni::String getId(jni::JNIEnv&);
@@ -60,6 +62,9 @@ protected:
     // Raw pointer that is valid until the source is removed from the map
     mbgl::style::Source& source;
 
+    // Set when the source is added to a map
+    jni::UniqueObject<Source> javaPeer;
+
     // RendererFrontend pointer is valid only when
     // added to the map
     AndroidRendererFrontend* rendererFrontend;
diff --git a/platform/android/src/style/sources/sources.cpp b/platform/android/src/style/sources/sources.cpp
index 72d7bcda8c..173f5c705c 100644
--- a/platform/android/src/style/sources/sources.cpp
+++ b/platform/android/src/style/sources/sources.cpp
@@ -20,7 +20,9 @@ namespace {
 
     Source* initializeSourcePeer(mbgl::style::Source& coreSource) {
         Source* source;
-        if (coreSource.is<mbgl::style::VectorSource>()) {
+        if (coreSource.peer.has_value()) {
+            source = mbgl::util::any_cast<std::unique_ptr<Source>>(&coreSource.peer)->get();
+        } else if (coreSource.is<mbgl::style::VectorSource>()) {
             source = new VectorSource(*coreSource.as<mbgl::style::VectorSource>());
         } else if (coreSource.is<mbgl::style::RasterSource>()) {
             source = new RasterSource(*coreSource.as<mbgl::style::RasterSource>());
@@ -43,11 +45,16 @@ namespace android {
 jni::jobject* createJavaSourcePeer(jni::JNIEnv& env, AndroidRendererFrontend& frontend, mbgl::style::Source& coreSource) {
     std::unique_ptr<Source> peerSource = std::unique_ptr<Source>(initializeSourcePeer(coreSource));
     peerSource->setRendererFrontend(frontend);
-    jni::jobject* result = peerSource->createJavaPeer(env);
+    jni::jobject* result = peerSource->getJavaPeer(env);
     peerSource.release();
     return result;
 }
 
+jni::jobject* removeSourceFromMap(std::unique_ptr<mbgl::style::Source>&& coreSource) {
+    std::unique_ptr<Source> peerSource = std::unique_ptr<Source>(initializeSourcePeer(*coreSource));
+    return peerSource.release()->attachCoreSource(std::move(coreSource));
+}
+
 void registerNativeSources(jni::JNIEnv& env) {
     Source::registerNative(env);
     GeoJSONSource::registerNative(env);
diff --git a/platform/android/src/style/sources/sources.hpp b/platform/android/src/style/sources/sources.hpp
index c7b36818b2..b2ab3d78ee 100644
--- a/platform/android/src/style/sources/sources.hpp
+++ b/platform/android/src/style/sources/sources.hpp
@@ -12,6 +12,8 @@ namespace android {
 
     jni::jobject* createJavaSourcePeer(jni::JNIEnv&, AndroidRendererFrontend&, mbgl::style::Source&);
 
+    jni::jobject* removeSourceFromMap(std::unique_ptr<mbgl::style::Source>&&);
+
     void registerNativeSources(jni::JNIEnv&);
 
 } // namespace android