[Cronet] Make metrics reporting happen after terminal callbacks.

components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
index eb2abaf83195efa16810c38ae43f3583407dbae8..07affb421e4c92cfc368b410a6866eef1648e956 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
@@ -51,13 +51,11 @@ public final class CronetUrlRequest extends UrlRequestBase {
     private long mUrlRequestAdapter;
 
     @GuardedBy("mUrlRequestAdapterLock")
-    private boolean mStarted = false;
+    private boolean mStarted;
     @GuardedBy("mUrlRequestAdapterLock")
-    private boolean mWaitingOnRedirect = false;
+    private boolean mWaitingOnRedirect;
     @GuardedBy("mUrlRequestAdapterLock")
-    private boolean mWaitingOnRead = false;
-    @GuardedBy("mUrlRequestAdapterLock")
-    private RequestFinishedInfo.Metrics mMetrics;
+    private boolean mWaitingOnRead;
 
     /*
      * Synchronize access to mUrlRequestAdapter, mStarted, mWaitingOnRedirect,
@@ -81,9 +79,6 @@ public final class CronetUrlRequest extends UrlRequestBase {
     private String mInitialMethod;
     private final HeadersList mRequestHeaders = new HeadersList();
     private final Collection<Object> mRequestAnnotations;
-    @RequestFinishedInfoImpl.FinishedReason
-    private int mFinishedReason;
-    private CronetException mException;
     private final boolean mDisableCache;
     private final boolean mDisableConnectionMigration;
 
@@ -91,6 +86,15 @@ public final class CronetUrlRequest extends UrlRequestBase {
 
     private UrlResponseInfoImpl mResponseInfo;
 
+    // mFinishedReason and mException should only be updated once with mUrlRequestAdapterLock held.
+    // They are read on executor's thread after the last update.
+    @RequestFinishedInfoImpl.FinishedReason
+    private int mFinishedReason;
+    private CronetException mException;
+
+    // Accessed only from the network thread.
+    private boolean mMetricsCollected;
+
     /*
      * Listener callback is repeatedly invoked when each read is completed, so it
      * is cached as a member variable.
@@ -235,8 +239,9 @@ public final class CronetUrlRequest extends UrlRequestBase {
                 }
             } catch (RuntimeException e) {
                 // If there's an exception, cleanup and then throw the
-                // exception to the caller.
-                destroyRequestAdapter(false);
+                // exception to the caller. Use null here to avoid rethrowing
+                // in onDestroyed().
+                failWithExceptionFromJava(null);
                 throw e;
             }
             mStarted = true;
@@ -298,7 +303,7 @@ public final class CronetUrlRequest extends UrlRequestBase {
             if (isDoneLocked() || !mStarted) {
                 return;
             }
-            destroyRequestAdapter(true);
+            nativeCancel(mUrlRequestAdapter);
         }
     }
 
@@ -309,6 +314,19 @@ public final class CronetUrlRequest extends UrlRequestBase {
         }
     }
 
+    private void setIsDone(
+            CronetException exception, @RequestFinishedInfoImpl.FinishedReason int finishedReason) {
+        synchronized (mUrlRequestAdapterLock) {
+            assert mUrlRequestAdapter != 0;
+            assert mException == null;
+            mException = exception;
+            mFinishedReason = finishedReason;
+            assert mException == null || mFinishedReason == RequestFinishedInfo.FAILED;
+            mRequestContext.onRequestDestroyed();
+            mUrlRequestAdapter = 0;
+        }
+    }
+
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean isDoneLocked() {
         return mStarted && mUrlRequestAdapter == 0;
@@ -368,7 +386,7 @@ public final class CronetUrlRequest extends UrlRequestBase {
             // latter two cases, there is at least have a chance to inform the embedder of the
             // request's failure, since failWithException does not enforce that onFailed() is not
             // executed inline.
-            failWithException(
+            failWithExceptionFromJava(
                     new CronetExceptionImpl("Exception posting task to executor", failException));
         }
     }
@@ -410,20 +428,6 @@ public final class CronetUrlRequest extends UrlRequestBase {
         }
     }
 
-    private void destroyRequestAdapter(boolean sendOnCanceled) {
-        synchronized (mUrlRequestAdapterLock) {
-            if (mUrlRequestAdapter == 0) {
-                return;
-            }
-            nativeDestroy(mUrlRequestAdapter, sendOnCanceled);
-            mRequestContext.onRequestDestroyed();
-            mUrlRequestAdapter = 0;
-            if (mOnDestroyedCallbackForTesting != null) {
-                mOnDestroyedCallbackForTesting.run();
-            }
-        }
-    }
-
     /**
      * If callback method throws an exception, request gets canceled
      * and exception is reported via onFailed listener callback.
@@ -433,7 +437,7 @@ public final class CronetUrlRequest extends UrlRequestBase {
         CallbackException requestError =
                 new CallbackExceptionImpl("Exception received from UrlRequest.Callback", e);
         Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in CalledByNative method", e);
-        failWithException(requestError);
+        failWithExceptionFromJava(requestError);
     }
 
     /**
@@ -443,34 +447,20 @@ public final class CronetUrlRequest extends UrlRequestBase {
         CallbackException uploadError =
                 new CallbackExceptionImpl("Exception received from UploadDataProvider", e);
         Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in upload method", e);
-        failWithException(uploadError);
+        failWithExceptionFromJava(uploadError);
     }
 
     /**
-     * Fails the request with an exception. Can be called on any thread.
+     * Fails the request with an exception on any thread. This method should only be used when error
+     * originates from Java.
      */
-    private void failWithException(final CronetException exception) {
-        mException = exception;
+    private void failWithExceptionFromJava(final CronetException exception) {
         synchronized (mUrlRequestAdapterLock) {
             if (isDoneLocked()) {
                 return;
             }
-            destroyRequestAdapter(false);
-        }
-        Runnable task = new Runnable() {
-            @Override
-            public void run() {
-                try {
-                    mCallback.onFailed(CronetUrlRequest.this, mResponseInfo, exception);
-                } catch (Exception e) {
-                    Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onFailed method", e);
-                }
-            }
-        };
-        try {
-            mExecutor.execute(task);
-        } catch (RejectedExecutionException e) {
-            Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor", e);
+            nativeDestroy(mUrlRequestAdapter);
+            setIsDone(exception, RequestFinishedInfo.FAILED);
         }
     }
 
@@ -579,7 +569,7 @@ public final class CronetUrlRequest extends UrlRequestBase {
             int initialLimit, long receivedByteCount) {
         mResponseInfo.setReceivedByteCount(mReceivedByteCountFromRedirects + receivedByteCount);
         if (byteBuffer.position() != initialPosition || byteBuffer.limit() != initialLimit) {
-            failWithException(
+            failWithExceptionFromJava(
                     new CronetExceptionImpl("ByteBuffer modified externally during read", null));
             return;
         }
@@ -600,19 +590,11 @@ public final class CronetUrlRequest extends UrlRequestBase {
     @SuppressWarnings("unused")
     @CalledByNative
     private void onSucceeded(long receivedByteCount) {
-        mFinishedReason = RequestFinishedInfo.SUCCEEDED;
+        setIsDone(null, RequestFinishedInfo.SUCCEEDED);
         mResponseInfo.setReceivedByteCount(mReceivedByteCountFromRedirects + receivedByteCount);
         Runnable task = new Runnable() {
             @Override
             public void run() {
-                synchronized (mUrlRequestAdapterLock) {
-                    if (isDoneLocked()) {
-                        return;
-                    }
-                    // Destroy adapter first, so request context could be shut
-                    // down from the listener.
-                    destroyRequestAdapter(false);
-                }
                 try {
                     mCallback.onSucceeded(CronetUrlRequest.this, mResponseInfo);
                 } catch (Exception e) {
@@ -637,18 +619,20 @@ public final class CronetUrlRequest extends UrlRequestBase {
     @CalledByNative
     private void onError(int errorCode, int nativeError, int nativeQuicError, String errorString,
             long receivedByteCount) {
-        mFinishedReason = RequestFinishedInfo.FAILED;
         if (mResponseInfo != null) {
             mResponseInfo.setReceivedByteCount(mReceivedByteCountFromRedirects + receivedByteCount);
         }
+        CronetException exception;
         if (errorCode == NetworkException.ERROR_QUIC_PROTOCOL_FAILED) {
-            failWithException(new QuicExceptionImpl(
-                    "Exception in CronetUrlRequest: " + errorString, nativeError, nativeQuicError));
+            exception = new QuicExceptionImpl(
+                    "Exception in CronetUrlRequest: " + errorString, nativeError, nativeQuicError);
         } else {
             int javaError = mapUrlRequestErrorToApiErrorCode(errorCode);
-            failWithException(new NetworkExceptionImpl(
-                    "Exception in CronetUrlRequest: " + errorString, javaError, nativeError));
+            exception = new NetworkExceptionImpl(
+                    "Exception in CronetUrlRequest: " + errorString, javaError, nativeError);
         }
+        setIsDone(exception, RequestFinishedInfo.FAILED);
+        // onDestroyed() will propagate mException to callback.
     }
 
     /**
@@ -657,7 +641,7 @@ public final class CronetUrlRequest extends UrlRequestBase {
     @SuppressWarnings("unused")
     @CalledByNative
     private void onCanceled() {
-        mFinishedReason = RequestFinishedInfo.CANCELED;
+        setIsDone(null, RequestFinishedInfo.CANCELED);
         Runnable task = new Runnable() {
             @Override
             public void run() {
@@ -698,21 +682,47 @@ public final class CronetUrlRequest extends UrlRequestBase {
             long sendingStartMs, long sendingEndMs, long pushStartMs, long pushEndMs,
             long responseStartMs, long requestEndMs, boolean socketReused, long sentByteCount,
             long receivedByteCount) {
-        synchronized (mUrlRequestAdapterLock) {
-            if (mMetrics != null) {
-                throw new IllegalStateException("Metrics collection should only happen once.");
-            }
-            mMetrics = new CronetMetrics(requestStartMs, dnsStartMs, dnsEndMs, connectStartMs,
-                    connectEndMs, sslStartMs, sslEndMs, sendingStartMs, sendingEndMs, pushStartMs,
-                    pushEndMs, responseStartMs, requestEndMs, socketReused, sentByteCount,
-                    receivedByteCount);
+        if (mMetricsCollected) {
+            throw new IllegalStateException("Metrics collection should only happen once.");
         }
-        mRequestContext.reportFinished(getRequestFinishedInfo());
+        mMetricsCollected = true;
+        CronetMetrics metrics = new CronetMetrics(requestStartMs, dnsStartMs, dnsEndMs,
+                connectStartMs, connectEndMs, sslStartMs, sslEndMs, sendingStartMs, sendingEndMs,
+                pushStartMs, pushEndMs, responseStartMs, requestEndMs, socketReused, sentByteCount,
+                receivedByteCount);
+        mRequestContext.reportFinished(new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations,
+                metrics, mFinishedReason, mResponseInfo, mException));
     }
 
-    private RequestFinishedInfo getRequestFinishedInfo() {
-        return new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations, mMetrics,
-                mFinishedReason, mResponseInfo, mException);
+    /**
+     * Called when the native adapter is destroyed.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onDestroyed() {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mOnDestroyedCallbackForTesting != null) {
+                mOnDestroyedCallbackForTesting.run();
+            }
+            if (mException == null) {
+                return;
+            }
+        }
+        Runnable task = new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    mCallback.onFailed(CronetUrlRequest.this, mResponseInfo, mException);
+                } catch (Exception e) {
+                    Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onFailed method", e);
+                }
+            }
+        };
+        try {
+            mExecutor.execute(task);
+        } catch (RejectedExecutionException e) {
+            Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor", e);
+        }
     }
 
     /** Enforces prohibition of direct execution. */
@@ -775,7 +785,10 @@ public final class CronetUrlRequest extends UrlRequestBase {
             long nativePtr, ByteBuffer byteBuffer, int position, int capacity);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
-    private native void nativeDestroy(long nativePtr, boolean sendOnCanceled);
+    private native void nativeCancel(long nativePtr);
+
+    @NativeClassQualifiedName("CronetURLRequestAdapter")
+    private native void nativeDestroy(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeGetStatus(