[Cronet] Make metrics reporting happen after terminal callbacks.

components/cronet/android/cronet_url_request_adapter.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/cronet_url_request_adapter.h"
 
 #include <limits>
 #include <utility>
 #include <vector>
 
@@ skipping 74 matching lines @@
     const GURL& url,
     net::RequestPriority priority,
     jboolean jdisable_cache,
     jboolean jdisable_connection_migration,
     jboolean jenable_metrics)
     : context_(context),
       initial_url_(url),
       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)
     load_flags_ |= net::LOAD_DISABLE_CACHE;
   if (jdisable_connection_migration == JNI_TRUE)
     load_flags_ |= net::LOAD_DISABLE_CONNECTION_MIGRATION;
 }
 
 CronetURLRequestAdapter::~CronetURLRequestAdapter() {
   DCHECK(context_->IsOnNetworkThread());
@@ skipping 33 matching lines @@
   DCHECK(!context_->IsOnNetworkThread());
   DCHECK(!upload_);
   upload_ = std::move(upload);
 }
 
 void CronetURLRequestAdapter::Start(JNIEnv* env,
                                     const JavaParamRef<jobject>& jcaller) {
   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(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller) {
   context_->PostTaskToNetworkThread(
       FROM_HERE,
       base::Bind(
           &CronetURLRequestAdapter::FollowDeferredRedirectOnNetworkThread,
-          base::Unretained(this)));
+          weak_ptr_factory_.GetWeakPtr()));
 }
 
 jboolean CronetURLRequestAdapter::ReadData(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     const JavaParamRef<jobject>& jbyte_buffer,
     jint jposition,
     jint jlimit) {
   DCHECK_LT(jposition, jlimit);
 
   void* data = env->GetDirectBufferAddress(jbyte_buffer);
   if (!data)
     return JNI_FALSE;
 
   scoped_refptr<IOBufferWithByteBuffer> read_buffer(
       new IOBufferWithByteBuffer(env, jbyte_buffer, data, jposition, jlimit));
 
   int remaining_capacity = jlimit - jposition;
 
   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(
     net::URLRequest* request,
     const net::RedirectInfo& redirect_info,
     bool* defer_redirect) {
   DCHECK(context_->IsOnNetworkThread());
   JNIEnv* env = base::android::AttachCurrentThread();
   cronet::Java_CronetUrlRequest_onRedirectReceived(
       env, owner_.obj(),
@@ skipping 19 matching lines @@
   request->ContinueWithCertificate(nullptr, nullptr);
 }
 
 void CronetURLRequestAdapter::OnSSLCertificateError(
     net::URLRequest* request,
     const net::SSLInfo& ssl_info,
     bool fatal) {
   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,
                                                 int net_error) {
   DCHECK_NE(net::ERR_IO_PENDING, net_error);
   DCHECK(context_->IsOnNetworkThread());
 
   if (net_error != net::OK) {
     ReportError(request, net_error);
     return;
@@ skipping 15 matching lines @@
                                               int bytes_read) {
   DCHECK(context_->IsOnNetworkThread());
 
   if (bytes_read < 0) {
     ReportError(request, bytes_read);
     return;
   }
 
   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(
         env, owner_.obj(), read_buffer_->byte_buffer(), bytes_read,
         read_buffer_->initial_position(), read_buffer_->initial_limit(),
         request->GetTotalReceivedBytes());
     // Free the read buffer. This lets the Java ByteBuffer be freed, if the
     // embedder releases it, too.
     read_buffer_ = nullptr;
   }
@@ skipping 65 matching lines @@
   read_buffer_ = read_buffer;
 
   int result = url_request_->Read(read_buffer_.get(), buffer_size);
   // If IO is pending, wait for the URLRequest to call OnReadCompleted.
   if (result == net::ERR_IO_PENDING)
     return;
 
   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);
   DCHECK_LT(net_error, 0);
   DCHECK_EQ(request, url_request_.get());
 
   net::NetErrorDetails net_error_details;
   url_request_->PopulateNetErrorDetails(&net_error_details);
   VLOG(1) << "Error " << net::ErrorToString(net_error)
           << " on chromium request: " << initial_url_.possibly_invalid_spec();
   JNIEnv* env = base::android::AttachCurrentThread();
   cronet::Java_CronetUrlRequest_onError(
       env, owner_.obj(), NetErrorToUrlRequestError(net_error), net_error,
       net_error_details.quic_connection_error,
       ConvertUTF8ToJavaString(env, net::ErrorToString(net_error)).obj(),
       request->GetTotalReceivedBytes());
+  DestroyOnNetworkThread();
 }
 
 void CronetURLRequestAdapter::MaybeReportMetrics(JNIEnv* env) const {
   // If there was an exception while starting the CronetUrlRequest, there won't
   // be a native URLRequest. In this case, the caller gets the exception
   // immediately, and the onFailed callback isn't called, so don't report
   // metrics either.
   if (!enable_metrics_ || !url_request_) {
     return;
   }
@@ skipping 22 matching lines @@
       metrics_util::ConvertTime(metrics.push_end, start_ticks, start_time),
       metrics_util::ConvertTime(metrics.receive_headers_end, start_ticks,
                                 start_time),
       metrics_util::ConvertTime(base::TimeTicks::Now(), start_ticks,
                                 start_time),
       metrics.socket_reused, url_request_->GetTotalSentBytes(),
       url_request_->GetTotalReceivedBytes());
 }
 
 }  // namespace cronet