feedv2: Make image fetches cancelable

Bug: 1119567
Change-Id: I78ce53c8fb7255315588bfd64b7044cc0c69d94c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2365363Reviewed-by: Cathy Li <chili@chromium.org>
Reviewed-by: Dan H <harringtond@chromium.org>
Commit-Queue: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806737}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedImageFetchClient.java

@@ -35,10 +35,22 @@ public class FeedImageFetchClient implements ImageFetchClient {
         }
     }
 
+    @Override
+    public int sendCancelableRequest(
+            String url, ImageFetchClient.HttpResponseConsumer responseConsumer) {
+        assert ThreadUtils.runningOnUiThread();
+        return FeedImageFetchClientJni.get().sendRequest(url, responseConsumer);
+    }
+
     @Override
     public void sendRequest(String url, ImageFetchClient.HttpResponseConsumer responseConsumer) {
+        sendCancelableRequest(url, responseConsumer);
+    }
+
+    @Override
+    public void cancel(int requestId) {
         assert ThreadUtils.runningOnUiThread();
-        FeedImageFetchClientJni.get().sendRequest(url, responseConsumer);
+        FeedImageFetchClientJni.get().cancel(requestId);
     }
 
     @CalledByNative
@@ -49,6 +61,7 @@ public class FeedImageFetchClient implements ImageFetchClient {
 
     @NativeMethods
     interface Natives {
-        void sendRequest(String url, ImageFetchClient.HttpResponseConsumer responseConsumer);
+        int sendRequest(String url, ImageFetchClient.HttpResponseConsumer responseConsumer);
+        void cancel(int requestId);
     }
 }

chrome/browser/android/feed/v2/feed_image_fetch_client.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "components/feed/core/v2/public/feed_service.h"
+#include "components/feed/core/v2/public/feed_stream_api.h"
 #include "components/feed/core/v2/public/types.h"
 
 using base::android::JavaParamRef;
@@ -26,9 +27,21 @@ void OnFetchFinished(JNIEnv* env,
       base::android::ToJavaByteArray(env, response.response_bytes));
 }
 
+FeedStreamApi* GetFeedStream() {
+  Profile* profile = ProfileManager::GetLastUsedProfile();
+  if (!profile)
+    return nullptr;
+
+  FeedService* feed_service = FeedServiceFactory::GetForBrowserContext(profile);
+  if (!feed_service)
+    return nullptr;
+
+  return feed_service->GetStream();
+}
+
 }  // namespace
 
