[Cronet] Make metrics reporting happen after terminal callbacks.

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

 // 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.
 
 package org.chromium.net.impl;
 
 import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNIAdditionalImport;
@@ skipping 33 matching lines @@
 @JNIAdditionalImport(VersionSafeCallbacks.class)
 @VisibleForTesting
 public final class CronetUrlRequest extends UrlRequestBase {
     private final boolean mAllowDirectExecutor;
 
     /* Native adapter object, owned by UrlRequest. */
     @GuardedBy("mUrlRequestAdapterLock")
     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,
      * and mWaitingOnRead.
      */
     private final Object mUrlRequestAdapterLock = new Object();
     private final CronetUrlRequestContext mRequestContext;
     private final Executor mExecutor;
 
     /*
      * URL chain contains the URL currently being requested, and
      * all URLs previously requested. New URLs are added before
      * mCallback.onRedirectReceived is called.
      */
     private final List<String> mUrlChain = new ArrayList<String>();
     private long mReceivedByteCountFromRedirects;
 
     private final VersionSafeCallbacks.UrlRequestCallback mCallback;
     private final String mInitialUrl;
     private final int mPriority;
     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;
 
     private CronetUploadDataStream mUploadDataStream;
 
     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.
      */
     private OnReadCompletedRunnable mOnReadCompletedTask;
 
     private Runnable mOnDestroyedCallbackForTesting;
 
     private static final class HeadersList extends ArrayList<Map.Entry<String, String>> {}
 
@@ skipping 124 matching lines @@
                                 }
                                 mUploadDataStream.attachNativeAdapterToRequest(mUrlRequestAdapter);
                                 startInternalLocked();
                             }
                         }
                     });
                     return;
                 }
             } 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;
             startInternalLocked();
         }
     }
 
     /*
      * Starts fully configured request. Could execute on UploadDataProvider executor.
      * Caller is expected to ensure that request isn't canceled and mUrlRequestAdapter is valid.
@@ skipping 41 matching lines @@
             }
         }
     }
 
     @Override
     public void cancel() {
         synchronized (mUrlRequestAdapterLock) {
             if (isDoneLocked() || !mStarted) {
                 return;
             }
-            destroyRequestAdapter(true);
+            nativeCancel(mUrlRequestAdapter);
         }
     }
 
     @Override
     public boolean isDone() {
         synchronized (mUrlRequestAdapterLock) {
             return isDoneLocked();
         }
     }
 
+    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;
     }
 
     @Override
     public void getStatus(UrlRequest.StatusListener unsafeListener) {
         final VersionSafeCallbacks.UrlRequestStatusListener listener =
                 new VersionSafeCallbacks.UrlRequestStatusListener(unsafeListener);
         synchronized (mUrlRequestAdapterLock) {
@@ skipping 39 matching lines @@
         } catch (RejectedExecutionException failException) {
             Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor",
                     failException);
             // If posting a task throws an exception, then we fail the request. This exception could
             // be permanent (executor shutdown), transient (AbortPolicy, or CallerRunsPolicy with
             // direct execution not permitted), or caused by the runnables we submit if
             // mUserExecutor is a direct executor and direct execution is not permitted. In the
             // 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));
         }
     }
 
     private static int convertRequestPriority(int priority) {
         switch (priority) {
             case Builder.REQUEST_PRIORITY_IDLE:
                 return RequestPriority.IDLE;
             case Builder.REQUEST_PRIORITY_LOWEST:
                 return RequestPriority.LOWEST;
@@ skipping 21 matching lines @@
     }
 
     private void checkNotStarted() {
         synchronized (mUrlRequestAdapterLock) {
             if (mStarted || isDoneLocked()) {
                 throw new IllegalStateException("Request is already started.");
             }
         }
     }
 
-    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.
      * Only called on the Executor.
      */
     private void onCallbackException(Exception e) {
         CallbackException requestError =
                 new CallbackExceptionImpl("Exception received from UrlRequest.Callback", e);
         Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in CalledByNative method", e);
-        failWithException(requestError);
+        failWithExceptionFromJava(requestError);
     }
 
     /**
      * Called when UploadDataProvider encounters an error.
      */
     void onUploadException(Throwable e) {
         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);
         }
     }
 
     ////////////////////////////////////////////////
     // Private methods called by the native code.
     // Always called on network thread.
     ////////////////////////////////////////////////
 
     /**
      * Called before following redirects. The redirect will only be followed if
@@ skipping 88 matching lines @@
      *        read(). Used as a minimal check that the buffer hasn't been
      *        modified while reading from the network.
      * @param receivedByteCount number of bytes received.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onReadCompleted(final ByteBuffer byteBuffer, int bytesRead, int initialPosition,
             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;
         }
         if (mOnReadCompletedTask == null) {
             mOnReadCompletedTask = new OnReadCompletedRunnable();
         }
         byteBuffer.position(initialPosition + bytesRead);
         mOnReadCompletedTask.mByteBuffer = byteBuffer;
         postTaskToExecutor(mOnReadCompletedTask);
     }
 
     /**
      * Called when request is completed successfully, no callbacks will be
      * called afterwards.
      *
      * @param receivedByteCount number of bytes received.
      */
     @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) {
                     Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onComplete method", e);
                 }
             }
         };
         postTaskToExecutor(task);
     }
 
     /**
      * Called when error has occured, no callbacks will be called afterwards.
      *
      * @param errorCode Error code represented by {@code UrlRequestError} that should be mapped
      *                  to one of {@link NetworkException#ERROR_HOSTNAME_NOT_RESOLVED
      *                  NetworkException.ERROR_*}.
      * @param nativeError native net error code.
      * @param errorString textual representation of the error code.
      * @param receivedByteCount number of bytes received.
      */
     @SuppressWarnings("unused")
     @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.
     }
 
     /**
      * Called when request is canceled, no callbacks will be called afterwards.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onCanceled() {
-        mFinishedReason = RequestFinishedInfo.CANCELED;
+        setIsDone(null, RequestFinishedInfo.CANCELED);
         Runnable task = new Runnable() {
             @Override
             public void run() {
                 try {
                     mCallback.onCanceled(CronetUrlRequest.this, mResponseInfo);
                 } catch (Exception e) {
                     Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onCanceled method", e);
                 }
             }
         };
@@ skipping 20 matching lines @@
     /**
      * Called by the native code to report metrics.
      */
     @SuppressWarnings("unused")
     @CalledByNative
     private void onMetricsCollected(long requestStartMs, long dnsStartMs, long dnsEndMs,
             long connectStartMs, long connectEndMs, long sslStartMs, long sslEndMs,
             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. */
     void checkCallingThread() {
         if (!mAllowDirectExecutor && mRequestContext.isNetworkThread(Thread.currentThread())) {
             throw new InlineExecutionProhibitedException();
         }
     }
 
     private int mapUrlRequestErrorToApiErrorCode(int errorCode) {
@@ skipping 42 matching lines @@
     private native void nativeStart(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native void nativeFollowDeferredRedirect(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestAdapter")
     private native boolean nativeReadData(
             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(
             long nativePtr, VersionSafeCallbacks.UrlRequestStatusListener listener);
 }