[Cronet] Add callback wrapper classes to enforce API version checking.

components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.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 android.os.ConditionVariable;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.Process;
@@ skipping 15 matching lines @@
 import org.chromium.net.UrlRequest;
 import org.chromium.net.urlconnection.CronetHttpURLConnection;
 import org.chromium.net.urlconnection.CronetURLStreamHandlerFactory;
 
 import java.net.Proxy;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import javax.annotation.concurrent.GuardedBy;
 
 /**
  * CronetEngine using Chromium HTTP stack implementation.
@@ skipping 66 matching lines @@
      * computed by the network quality estimator.
      */
     @GuardedBy("mNetworkQualityLock")
     private int mDownstreamThroughputKbps = RttThroughputValues.INVALID_RTT_THROUGHPUT;
 
     @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityRttListener> mRttListenerList =
             new ObserverList<NetworkQualityRttListener>();
 
     @GuardedBy("mNetworkQualityLock")
+    private final Map<NetworkQualityRttListener,
+            VersionSafeCallbacks.NetworkQualityRttListenerWrapper> mRttListenerMap =
+            new HashMap<NetworkQualityRttListener,
+                    VersionSafeCallbacks.NetworkQualityRttListenerWrapper>();
+
+    @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityThroughputListener> mThroughputListenerList =
             new ObserverList<NetworkQualityThroughputListener>();
 
+    @GuardedBy("mNetworkQualityLock")
+    private final Map<NetworkQualityThroughputListener,
+            VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper> mThroughputListenerMap =
+            new HashMap<NetworkQualityThroughputListener,
+                    VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper>();
+
     @GuardedBy("mFinishedListenerLock")
-    private final List<RequestFinishedInfo.Listener> mFinishedListenerList =
-            new ArrayList<RequestFinishedInfo.Listener>();
+    private final Map<RequestFinishedInfo.Listener,

[kapishnikov, 2016/11/18 20:11:18]

Can we instead of using the map, implement equals() & hashCode() method on
VersionSafeCallbacks.RequestFinishedInfoListener? Objects that are referencing
the same RequestFinishedInfo.Listener are equal. This would allow to continue
using lists instead of maps.

[pauljensen, 2016/11/19 01:12:18]

I thought about that but it had a couple downsides:
1. The delegation of equals() and hashcode() are somewhat counterintuitive and
kinda abnormal behavior that seems like it could trip someone up in the future
(e.g. someone removing the delegating equals() and hashcode() methods, and us
not having a test to check that we actually remove items from the list).
2. If you want to delete an element you have to create another wrapping class
just for the sake of searching for the pre-existing member and then throw it
away which seemed fairly inefficient.

I don't feel strongly though, if you prefer this behavior let me know and I'll
implement.

[kapishnikov, 2016/11/21 17:35:22]

I personally like the idea of implementing equals(). But I agree that some
functionality may not be intuitive, e.g. to delete a listener, it will be
necessary to create a new object. What about creating a new private nested class
that would wrap a list/map and do the necessary logic? At least, the top class
will be simplified.

One advantage of a list versus a map is that it uses less memory but I don't
think it should be a contributing factor here.

[pauljensen, 2016/11/22 12:38:02]

I delegated the equals() and hashCode() to the wrapped listener, and documented
it as such.  I think the is the simplest and cleanest implementation.  Let me
know what you think.

+            VersionSafeCallbacks.RequestFinishedInfoListener> mFinishedListenerMap =
+            new HashMap<RequestFinishedInfo.Listener,
+                    VersionSafeCallbacks.RequestFinishedInfoListener>();
 
     /**
      * Synchronize access to mCertVerifierData.
      */
     private ConditionVariable mWaitGetCertVerifierDataComplete = new ConditionVariable();
 
     /** Holds CertVerifier data. */
     private String mCertVerifierData;
 
     private ConditionVariable mStopNetLogCompleted;
