[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..2da0cccad1bfb09312b84b5a1d11984beaaf8f34 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);
-
   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);

[miloslav, 2015/02/26 15:56:49]

It's nice to have these checks, but it would be more useful if you these throw a
java exception rather than crashing the process (when enabled). Clients should
not crash the process if they get the lifecycle wrong.

Maybe a helper function or a macro will do the job? The alternative is to add
java-side checks before calling all the native methods (and then leaving these
DCHECKs just in case).

[xunjieli, 2015/02/26 17:54:00]

Acknowledged. Checking on java side before calling the methods might be better.
Looks like we should leave DCHECKs in these JNI methods, as it seems to be the
standard way in chromium (If we use @NativeClassQualifiedName, JNI generator
will automatically generate those DCHECKs). 

   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,7 @@ static jint GetErrorCode(JNIEnv* env,
                          jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   int error_code = request_adapter->error_code();
   switch (error_code) {
     // TODO(mef): Investigate returning success on positive values, too, as
@@ -286,6 +289,7 @@ static jstring GetErrorString(JNIEnv* env,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   int error_code = request_adapter->error_code();
   char buffer[200];
   std::string error_string = net::ErrorToString(error_code);
@@ -302,6 +306,7 @@ static jint GetHttpStatusCode(JNIEnv* env,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   return request_adapter->http_status_code();
 }
 
@@ -310,6 +315,7 @@ static jstring GetHttpStatusText(JNIEnv* env,
                                  jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter);
   return ConvertUTF8ToJavaString(env, request_adapter->http_status_text())
       .Release();
 }
@@ -319,8 +325,7 @@ static jstring GetContentType(JNIEnv* env,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
-  if (request_adapter == NULL)
-    return NULL;
+  DCHECK(request_adapter);
   std::string type = request_adapter->content_type();
   if (!type.empty()) {
     return ConvertUTF8ToJavaString(env, type.c_str()).Release();
@@ -334,8 +339,7 @@ static jlong GetContentLength(JNIEnv* env,
                               jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
-  if (request_adapter == NULL)
-    return 0;
+  DCHECK(request_adapter);
   return request_adapter->content_length();
 }
 
@@ -345,15 +349,12 @@ static jstring GetHeader(JNIEnv* env,
                          jstring jheader_name) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
-  if (request_adapter == NULL)
-    return NULL;
+  DCHECK(request_adapter);
   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,
@@ -362,8 +363,7 @@ static void GetAllHeaders(JNIEnv* env,
                           jobject jheaders_map) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
-  if (request_adapter == NULL)
-    return;
+  DCHECK(request_adapter);
 
   net::HttpResponseHeaders* headers = request_adapter->GetResponseHeaders();
   if (headers == NULL)
@@ -394,8 +394,7 @@ static jstring GetNegotiatedProtocol(JNIEnv* env,
                                      jlong jurl_request_adapter) {
   URLRequestAdapter* request_adapter =
       reinterpret_cast<URLRequestAdapter*>(jurl_request_adapter);
-  if (request_adapter == NULL)
-    return ConvertUTF8ToJavaString(env, "").Release();
+  DCHECK(request_adapter);
 
   std::string negotiated_protocol = request_adapter->GetNegotiatedProtocol();
   return ConvertUTF8ToJavaString(env, negotiated_protocol.c_str()).Release();
@@ -405,8 +404,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);
+  request_adapter->DisableRedirects();
 }
 
 }  // namespace cronet