Import of Volley from GitHub to AOSP.

  - 95f64de9bfa2ee44d6cf93a6dc4b980e5ec56f6a Fix leak of callbacks on canceled requests. by Jeff Davidson <jpd236@cornell.edu>

GitOrigin-RevId: 95f64de9bfa2ee44d6cf93a6dc4b980e5ec56f6a
Change-Id: I9537a9937f9c0e7078dfa8f8fdfcd8deb36417a7

5 files changed, 115 insertions, 26 deletions

diff --git a/src/main/java/com/android/volley/ExecutorDelivery.java b/src/main/java/com/android/volley/ExecutorDelivery.java
index 1babfcd..f36fb75 100644
--- a/src/main/java/com/android/volley/ExecutorDelivery.java
+++ b/src/main/java/com/android/volley/ExecutorDelivery.java
@@ -88,6 +88,13 @@ public class ExecutorDelivery implements ResponseDelivery {
         @SuppressWarnings("unchecked")
         @Override
         public void run() {
+            // NOTE: If cancel() is called off the thread that we're currently running in (by
+            // default, the main thread), we cannot guarantee that deliverResponse()/deliverError()
+            // won't be called, since it may be canceled after we check isCanceled() but before we
+            // deliver the response. Apps concerned about this guarantee must either call cancel()
+            // from the same thread or implement their own guarantee about not invoking their
+            // listener after cancel() has been called.
+
             // If this request has canceled, finish it and don't deliver.
             if (mRequest.isCanceled()) {
                 mRequest.finish("canceled-at-delivery");
diff --git a/src/main/java/com/android/volley/Request.java b/src/main/java/com/android/volley/Request.java
index a98277e..358a327 100644
--- a/src/main/java/com/android/volley/Request.java
+++ b/src/main/java/com/android/volley/Request.java
@@ -83,8 +83,12 @@ public abstract class Request<T> implements Comparable<Request<T>> {
     /** Default tag for {@link TrafficStats}. */
     private final int mDefaultTrafficStatsTag;
 
+    /** Lock to guard state which can be mutated after a request is added to the queue. */
+    private final Object mLock = new Object();
+
     /** Listener interface for errors. */
-    private final Response.ErrorListener mErrorListener;
+    // @GuardedBy("mLock")
+    private Response.ErrorListener mErrorListener;
 
     /** Sequence number of this request, used to enforce FIFO ordering. */
     private Integer mSequence;
@@ -96,9 +100,11 @@ public abstract class Request<T> implements Comparable<Request<T>> {
     private boolean mShouldCache = true;
 
     /** Whether or not this request has been canceled. */
+    // @GuardedBy("mLock")
     private boolean mCanceled = false;
 
     /** Whether or not a response has been delivered for this request yet. */
+    // @GuardedBy("mLock")
     private boolean mResponseDelivered = false;
 
     /** Whether the request should be retried in the event of an HTTP 5xx (server) error. */
@@ -118,11 +124,9 @@ public abstract class Request<T> implements Comparable<Request<T>> {
     private Object mTag;
 
     /** Listener that will be notified when a response has been delivered. */
+    // @GuardedBy("mLock")
     private NetworkRequestCompleteListener mRequestCompleteListener;
 
-    /** Object to guard access to mRequestCompleteListener. */
-    private final Object mLock = new Object();
-
     /**
      * Creates a new request with the given URL and error listener.  Note that
      * the normal response listener is not provided here as delivery of responses
@@ -320,17 +324,34 @@ public abstract class Request<T> implements Comparable<Request<T>> {
     }
 
     /**
-     * Mark this request as canceled.  No callback will be delivered.
+     * Mark this request as canceled.
+     *
+     * <p>No callback will be delivered as long as either:
+     * <ul>
+     *     <li>This method is called on the same thread as the {@link ResponseDelivery} is running
+     *     on. By default, this is the main thread.
+     *     <li>The request subclass being used overrides cancel() and ensures that it does not
+     *     invoke the listener in {@link #deliverResponse} after cancel() has been called in a
+     *     thread-safe manner.
+     * </ul>
+     *
+     * <p>There are no guarantees if both of these conditions aren't met.
      */
+    // @CallSuper
     public void cancel() {
-        mCanceled = true;
+        synchronized (mLock) {
+            mCanceled = true;
+            mErrorListener = null;
+        }
     }
 
     /**
      * Returns true if this request has been canceled.
      */
     public boolean isCanceled() {
-        return mCanceled;
+        synchronized (mLock) {
+            return mCanceled;
+        }
     }
 
     /**
@@ -549,14 +570,18 @@ public abstract class Request<T> implements Comparable<Request<T>> {
      * later in the request's lifetime for suppressing identical responses.
      */
     public void markDelivered() {
-        mResponseDelivered = true;
+        synchronized (mLock) {
+            mResponseDelivered = true;
+        }
     }
 
     /**
      * Returns true if this request has had a response delivered for it.
      */
     public boolean hasHadResponseDelivered() {
-        return mResponseDelivered;
+        synchronized (mLock) {
+            return mResponseDelivered;
+        }
     }
 
     /**
@@ -597,8 +622,12 @@ public abstract class Request<T> implements Comparable<Request<T>> {
      * @param error Error details
      */
     public void deliverError(VolleyError error) {
-        if (mErrorListener != null) {
-            mErrorListener.onErrorResponse(error);
+        Response.ErrorListener listener;
+        synchronized (mLock) {
+            listener = mErrorListener;
+        }
+        if (listener != null) {
+            listener.onErrorResponse(error);
         }
     }
 
@@ -619,10 +648,12 @@ public abstract class Request<T> implements Comparable<Request<T>> {
      * @param response received from the network
      */
     /* package */ void notifyListenerResponseReceived(Response<?> response) {
+        NetworkRequestCompleteListener listener;
         synchronized (mLock) {
-            if (mRequestCompleteListener != null) {
-               mRequestCompleteListener.onResponseReceived(this, response);
-            }
+            listener = mRequestCompleteListener;
+        }
+        if (listener != null) {
+            listener.onResponseReceived(this, response);
         }
     }
 
@@ -631,10 +662,12 @@ public abstract class Request<T> implements Comparable<Request<T>> {
      * a response which can be used for other, waiting requests.
      */
     /* package */ void notifyListenerResponseNotUsable() {
+        NetworkRequestCompleteListener listener;
         synchronized (mLock) {
-            if (mRequestCompleteListener != null) {
-                mRequestCompleteListener.onNoUsableResponseReceived(this);
-            }
+            listener = mRequestCompleteListener;
+        }
+        if (listener != null) {
+            listener.onNoUsableResponseReceived(this);
         }
     }
 
diff --git a/src/main/java/com/android/volley/toolbox/ImageRequest.java b/src/main/java/com/android/volley/toolbox/ImageRequest.java
index 0f33cd8..1db7e35 100644
--- a/src/main/java/com/android/volley/toolbox/ImageRequest.java
+++ b/src/main/java/com/android/volley/toolbox/ImageRequest.java
@@ -42,7 +42,11 @@ public class ImageRequest extends Request<Bitmap> {
     /** Default backoff multiplier for image requests */
     public static final float DEFAULT_IMAGE_BACKOFF_MULT = 2f;
 
-    private final Response.Listener<Bitmap> mListener;
+    /** Lock to guard mListener as it is cleared on cancel() and read on delivery. */
+    private final Object mLock = new Object();
+
+    // @GuardedBy("mLock")
+    private Response.Listener<Bitmap> mListener;
     private final Config mDecodeConfig;
     private final int mMaxWidth;
     private final int mMaxHeight;
@@ -215,9 +219,21 @@ public class ImageRequest extends Request<Bitmap> {
     }
 
     @Override
+    public void cancel() {
+        super.cancel();
+        synchronized (mLock) {
+            mListener = null;
+        }
+    }
+
+    @Override
     protected void deliverResponse(Bitmap response) {
-        if (mListener != null) {
-            mListener.onResponse(response);
+        Response.Listener<Bitmap> listener;
+        synchronized (mLock) {
+            listener = mListener;
+        }
+        if (listener != null) {
+            listener.onResponse(response);
         }
     }
 
diff --git a/src/main/java/com/android/volley/toolbox/JsonRequest.java b/src/main/java/com/android/volley/toolbox/JsonRequest.java
index 40877b1..f291076 100644
--- a/src/main/java/com/android/volley/toolbox/JsonRequest.java
+++ b/src/main/java/com/android/volley/toolbox/JsonRequest.java
@@ -39,7 +39,11 @@ public abstract class JsonRequest<T> extends Request<T> {
     private static final String PROTOCOL_CONTENT_TYPE =
         String.format("application/json; charset=%s", PROTOCOL_CHARSET);
 
-    private final Listener<T> mListener;
+    /** Lock to guard mListener as it is cleared on cancel() and read on delivery. */
+    private final Object mLock = new Object();
+
+    // @GuardedBy("mLock")
+    private Listener<T> mListener;
     private final String mRequestBody;
 
     /**
@@ -62,9 +66,21 @@ public abstract class JsonRequest<T> extends Request<T> {
     }
 
     @Override
+    public void cancel() {
+        super.cancel();
+        synchronized (mLock) {
+            mListener = null;
+        }
+    }
+
+    @Override
     protected void deliverResponse(T response) {
-        if (mListener != null) {
-            mListener.onResponse(response);
+        Response.Listener<T> listener;
+        synchronized (mLock) {
+            listener = mListener;
+        }
+        if (listener != null) {
+            listener.onResponse(response);
         }
     }
 
diff --git a/src/main/java/com/android/volley/toolbox/StringRequest.java b/src/main/java/com/android/volley/toolbox/StringRequest.java
index 05a62f6..9deba1d 100644
--- a/src/main/java/com/android/volley/toolbox/StringRequest.java
+++ b/src/main/java/com/android/volley/toolbox/StringRequest.java
@@ -28,7 +28,12 @@ import java.io.UnsupportedEncodingException;
  * A canned request for retrieving the response body at a given URL as a String.
  */
 public class StringRequest extends Request<String> {
-    private final Listener<String> mListener;
+
+    /** Lock to guard mListener as it is cleared on cancel() and read on delivery. */
+    private final Object mLock = new Object();
+
+    // @GuardedBy("mLock")
+    private Listener<String> mListener;
 
     /**
      * Creates a new request with the given method.
@@ -56,9 +61,21 @@ public class StringRequest extends Request<String> {
     }
 
     @Override
+    public void cancel() {
+        super.cancel();
+        synchronized (mLock) {
+            mListener = null;
+        }
+    }
+
+    @Override
     protected void deliverResponse(String response) {
-        if (mListener != null) {
-            mListener.onResponse(response);
+        Response.Listener<String> listener;
+        synchronized (mLock) {
+            listener = mListener;
+        }
+        if (listener != null) {
+            listener.onResponse(response);
         }
     }