[Cronet] Do not call into native adapter after it is destroyed in ChromiumUrlRequest.java

components/cronet/android/chromium_url_request.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/cronet/android/chromium_url_request.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/macros.h"
 #include "components/cronet/android/url_request_adapter.h"
@@ skipping 22 matching lines @@
     case REQUEST_PRIORITY_HIGHEST:
       return net::HIGHEST;
     default:
       return net::LOWEST;
   }
 }
 
 void SetPostContentType(JNIEnv* env,
                         URLRequestAdapter* request_adapter,
                         jstring content_type) {
-  DCHECK(request_adapter);

[xunjieli, 2015/02/20 22:39:18]

Moving the DCHECK to caller, to make it clearer that we are checking the
adapter.

-
   std::string method_post("POST");
   request_adapter->SetMethod(method_post);
 
   std::string content_type_header("Content-Type");
   std::string content_type_string(ConvertJavaStringToUTF8(env, content_type));
 
   request_adapter->AddHeader(content_type_header, content_type_string);
 }
 
 // A delegate of URLRequestAdapter that delivers callbacks to the Java layer.
@@ skipping 102 matching lines @@
   request_adapter->SetMethod(method);
 }
 
 static void SetUploadData(JNIEnv* env,
                           jobject jcaller,
                           jlong jurl_request_adapter,
                           jstring jcontent_type,
                           jbyteArray jcontent) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   SetPostContentType(env, request_adapter, jcontent_type);
 
   if (jcontent != NULL) {
     jsize size = env->GetArrayLength(jcontent);
     if (size > 0) {
       jbyte* content_bytes = env->GetByteArrayElements(jcontent, NULL);
       request_adapter->SetUploadContent(
           reinterpret_cast<const char*>(content_bytes), size);
       env->ReleaseByteArrayElements(jcontent, content_bytes, 0);
     }
   }
 }
 
 static void SetUploadChannel(JNIEnv* env,
                              jobject jcaller,
                              jlong jurl_request_adapter,
                              jstring jcontent_type,
                              jlong jcontent_length) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   SetPostContentType(env, request_adapter, jcontent_type);
 
   request_adapter->SetUploadChannel(env, jcontent_length);
 }
 
 static void EnableChunkedUpload(JNIEnv* env,
                                 jobject jcaller,
                                 jlong jurl_request_adapter,
                                 jstring jcontent_type) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   SetPostContentType(env, request_adapter, jcontent_type);
 
   request_adapter->EnableChunkedUpload();
 }
 
 static void AppendChunk(JNIEnv* env,
                         jobject jcaller,
                         jlong jurl_request_adapter,
                         jobject jchunk_byte_buffer,
                         jint jchunk_size,
                         jboolean jis_last_chunk) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   DCHECK(jchunk_byte_buffer);
 
   void* chunk = env->GetDirectBufferAddress(jchunk_byte_buffer);
   request_adapter->AppendChunk(reinterpret_cast<const char*>(chunk),
                                jchunk_size, jis_last_chunk);
 }
 
 /* synchronized */
 static void Start(JNIEnv* env, jobject jcaller, jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
@@ skipping 18 matching lines @@
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
   if (request_adapter != NULL)
     request_adapter->Cancel();
 }
 
 static jint GetErrorCode(JNIEnv* env,
                          jobject jcaller,
                          jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  if (request_adapter == NULL)

[mmenke, 2015/02/20 23:19:42]

I think these checks make a little more sense Java side - the NULLs originate
from how it manages the lifetime of C++-side objects.

It may even make sense to cache these in Java, after we've destroyed the
request.

[xunjieli, 2015/02/25 21:31:28]

Done.

+    return REQUEST_ERROR_UNKNOWN;
   int error_code = request_adapter->error_code();
   switch (error_code) {
     // TODO(mef): Investigate returning success on positive values, too, as
     // they technically indicate success.
     case net::OK:
       return REQUEST_ERROR_SUCCESS;
 
     // TODO(mef): Investigate this. The fact is that Chrome does not do this,
     // and this library is not just being used for downloads.
 
@@ skipping 20 matching lines @@
       return REQUEST_ERROR_TOO_MANY_REDIRECTS;
   }
   return REQUEST_ERROR_UNKNOWN;
 }
 
 static jstring GetErrorString(JNIEnv* env,
                               jobject jcaller,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  if (request_adapter == NULL)
+    return ConvertUTF8ToJavaString(env, "").Release();
   int error_code = request_adapter->error_code();
   char buffer[200];
   std::string error_string = net::ErrorToString(error_code);
   snprintf(buffer,
            sizeof(buffer),
            "System error: %s(%d)",
            error_string.c_str(),
            error_code);
   return ConvertUTF8ToJavaString(env, buffer).Release();
 }
 
 static jint GetHttpStatusCode(JNIEnv* env,
                               jobject jcaller,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  if (request_adapter == NULL)
+    return 0;
   return request_adapter->http_status_code();
 }
 
 static jstring GetHttpStatusText(JNIEnv* env,
                                  jobject jcaller,
                                  jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  if (request_adapter == NULL)
+    return ConvertUTF8ToJavaString(env, "").Release();
   return ConvertUTF8ToJavaString(env, request_adapter->http_status_text())
       .Release();
 }
 
 static jstring GetContentType(JNIEnv* env,
                               jobject jcaller,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
   if (request_adapter == NULL)
@@ skipping 19 matching lines @@
 static jstring GetHeader(JNIEnv* env,
                          jobject jcaller,
                          jlong jurl_request_adapter,
                          jstring jheader_name) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
   if (request_adapter == NULL)
     return NULL;
   std::string header_name = ConvertJavaStringToUTF8(env, jheader_name);
   std::string header_value = request_adapter->GetHeader(header_name);
-  if (!header_value.empty()) {
+  if (!header_value.empty())
     return ConvertUTF8ToJavaString(env, header_value.c_str()).Release();
-  } else {
-    return NULL;
-  }
+  return NULL;
 }
 
 static void GetAllHeaders(JNIEnv* env,
                           jobject jcaller,
                           jlong jurl_request_adapter,
                           jobject jheaders_map) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
   if (request_adapter == NULL)
     return;
@@ skipping 31 matching lines @@
     return ConvertUTF8ToJavaString(env, "").Release();
 
   std::string negotiated_protocol = request_adapter->GetNegotiatedProtocol();
   return ConvertUTF8ToJavaString(env, negotiated_protocol.c_str()).Release();
 }
 
 static void DisableRedirects(JNIEnv* env, jobject jcaller,
                              jlong jrequest_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jrequest_adapter);
-  if (request_adapter != NULL)
-    request_adapter->DisableRedirects();
+  DCHECK(request_adapter);

[xunjieli, 2015/02/20 22:39:18]

This method should only be called when the adapter is alive, so I have changed
it to a DCHECK.

+  request_adapter->DisableRedirects();
 }
 
 }  // namespace cronet