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

components/cronet/android/chromium_url_request.cc

Index: components/cronet/android/chromium_url_request.cc
diff --git a/components/cronet/android/chromium_url_request.cc b/components/cronet/android/chromium_url_request.cc
index 156c0636a80f86836190af808b2d6d1d7045e6c8..328e42e7f42cc801d9b6c557b06ca65424ea8b22 100644
--- a/components/cronet/android/chromium_url_request.cc
+++ b/components/cronet/android/chromium_url_request.cc
@@ -40,8 +40,6 @@ net::RequestPriority ConvertRequestPriority(jint request_priority) {
 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);
 
@@ -164,6 +162,7 @@ static void SetUploadData(JNIEnv* env,
                           jbyteArray jcontent) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   SetPostContentType(env, request_adapter, jcontent_type);
 
   if (jcontent != NULL) {
@@ -184,6 +183,7 @@ static void SetUploadChannel(JNIEnv* env,
                              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);
@@ -195,6 +195,7 @@ static void EnableChunkedUpload(JNIEnv* env,
                                 jstring jcontent_type) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   SetPostContentType(env, request_adapter, jcontent_type);
 
   request_adapter->EnableChunkedUpload();
@@ -208,6 +209,7 @@ static void AppendChunk(JNIEnv* env,
                         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);
@@ -246,6 +248,8 @@ static jint GetErrorCode(JNIEnv* env,
                          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
@@ -286,6 +290,8 @@ static jstring GetErrorString(JNIEnv* env,
                               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);
@@ -302,6 +308,8 @@ static jint GetHttpStatusCode(JNIEnv* env,
                               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();
 }
 
@@ -310,6 +318,8 @@ static jstring GetHttpStatusText(JNIEnv* env,
                                  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();
 }
@@ -349,11 +359,9 @@ static jstring GetHeader(JNIEnv* env,
     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,
@@ -405,8 +413,8 @@ 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