[Cronet] Make metrics reporting happen after terminal callbacks.

components/cronet/android/cronet_url_request_adapter.cc

Index: components/cronet/android/cronet_url_request_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_adapter.cc b/components/cronet/android/cronet_url_request_adapter.cc
index 19437d59c9ec815557188fb4efbfe765550ad5ec..d80c1e90904f76d120288f279a60488642253ea2 100644
--- a/components/cronet/android/cronet_url_request_adapter.cc
+++ b/components/cronet/android/cronet_url_request_adapter.cc
@@ -92,7 +92,8 @@ CronetURLRequestAdapter::CronetURLRequestAdapter(
       initial_priority_(priority),
       initial_method_("GET"),
       load_flags_(context->default_load_flags()),
-      enable_metrics_(jenable_metrics == JNI_TRUE) {
+      enable_metrics_(jenable_metrics == JNI_TRUE),
+      weak_ptr_factory_(this) {
   DCHECK(!context_->IsOnNetworkThread());
   owner_.Reset(env, jurl_request);
   if (jdisable_cache == JNI_TRUE)
@@ -146,18 +147,19 @@ void CronetURLRequestAdapter::Start(JNIEnv* env,
   DCHECK(!context_->IsOnNetworkThread());
   context_->PostTaskToNetworkThread(
       FROM_HERE, base::Bind(&CronetURLRequestAdapter::StartOnNetworkThread,
-                            base::Unretained(this)));
+                            weak_ptr_factory_.GetWeakPtr()));
 }
 
 void CronetURLRequestAdapter::GetStatus(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
-    const JavaParamRef<jobject>& jstatus_listener) const {
+    const JavaParamRef<jobject>& jstatus_listener) {
   base::android::ScopedJavaGlobalRef<jobject> status_listener_ref;
   status_listener_ref.Reset(env, jstatus_listener);
   context_->PostTaskToNetworkThread(
-      FROM_HERE, base::Bind(&CronetURLRequestAdapter::GetStatusOnNetworkThread,
-                            base::Unretained(this), status_listener_ref));
+      FROM_HERE,
+      base::Bind(&CronetURLRequestAdapter::GetStatusOnNetworkThread,
+                 weak_ptr_factory_.GetWeakPtr(), status_listener_ref));
 }
 
 void CronetURLRequestAdapter::FollowDeferredRedirect(
@@ -167,7 +169,7 @@ void CronetURLRequestAdapter::FollowDeferredRedirect(
       FROM_HERE,
       base::Bind(
           &CronetURLRequestAdapter::FollowDeferredRedirectOnNetworkThread,
-          base::Unretained(this)));
+          weak_ptr_factory_.GetWeakPtr()));
 }
 
 jboolean CronetURLRequestAdapter::ReadData(
@@ -189,23 +191,23 @@ jboolean CronetURLRequestAdapter::ReadData(
 
   context_->PostTaskToNetworkThread(
       FROM_HERE, base::Bind(&CronetURLRequestAdapter::ReadDataOnNetworkThread,
-                            base::Unretained(this),
-                            read_buffer,
+                            weak_ptr_factory_.GetWeakPtr(), read_buffer,
                             remaining_capacity));
   return JNI_TRUE;
 }
 
 void CronetURLRequestAdapter::Destroy(JNIEnv* env,
-                                      const JavaParamRef<jobject>& jcaller,
-                                      jboolean jsend_on_canceled) {
-  // Destroy could be called from any thread, including network thread (if
-  // posting task to executor throws an exception), but is posted, so |this|
-  // is valid until calling task is complete. Destroy() is always called from
-  // within a synchronized java block that guarantees no future posts to the
-  // network thread with the adapter pointer.
+                                      const JavaParamRef<jobject>& jcaller) {
   context_->PostTaskToNetworkThread(
       FROM_HERE, base::Bind(&CronetURLRequestAdapter::DestroyOnNetworkThread,
-                            base::Unretained(this), jsend_on_canceled));
+                            weak_ptr_factory_.GetWeakPtr()));
+}
+
+void CronetURLRequestAdapter::Cancel(JNIEnv* env,
+                                     const JavaParamRef<jobject>& jcaller) {
+  context_->PostTaskToNetworkThread(
+      FROM_HERE, base::Bind(&CronetURLRequestAdapter::CancelOnNetworkThread,

[mef, 2017/04/27 21:33:13]

Hrm, from comment on WeakPtrFactory:

// Note that GetWeakPtr must be called on one and only one TaskQueue or thread
// and the WeakPtr must only be dereferenced and invalidated on that same
// TaskQueue/thread. A WeakPtr instance can be copied and posted to other
// sequences though as long as it is not dereferenced (WeakPtr<T>::get()).

CronetURLRequestAdapter::Cancel() could be called on any thread though. 
What am I missing?

[xunjieli, 2017/04/28 13:36:47]

Ah, thanks, You are right! I missed this. I tried another approach which keeps
the native code as it is. PTAL.

+                            weak_ptr_factory_.GetWeakPtr()));
 }
 
 void CronetURLRequestAdapter::OnReceivedRedirect(
@@ -245,12 +247,7 @@ void CronetURLRequestAdapter::OnSSLCertificateError(
   DCHECK(context_->IsOnNetworkThread());
   request->Cancel();
   int net_error = net::MapCertStatusToNetError(ssl_info.cert_status);
-  JNIEnv* env = base::android::AttachCurrentThread();
-  cronet::Java_CronetUrlRequest_onError(
-      env, owner_.obj(), NetErrorToUrlRequestError(net_error), net_error,
-      net::QUIC_NO_ERROR,
-      ConvertUTF8ToJavaString(env, net::ErrorToString(net_error)).obj(),
-      request->GetTotalReceivedBytes());
+  ReportError(request, net_error);

[mef, 2017/04/27 21:33:13]

nice!

 }
 
 void CronetURLRequestAdapter::OnResponseStarted(net::URLRequest* request,
@@ -286,8 +283,9 @@ void CronetURLRequestAdapter::OnReadCompleted(net::URLRequest* request,
 
   if (bytes_read == 0) {
     JNIEnv* env = base::android::AttachCurrentThread();
-    cronet::Java_CronetUrlRequest_onSucceeded(
-        env, owner_.obj(), url_request_->GetTotalReceivedBytes());
+    cronet::Java_CronetUrlRequest_onSucceeded(env, owner_.obj(),
+                                              request->GetTotalReceivedBytes());
+    DestroyOnNetworkThread();
   } else {
     JNIEnv* env = base::android::AttachCurrentThread();
     cronet::Java_CronetUrlRequest_onReadCompleted(
@@ -373,16 +371,20 @@ void CronetURLRequestAdapter::ReadDataOnNetworkThread(
   OnReadCompleted(url_request_.get(), result);
 }
 
-void CronetURLRequestAdapter::DestroyOnNetworkThread(bool send_on_canceled) {
+void CronetURLRequestAdapter::DestroyOnNetworkThread() {
   DCHECK(context_->IsOnNetworkThread());
   JNIEnv* env = base::android::AttachCurrentThread();
-  if (send_on_canceled) {
-    cronet::Java_CronetUrlRequest_onCanceled(env, owner_.obj());
-  }
+  cronet::Java_CronetUrlRequest_onDestroyed(env, owner_.obj());
   MaybeReportMetrics(env);
   delete this;
 }
 
+void CronetURLRequestAdapter::CancelOnNetworkThread() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  cronet::Java_CronetUrlRequest_onCanceled(env, owner_.obj());
+  DestroyOnNetworkThread();
+}
+
 void CronetURLRequestAdapter::ReportError(net::URLRequest* request,
                                           int net_error) {
   DCHECK_NE(net::ERR_IO_PENDING, net_error);
@@ -399,6 +401,7 @@ void CronetURLRequestAdapter::ReportError(net::URLRequest* request,
       net_error_details.quic_connection_error,
       ConvertUTF8ToJavaString(env, net::ErrorToString(net_error)).obj(),
       request->GetTotalReceivedBytes());
+  DestroyOnNetworkThread();
 }
 
 void CronetURLRequestAdapter::MaybeReportMetrics(JNIEnv* env) const {