@@ skipping 268 matching lines @@
             throw new IllegalStateException("Network quality estimator must be enabled");
         }
         synchronized (mNetworkQualityLock) {
             if (mRttListenerList.isEmpty()) {
                 synchronized (mLock) {
                     checkHaveAdapter();
                     nativeProvideRTTObservations(mUrlRequestContextAdapter, true);
                 }
             }
             mRttListenerList.addObserver(listener);
+            mRttListenerMap.put(
+                    listener, new VersionSafeCallbacks.NetworkQualityRttListenerWrapper(listener));
         }
     }
 
     @Override
     public void removeRttListener(NetworkQualityRttListener listener) {
         if (!mNetworkQualityEstimatorEnabled) {
             throw new IllegalStateException("Network quality estimator must be enabled");
         }
         synchronized (mNetworkQualityLock) {
-            mRttListenerList.removeObserver(listener);
-            if (mRttListenerList.isEmpty()) {
-                synchronized (mLock) {
-                    checkHaveAdapter();
-                    nativeProvideRTTObservations(mUrlRequestContextAdapter, false);
+            if (mRttListenerList.removeObserver(listener)) {
+                mRttListenerMap.remove(listener);
+                if (mRttListenerList.isEmpty()) {
+                    synchronized (mLock) {
+                        checkHaveAdapter();
+                        nativeProvideRTTObservations(mUrlRequestContextAdapter, false);
+                    }
                 }
             }
         }
     }
 
     @Override
     public void addThroughputListener(NetworkQualityThroughputListener listener) {
         if (!mNetworkQualityEstimatorEnabled) {
             throw new IllegalStateException("Network quality estimator must be enabled");
         }
         synchronized (mNetworkQualityLock) {
             if (mThroughputListenerList.isEmpty()) {
                 synchronized (mLock) {
                     checkHaveAdapter();
                     nativeProvideThroughputObservations(mUrlRequestContextAdapter, true);
                 }
             }
             mThroughputListenerList.addObserver(listener);
+            mThroughputListenerMap.put(listener,
+                    new VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper(listener));
         }
     }
 
     @Override
     public void removeThroughputListener(NetworkQualityThroughputListener listener) {
         if (!mNetworkQualityEstimatorEnabled) {
             throw new IllegalStateException("Network quality estimator must be enabled");
         }
         synchronized (mNetworkQualityLock) {
-            mThroughputListenerList.removeObserver(listener);
-            if (mThroughputListenerList.isEmpty()) {
-                synchronized (mLock) {
-                    checkHaveAdapter();
-                    nativeProvideThroughputObservations(mUrlRequestContextAdapter, false);
+            if (mThroughputListenerList.removeObserver(listener)) {
+                mThroughputListenerMap.remove(listener);
+                if (mThroughputListenerList.isEmpty()) {
+                    synchronized (mLock) {
+                        checkHaveAdapter();
+                        nativeProvideThroughputObservations(mUrlRequestContextAdapter, false);
+                    }
                 }
             }
         }
     }
 
     @Override
     public void addRequestFinishedListener(RequestFinishedInfo.Listener listener) {
         synchronized (mFinishedListenerLock) {
-            mFinishedListenerList.add(listener);
+            mFinishedListenerMap.put(
+                    listener, new VersionSafeCallbacks.RequestFinishedInfoListener(listener));
         }
     }
 
     @Override
     public void removeRequestFinishedListener(RequestFinishedInfo.Listener listener) {
         synchronized (mFinishedListenerLock) {
-            mFinishedListenerList.remove(listener);
+            mFinishedListenerMap.remove(listener);
         }
     }
 
     boolean hasRequestFinishedListener() {
         synchronized (mFinishedListenerLock) {
-            return !mFinishedListenerList.isEmpty();
+            return !mFinishedListenerMap.isEmpty();
         }
     }
 
     @Override
     public URLConnection openConnection(URL url) {
         return openConnection(url, Proxy.NO_PROXY);
     }
 
     @Override
     public URLConnection openConnection(URL url, Proxy proxy) {
@@ skipping 112 matching lines @@
             mHttpRttMs = httpRttMs;
             mTransportRttMs = transportRttMs;
             mDownstreamThroughputKbps = downstreamThroughputKbps;
         }
     }
 
     @SuppressWarnings("unused")
     @CalledByNative
     private void onRttObservation(final int rttMs, final long whenMs, final int source) {
         synchronized (mNetworkQualityLock) {
-            for (final NetworkQualityRttListener listener : mRttListenerList) {
+            for (NetworkQualityRttListener unsafeListener : mRttListenerList) {
+                final VersionSafeCallbacks.NetworkQualityRttListenerWrapper listener =
+                        mRttListenerMap.get(unsafeListener);
                 Runnable task = new Runnable() {
                     @Override
                     public void run() {
                         listener.onRttObservation(rttMs, whenMs, source);
                     }
                 };
                 postObservationTaskToExecutor(listener.getExecutor(), task);
             }
         }
     }
 
     @SuppressWarnings("unused")
     @CalledByNative
     private void onThroughputObservation(
             final int throughputKbps, final long whenMs, final int source) {
         synchronized (mNetworkQualityLock) {
-            for (final NetworkQualityThroughputListener listener : mThroughputListenerList) {
+            for (NetworkQualityThroughputListener unsafeListener : mThroughputListenerList) {
+                final VersionSafeCallbacks.NetworkQualityThroughputListenerWrapper listener =
+                        mThroughputListenerMap.get(unsafeListener);
                 Runnable task = new Runnable() {
                     @Override
                     public void run() {
                         listener.onThroughputObservation(throughputKbps, whenMs, source);
                     }
                 };
                 postObservationTaskToExecutor(listener.getExecutor(), task);
             }
         }
     }
 
     @SuppressWarnings("unused")
     @CalledByNative
     private void onGetCertVerifierData(String certVerifierData) {
         mCertVerifierData = certVerifierData;
         mWaitGetCertVerifierDataComplete.open();
     }
 
     void reportFinished(final RequestFinishedInfo requestInfo) {
-        ArrayList<RequestFinishedInfo.Listener> currentListeners;
+        ArrayList<VersionSafeCallbacks.RequestFinishedInfoListener> currentListeners;
         synchronized (mFinishedListenerLock) {
-            currentListeners = new ArrayList<RequestFinishedInfo.Listener>(mFinishedListenerList);
+            currentListeners = new ArrayList<VersionSafeCallbacks.RequestFinishedInfoListener>(
+                    mFinishedListenerMap.values());
         }
-        for (final RequestFinishedInfo.Listener listener : currentListeners) {
+        for (final VersionSafeCallbacks.RequestFinishedInfoListener listener : currentListeners) {
             Runnable task = new Runnable() {
                 @Override
                 public void run() {
                     listener.onRequestFinished(requestInfo);
                 }
             };
             postObservationTaskToExecutor(listener.getExecutor(), task);
         }
     }
 
@@ skipping 54 matching lines @@
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeProvideRTTObservations(long nativePtr, boolean should);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeProvideThroughputObservations(long nativePtr, boolean should);
 
     public boolean isNetworkThread(Thread thread) {
         return thread == mNetworkThread;
     }
 }