From fe811bd38e5374b1ab2cb89788abbf51f6a8e5e6 Mon Sep 17 00:00:00 2001
From: "Thiago Marcos P. Santos" <thiago@mapbox.com>
Date: Fri, 4 Mar 2016 17:31:51 -0300
Subject: [PATCH] [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().
---
 .../mapbox/mapboxsdk/http/HTTPRequest.java    | 30 +++++++++++++++----
 platform/android/src/http_request_android.cpp | 16 +++-------
 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 66bdc4ae774..7be1c1a0f40 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.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 @@ private HTTPRequest(long nativePtr, String resourceUrl, String userAgent, String
 
     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 @@ public void onResponse(Call call, Response response) throws IOException {
             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 @@ public void onFailure(Call call, IOException e) {
             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 3f955671059..1e0039cdf86 100644
--- a/platform/android/src/http_request_android.cpp
+++ b/platform/android/src/http_request_android.cpp
@@ -40,8 +40,6 @@ class HTTPRequest : public HTTPRequestBase {
 private:
     void finish();
 
-    bool cancelled = false;
-
     std::unique_ptr<Response> response;
     const std::shared_ptr<const Response> existingResponse;
 
@@ -50,7 +48,6 @@ class HTTPRequest : public HTTPRequestBase {
     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);
     }