Change RequestFinishedListener to provide executor

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

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
index 345692dc565c1e8efbbb3e1241d0c14a3e41c3bf..b4f990f77669b20e4a3e4896a47107aa7069ee71 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
@@ -22,6 +22,7 @@ import org.chromium.net.BidirectionalStream;
 import org.chromium.net.CronetEngine;
 import org.chromium.net.NetworkQualityRttListener;
 import org.chromium.net.NetworkQualityThroughputListener;
+import org.chromium.net.RequestFinishedListener;
 import org.chromium.net.UrlRequest;
 import org.chromium.net.urlconnection.CronetHttpURLConnection;
 import org.chromium.net.urlconnection.CronetURLStreamHandlerFactory;
@@ -68,6 +69,7 @@ public class CronetUrlRequestContext extends CronetEngine {
      * addition and removal may occur on a different thread from notification.
      */
     private final Object mNetworkQualityLock = new Object();
+    private final Object mFinishedListenerLock = new Object();

[tbansal1, 2016/07/26 20:37:26]

May be add a separate comment block for this.

[xunjieli, 2016/07/26 21:11:09]

Can you add a comment here?

[mgersh, 2016/07/27 20:54:12]

Done.

[mgersh, 2016/07/27 20:54:12]

Done.

 
     @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityRttListener> mRttListenerList =
@@ -77,7 +79,7 @@ public class CronetUrlRequestContext extends CronetEngine {
     private final ObserverList<NetworkQualityThroughputListener> mThroughputListenerList =
             new ObserverList<NetworkQualityThroughputListener>();
 
-    @GuardedBy("mNetworkQualityLock")
+    @GuardedBy("mFinishedListenerLock")
     private final ObserverList<RequestFinishedListener> mFinishedListenerList =
             new ObserverList<RequestFinishedListener>();
 
@@ -153,11 +155,9 @@ public class CronetUrlRequestContext extends CronetEngine {
             boolean disableConnectionMigration) {
         synchronized (mLock) {
             checkHaveAdapter();
-            boolean metricsCollectionEnabled = mNetworkQualityEstimatorEnabled;
-            if (metricsCollectionEnabled) { // Collect metrics only if someone is listening.
-                synchronized (mNetworkQualityLock) {
-                    metricsCollectionEnabled = !mFinishedListenerList.isEmpty();
-                }
+            boolean metricsCollectionEnabled = false;
+            synchronized (mFinishedListenerLock) {
+                metricsCollectionEnabled = !mFinishedListenerList.isEmpty();
             }
             return new CronetUrlRequest(this, url, priority, callback, executor, requestAnnotations,
                     metricsCollectionEnabled, disableCache, disableConnectionMigration);
@@ -379,14 +379,7 @@ public class CronetUrlRequestContext extends CronetEngine {
      */
     @Override
     public void addRequestFinishedListener(RequestFinishedListener listener) {
-        if (!mNetworkQualityEstimatorEnabled) {
-            throw new IllegalStateException("Network quality estimator must be enabled");
-        }
-        // RequestFinishedListener does not provide its own executor.
-        if (mNetworkQualityExecutor == null) {
-            throw new IllegalStateException("Executor must not be null");
-        }
-        synchronized (mNetworkQualityLock) {
+        synchronized (mFinishedListenerLock) {
             mFinishedListenerList.addObserver(listener);
         }
     }
@@ -396,10 +389,7 @@ public class CronetUrlRequestContext extends CronetEngine {
      */
     @Override
     public void removeRequestFinishedListener(RequestFinishedListener listener) {
-        if (!mNetworkQualityEstimatorEnabled) {
-            throw new IllegalStateException("Network quality estimator must be enabled");
-        }
-        synchronized (mNetworkQualityLock) {
+        synchronized (mFinishedListenerLock) {
             mFinishedListenerList.removeObserver(listener);
         }
     }
@@ -528,30 +518,18 @@ public class CronetUrlRequestContext extends CronetEngine {
     }
 
     void reportFinished(final CronetUrlRequest request) {
-        if (!mNetworkQualityEstimatorEnabled) {
-            return;
-        }
-        // If no request finished listener has been added, then mNetworkQualityExecutor may be
-        // null. Exit early to avoid posting to a null executor.
-        synchronized (mNetworkQualityLock) {
-            if (mFinishedListenerList.isEmpty()) {
-                return;
-            }
-        }
-        Runnable task = new Runnable() {
-            @Override
-            public void run() {
-                synchronized (mNetworkQualityLock) {
-                    UrlRequestInfo requestInfo = request.getRequestInfo();
-                    for (RequestFinishedListener listener : mFinishedListenerList) {
+        final UrlRequestInfo requestInfo = request.getRequestInfo();
+        synchronized (mFinishedListenerLock) {
+            for (final RequestFinishedListener listener : mFinishedListenerList) {
+                Runnable task = new Runnable() {
+                    @Override
+                    public void run() {
                         listener.onRequestFinished(requestInfo);
                     }
-                }
+                };
+                postObservationTaskToExecutor(listener.getExecutor(), task);

[xunjieli, 2016/07/26 21:11:09]

We shouldn't call into the executor, which might happen to be a direct executor.
The caller might spend considerable time doing things and we don't want to block
other requests which are trying to acquire this listener lock.

Can you make a copy of the list of listeners, and do the rest out of the
synchronized block?

[mgersh, 2016/07/27 20:54:12]

Done. I also changed mFinishedListenerList from an ObserverList to a normal
ArrayList, since if we're making a copy we're not using the ObserverList
functionality at all, and I don't think we need it.

[xunjieli, 2016/07/29 16:35:24]

Acknowledged. I agree with you.

             }
-        };
-        // Use {@link mNetworkQualityExecutor} since
-        // RequestFInishedListeners do not provide an executor.
-        postObservationTaskToExecutor(mNetworkQualityExecutor, task);
+        }
     }
 
     private static void postObservationTaskToExecutor(Executor executor, Runnable task) {