[android] Do not reach native if the HTTPContext was invalidated

Native will get destroyed before the Java HTTPRequest and asynchronous
requests might still get answered.

If the request gets canceled, we now clear the reference to the native
code and it will never reach it.

Lock is needed because OkHTTP replies on its own thread, so we need to
guard the access to the variable we use for checking if the Request was
canceled. In the future we could remove the lock and use Runnable when
we move Android main loop to Looper instead of libuv.

Like the other implementations of HTTPRequest on Mapbox GL Native, the
request is destroyed synchronously on ::cancel().

2 files changed, 29 insertions, 17 deletions

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java
index 66bdc4ae77..7be1c1a0f4 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java
@@ -10,6 +10,7 @@ import java.io.InterruptedIOException;
 import java.net.ProtocolException;
 import java.net.SocketException;
 import java.net.UnknownHostException;
+import java.util.concurrent.locks.ReentrantLock;
 
 import javax.net.ssl.SSLException;
 
@@ -27,7 +28,10 @@ class HTTPRequest implements Callback {
     private static final int CONNECTION_ERROR = 0;
     private static final int TEMPORARY_ERROR = 1;
     private static final int PERMANENT_ERROR = 2;
-    private static final int CANCELED_ERROR = 3;
+
+    // Reentrancy is not needed, but "Lock" is an
+    // abstract class.
+    private ReentrantLock mLock = new ReentrantLock();
 
     private long mNativePtr = 0;
 
@@ -52,6 +56,15 @@ class HTTPRequest implements Callback {
 
     public void cancel() {
         mCall.cancel();
+
+        // TODO: We need a lock here because we can try
+        // to cancel at the same time the request is getting
+        // answered on the OkHTTP thread. We could get rid of
+        // this lock by using Runnable when we move Android
+        // implementation of mbgl::RunLoop to Looper.
+        mLock.lock();
+        mNativePtr = 0;
+        mLock.unlock();
     }
 
     @Override
@@ -77,7 +90,11 @@ class HTTPRequest implements Callback {
             response.body().close();
         }
 
-        nativeOnResponse(response.code(), response.header("ETag"), response.header("Last-Modified"), response.header("Cache-Control"), response.header("Expires"), body);
+        mLock.lock();
+        if (mNativePtr != 0) {
+            nativeOnResponse(response.code(), response.header("ETag"), response.header("Last-Modified"), response.header("Cache-Control"), response.header("Expires"), body);
+        }
+        mLock.unlock();
     }
 
     @Override
@@ -89,11 +106,14 @@ class HTTPRequest implements Callback {
             type = CONNECTION_ERROR;
         } else if ((e instanceof InterruptedIOException)) {
             type = TEMPORARY_ERROR;
-        } else if (mCall.isCanceled()) {
-            type = CANCELED_ERROR;
         }
 
         String errorMessage = e.getMessage() != null ? e.getMessage() : "Error processing the request";
-        nativeOnFailure(type, errorMessage);
+
+        mLock.lock();
+        if (mNativePtr != 0) {
+            nativeOnFailure(type, errorMessage);
+        }
+        mLock.unlock();
     }
 }
diff --git a/platform/android/src/http_request_android.cpp b/platform/android/src/http_request_android.cpp
index 3f95567105..1e0039cdf8 100644
--- a/platform/android/src/http_request_android.cpp
+++ b/platform/android/src/http_request_android.cpp
@@ -40,8 +40,6 @@ public:
 private:
     void finish();
 
-    bool cancelled = false;
-
     std::unique_ptr<Response> response;
     const std::shared_ptr<const Response> existingResponse;
 
@@ -50,7 +48,6 @@ private:
     static const int connectionError = 0;
     static const int temporaryError = 1;
     static const int permanentError = 2;
-    static const int canceledError = 3;
 };
 
 jni::Class<HTTPRequest> HTTPRequest::javaClass;
@@ -97,20 +94,18 @@ HTTPRequest::HTTPRequest(jni::JNIEnv& env, const Resource& resource_, Callback c
 }
 
 void HTTPRequest::cancel() {
-    cancelled = true;
-
     UniqueEnv env = android::AttachEnv();
 
     static auto cancel = javaClass.GetMethod<void ()>(*env, "cancel");
 
     javaRequest->Call(*env, cancel);
+
+    delete this;
 }
 
 void HTTPRequest::finish() {
-    if (!cancelled) {
-        assert(response);
-        notify(*response);
-    }
+    assert(response);
+    notify(*response);
 
     delete this;
 }
@@ -173,9 +168,6 @@ void HTTPRequest::onFailure(jni::JNIEnv& env, int type, jni::String message) {
         case temporaryError:
             response->error = std::make_unique<Error>(Error::Reason::Server, messageStr);
             break;
-        case canceledError:
-            response.reset();
-            break;
         default:
             response->error = std::make_unique<Error>(Error::Reason::Other, messageStr);
     }