-void JNI_FeedImageFetchClient_SendRequest(
+jint JNI_FeedImageFetchClient_SendRequest(
     JNIEnv* env,
     const JavaParamRef<jstring>& j_url,
     const JavaParamRef<jobject>& j_response_callback) {
@@ -36,17 +49,24 @@ void JNI_FeedImageFetchClient_SendRequest(
   // with OnFetchFinished.
   base::android::ScopedJavaGlobalRef<jobject> callback(j_response_callback);
 
-  Profile* profile = ProfileManager::GetLastUsedProfile();
-  if (!profile)
-    return OnFetchFinished(env, std::move(callback), {});
+  FeedStreamApi* stream = GetFeedStream();
+  if (!stream) {
+    OnFetchFinished(env, std::move(callback), {});
+    return 0;
+  }
 
-  FeedService* feed_service = FeedServiceFactory::GetForBrowserContext(profile);
-  if (!feed_service || !feed_service->GetStream())
-    return OnFetchFinished(env, std::move(callback), {});
+  return stream
+      ->FetchImage(GURL(base::android::ConvertJavaStringToUTF8(env, j_url)),
+                   base::BindOnce(&OnFetchFinished, env, std::move(callback)))
+      .GetUnsafeValue();
+}
+
+void JNI_FeedImageFetchClient_Cancel(JNIEnv* env, jint j_request_id) {
+  FeedStreamApi* stream = GetFeedStream();
+  if (!stream)
+    return;
 
-  feed_service->GetStream()->FetchImage(
-      GURL(base::android::ConvertJavaStringToUTF8(env, j_url)),
-      base::BindOnce(&OnFetchFinished, env, std::move(callback)));
+  stream->CancelImageFetch(ImageFetchId::FromUnsafeValue(j_request_id));
 }
 
 }  // namespace feed

chrome/browser/xsurface/android/java/src/org/chromium/chrome/browser/xsurface/ImageFetchClient.java

@@ -37,6 +37,22 @@ public interface ImageFetchClient {
      *
      * @param url URL to request
      * @param responseConsumer The callback to call with the response
+     * @return Request ID that can be passed to cancel()
+     */
+    default int sendCancelableRequest(String url, HttpResponseConsumer responseConsumer) {
+        return 0;
+    }
+
+    /**
+     * Send a GET request. TODO(iwells): Remove when the caller switches to the cancelable version.
      */
     default void sendRequest(String url, HttpResponseConsumer responseConsumer) {}
+
+    /**
+     * Cancel a pending request. Causes the request's response callback to be called with an empty
+     * response body and net::Error::ERR_ABORTED.
+     *
+     * @param requestId ID of request to be canceled.
+     */
+    default void cancel(int requestId) {}
 }

components/feed/core/v2/feed_stream.cc

@@ -627,10 +627,14 @@ void FeedStream::FinishClearAll() {
   delegate_->ClearAll();
 }
 
-void FeedStream::FetchImage(
+ImageFetchId FeedStream::FetchImage(
     const GURL& url,
     base::OnceCallback<void(NetworkResponse)> callback) {
-  image_fetcher_->Fetch(url, std::move(callback));
+  return image_fetcher_->Fetch(url, std::move(callback));
+}
+
+void FeedStream::CancelImageFetch(ImageFetchId id) {
+  image_fetcher_->Cancel(id);
 }
 
 void FeedStream::UploadAction(

components/feed/core/v2/feed_stream.h

@@ -129,8 +129,10 @@ class FeedStream : public FeedStreamApi,
   bool IsArticlesListVisible() override;
   std::string GetClientInstanceId() override;
   void ExecuteRefreshTask() override;
-  void FetchImage(const GURL& url,
-                  base::OnceCallback<void(NetworkResponse)> callback) override;
+  ImageFetchId FetchImage(
+      const GURL& url,
+      base::OnceCallback<void(NetworkResponse)> callback) override;
+  void CancelImageFetch(ImageFetchId id) override;
   void LoadMore(SurfaceId surface_id,
                 base::OnceCallback<void(bool)> callback) override;
   void ExecuteOperations(

components/feed/core/v2/feed_stream_unittest.cc

@@ -264,12 +264,18 @@ class TestImageFetcher : public ImageFetcher {
   explicit TestImageFetcher(
       scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory)
       : ImageFetcher(url_loader_factory) {}
-  void Fetch(const GURL& url,
-             base::OnceCallback<void(NetworkResponse)> callback) override {
+  ImageFetchId Fetch(
+      const GURL& url,
+      base::OnceCallback<void(NetworkResponse)> callback) override {
     // Emulate a response.
     NetworkResponse response = {"dummyresponse", 200};
     std::move(callback).Run(std::move(response));
+    return id_generator_.GenerateNextId();
   }
+  void Cancel(ImageFetchId id) override {}
+
+ private:
+  ImageFetchId::Generator id_generator_;
 };
 
 class TestFeedNetwork : public FeedNetwork {

components/feed/core/v2/image_fetcher.cc

@@ -6,6 +6,7 @@
 
 #include "components/feed/core/v2/metrics_reporter.h"
 #include "components/feed/core/v2/public/types.h"
+#include "net/base/net_errors.h"
 #include "net/http/http_request_headers.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
@@ -18,7 +19,7 @@ ImageFetcher::ImageFetcher(
 
 ImageFetcher::~ImageFetcher() = default;
 
-void ImageFetcher::Fetch(const GURL& url, ImageCallback callback) {
+ImageFetchId ImageFetcher::Fetch(const GURL& url, ImageCallback callback) {
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("interest_feedv2_image_send", R"(
         semantics {
@@ -48,29 +49,73 @@ void ImageFetcher::Fetch(const GURL& url, ImageCallback callback) {
   auto simple_loader = network::SimpleURLLoader::Create(
       std::move(resource_request), traffic_annotation);
   auto* const simple_loader_ptr = simple_loader.get();
+
+  ImageFetchId id = id_generator_.GenerateNextId();
+  bool inserted =
+      pending_requests_
+          .try_emplace(id, std::move(simple_loader), std::move(callback))
+          .second;
+  DCHECK(inserted);
+
   simple_loader_ptr->DownloadToString(
       url_loader_factory_.get(),
       base::BindOnce(&ImageFetcher::OnFetchComplete, weak_factory_.GetWeakPtr(),
-                     std::move(simple_loader), std::move(callback)),
+                     id),
       network::SimpleURLLoader::kMaxBoundedStringDownloadSize);
+  return id;
 }
 
-void ImageFetcher::OnFetchComplete(
-    std::unique_ptr<network::SimpleURLLoader> simple_loader,
-    ImageCallback callback,
-    std::unique_ptr<std::string> response_data) {
-  MetricsReporter::OnImageFetched(
-      response_data ? simple_loader->ResponseInfo()->headers->response_code()
-                    : simple_loader->NetError());
-  NetworkResponse response{std::string(), simple_loader->NetError()};
-  if (simple_loader->ResponseInfo() && simple_loader->ResponseInfo()->headers) {
+void ImageFetcher::OnFetchComplete(ImageFetchId id,
+                                   std::unique_ptr<std::string> response_data) {
+  base::Optional<PendingRequest> request = RemovePending(id);
+  if (!request)
+    return;
+
+  NetworkResponse response;
+  if (request->loader->ResponseInfo() &&
+      request->loader->ResponseInfo()->headers) {
     response.status_code =
-        simple_loader->ResponseInfo()->headers->response_code();
+        request->loader->ResponseInfo()->headers->response_code();
+  } else {
+    response.status_code = request->loader->NetError();
   }
+  MetricsReporter::OnImageFetched(response.status_code);
 
   if (response_data)
     response.response_bytes = std::move(*response_data);
-  std::move(callback).Run(std::move(response));
+  std::move(request->callback).Run(std::move(response));
+}
+
+void ImageFetcher::Cancel(ImageFetchId id) {
+  base::Optional<PendingRequest> request = RemovePending(id);
+  if (!request)
+    return;
+
+  // Cancel the fetch before running the callback.
+  request->loader.reset();
+  std::move(request->callback)
+      .Run({/*response_bytes=*/std::string(), net::Error::ERR_ABORTED});
 }
 
+base::Optional<ImageFetcher::PendingRequest> ImageFetcher::RemovePending(
+    ImageFetchId id) {
+  auto iterator = pending_requests_.find(id);
+  if (iterator == pending_requests_.end())
+    return base::nullopt;
+
+  auto request = base::make_optional(std::move(iterator->second));
+  pending_requests_.erase(iterator);
+  return request;
+}
+
+ImageFetcher::PendingRequest::PendingRequest(
+    std::unique_ptr<network::SimpleURLLoader> loader,
+    ImageCallback callback)
+    : loader(std::move(loader)), callback(std::move(callback)) {}
+ImageFetcher::PendingRequest::PendingRequest(
+    ImageFetcher::PendingRequest&& other) = default;
+ImageFetcher::PendingRequest& ImageFetcher::PendingRequest::operator=(
+    ImageFetcher::PendingRequest&& other) = default;
+ImageFetcher::PendingRequest::~PendingRequest() = default;
+
 }  // namespace feed

components/feed/core/v2/image_fetcher.h

@@ -6,8 +6,11 @@
 #define COMPONENTS_FEED_CORE_V2_IMAGE_FETCHER_H_
 
 #include "base/callback.h"
+#include "base/containers/flat_map.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
+#include "components/feed/core/v2/public/types.h"
 #include "url/gurl.h"
 
 namespace network {
@@ -29,14 +32,29 @@ class ImageFetcher {
   ImageFetcher(const ImageFetcher&) = delete;
   ImageFetcher& operator=(const ImageFetcher&) = delete;
 
-  virtual void Fetch(const GURL& url, ImageCallback callback);
+  virtual ImageFetchId Fetch(const GURL& url, ImageCallback callback);
+  virtual void Cancel(ImageFetchId id);
 
  private:
+  struct PendingRequest {
+    PendingRequest(std::unique_ptr<network::SimpleURLLoader> loader,
+                   ImageCallback callback);
+    PendingRequest(PendingRequest&& other);
+    PendingRequest& operator=(PendingRequest&& other);
+    ~PendingRequest();
+
+    std::unique_ptr<network::SimpleURLLoader> loader;
+    ImageCallback callback;
+  };
+
   // Called when fetch request completes.
-  void OnFetchComplete(std::unique_ptr<network::SimpleURLLoader> simple_loader,
-                       ImageCallback callback,
+  void OnFetchComplete(ImageFetchId id,
                        std::unique_ptr<std::string> response_data);
 
+  base::Optional<PendingRequest> RemovePending(ImageFetchId id);
+
+  ImageFetchId::Generator id_generator_;
+  base::flat_map<ImageFetchId, PendingRequest> pending_requests_;
   const scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory_;
   base::WeakPtrFactory<ImageFetcher> weak_factory_{this};
 };

components/feed/core/v2/image_fetcher_unittest.cc

@@ -151,5 +151,39 @@ TEST_F(ImageFetcherTest, SendParallelRequestsValidResponses) {
               HasSubstr("example2_response"));
 }
 
+TEST_F(ImageFetcherTest, CancelRunsCallback) {
+  CallbackReceiver<NetworkResponse> receiver;
+  ImageFetchId id =
+      image_fetcher()->Fetch(GURL("https://example.com"), receiver.Bind());
+
+  image_fetcher()->Cancel(id);
+  ASSERT_TRUE(receiver.GetResult());
+  EXPECT_EQ(std::string(), receiver.GetResult()->response_bytes);
+  EXPECT_EQ(net::Error::ERR_ABORTED, receiver.GetResult()->status_code);
+}
+
+TEST_F(ImageFetcherTest, CancelThenRespond) {
+  CallbackReceiver<NetworkResponse> receiver;
+  ImageFetchId id =
+      image_fetcher()->Fetch(GURL("https://example.com"), receiver.Bind());
+
+  image_fetcher()->Cancel(id);
+  Respond("example1_response", net::HTTP_OK);
+  ASSERT_TRUE(receiver.GetResult());
+  EXPECT_EQ(std::string(), receiver.GetResult()->response_bytes);
+  EXPECT_EQ(net::Error::ERR_ABORTED, receiver.GetResult()->status_code);
+}
+
+TEST_F(ImageFetcherTest, CallbackCallsCancel) {
+  // Ensure nothing terrible happens if cancel is called from the callback.
+  ImageFetchId id;
+  id = image_fetcher()->Fetch(
+      GURL("https://example.com"),
+      base::BindLambdaForTesting(
+          [&](NetworkResponse response) { image_fetcher()->Cancel(id); }));
+
+  image_fetcher()->Cancel(id);
+}
+
 }  // namespace
 }  // namespace feed

components/feed/core/v2/public/feed_stream_api.h

@@ -78,10 +78,15 @@ class FeedStreamApi {
                         base::OnceCallback<void(bool)> callback) = 0;
 
   // Request to fetch and image for use in the feed. Calls |callback|
-  // with the network response when complete.
-  virtual void FetchImage(
+  // with the network response when complete. The returned ImageFetchId can be
+  // passed to CancelImageFetch() to cancel the request.
+  virtual ImageFetchId FetchImage(
       const GURL& url,
       base::OnceCallback<void(NetworkResponse)> callback) = 0;
+  // If |id| matches an active fetch, cancels the fetch and runs the callback
+  // with an empty response body and status_code=net::Error::ERR_ABORTED. If
+  // |id| doesn't match an active fetch, nothing happens.
+  virtual void CancelImageFetch(ImageFetchId id) = 0;
 
   // Apply |operations| to the stream model. Does nothing if the model is not
   // yet loaded.

components/feed/core/v2/public/types.h

@@ -32,6 +32,7 @@ struct DisplayMetrics {
 // A unique ID for an ephemeral change.
 using EphemeralChangeId = util::IdTypeU32<class EphemeralChangeIdClass>;
 using SurfaceId = util::IdTypeU32<class SurfaceIdClass>;
+using ImageFetchId = util::IdTypeU32<class ImageFetchIdClass>;
 
 struct NetworkResponseInfo {
   NetworkResponseInfo();
@@ -55,6 +56,7 @@ struct NetworkResponse {
   // HTTP status code if available, or net::Error otherwise.
   int status_code;
 
+  NetworkResponse() = default;
   NetworkResponse(NetworkResponse&& other) = default;
   NetworkResponse& operator=(NetworkResponse&& other) = default;
 };