CacheStorage: Abort outstanding addAll() requests on error.

The spec requires that requests be aborted on addAll() failure:

https://w3c.github.io/ServiceWorker/#cache-addAll

See the sections that say "Terminate all the ongoing fetches
initiated by requests with the aborted flag set."  Note, there
appear to be a few places where this text is missing as noted in:

w3c/ServiceWorker#1543

Currently there is no way to reliably observe that this abort takes
place in a WPT test.  The AbortSignal is not updated for internal
abort decisions and the information is not propagated to the signal
on a service worker FetchEvent.request.  Therefore this test only adds
a simple unit test to verify the related AbortController was triggered.

Bug: 1130781
Change-Id: I0e0498741a6d387147a2ef0ea9d31feaa805dee8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429308
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812273}
GitOrigin-RevId: d6d55147aa1276bb6815e520b71b6c8277ece517

blink/renderer/modules/cache_storage/cache.cc

@@ -23,6 +23,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_request_init.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_response.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_throw_dom_exception.h"
+#include "third_party/blink/renderer/core/dom/abort_controller.h"
 #include "third_party/blink/renderer/core/dom/dom_exception.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/fetch/blob_bytes_consumer.h"
@@ -184,6 +185,8 @@ class Cache::BarrierCallbackForPutResponse final
                                 const ExceptionState& exception_state,
                                 int64_t trace_id)
       : resolver_(MakeGarbageCollected<ScriptPromiseResolver>(script_state)),
+        abort_controller_(
+            cache->CreateAbortController(ExecutionContext::From(script_state))),
         cache_(cache),
         method_name_(method_name),
         request_list_(request_list),
@@ -197,6 +200,8 @@ class Cache::BarrierCallbackForPutResponse final
   // Must be called prior to starting the load of any response.
   ScriptPromise Promise() const { return resolver_->Promise(); }
 
+  AbortSignal* Signal() const { return abort_controller_->signal(); }
+
   void CompletedResponse(int index,
                          Response* response,
                          scoped_refptr<BlobDataHandle> blob) {
@@ -251,18 +256,23 @@ class Cache::BarrierCallbackForPutResponse final
 
   void Trace(Visitor* visitor) const {
     visitor->Trace(resolver_);
+    visitor->Trace(abort_controller_);
     visitor->Trace(cache_);
     visitor->Trace(request_list_);
     visitor->Trace(response_list_);
   }
 
  private:
   void Stop() {
-    // TODO(crbug.com/1130781): abort outstanding requests
+    if (stopped_)
+      return;
+    abort_controller_->abort();
+    blob_list_.clear();
     stopped_ = true;
   }
 
   Member<ScriptPromiseResolver> resolver_;
+  Member<AbortController> abort_controller_;
   Member<Cache> cache_;
   const String method_name_;
   const HeapVector<Member<Request>> request_list_;
@@ -867,6 +877,10 @@ void Cache::Trace(Visitor* visitor) const {
   ScriptWrappable::Trace(visitor);
 }
 
+AbortController* Cache::CreateAbortController(ExecutionContext* context) {
+  return AbortController::Create(context);
+}
+
 ScriptPromise Cache::MatchImpl(ScriptState* script_state,
                                const Request* request,
                                const CacheQueryOptions* options) {
@@ -1054,8 +1068,13 @@ ScriptPromise Cache::AddAllImpl(ScriptState* script_state,
 
   // Begin loading each of the requests.
   for (wtf_size_t i = 0; i < request_list.size(); ++i) {
+    // Chain the AbortSignal objects together so the requests will abort if
+    // the |barrier_callback| encounters an error.
+    request_list[i]->signal()->Follow(barrier_callback->Signal());
+
     RequestInfo info;
     info.SetRequest(request_list[i]);
+
     auto* response_loader = MakeGarbageCollected<ResponseBodyLoader>(
         script_state, barrier_callback, i, /*require_ok_response=*/true,
         trace_id);

blink/renderer/modules/cache_storage/cache.h

@@ -40,6 +40,7 @@ struct TypeConverter<CacheQueryOptionsPtr, const blink::CacheQueryOptions*> {
 
 namespace blink {
 
+class AbortController;
 class CacheStorageBlobClientList;
 class ExceptionState;
 class Response;
@@ -49,7 +50,7 @@ class ScriptState;
 
 typedef RequestOrUSVString RequestInfo;
 
-class MODULES_EXPORT Cache final : public ScriptWrappable {
+class MODULES_EXPORT Cache : public ScriptWrappable {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
@@ -87,6 +88,10 @@ class MODULES_EXPORT Cache final : public ScriptWrappable {
 
   void Trace(Visitor*) const override;
 
+ protected:
+  // Virtual for testing.
+  virtual AbortController* CreateAbortController(ExecutionContext* context);
+
  private:
   class BarrierCallbackForPutResponse;
   class BarrierCallbackForPutComplete;

blink/renderer/modules/cache_storage/cache_test.cc

@@ -29,6 +29,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_request_init.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_response.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_response_init.h"
+#include "third_party/blink/renderer/core/dom/abort_controller.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/fetch/body_stream_buffer.h"
@@ -257,18 +258,46 @@ class NotImplementedErrorCache : public ErrorCacheForTests {
             mojom::blink::CacheStorageError::kErrorNotImplemented) {}
 };
 
+class TestCache : public Cache {
+ public:
+  TestCache(
+      GlobalFetch::ScopedFetcher* fetcher,
+      mojo::PendingAssociatedRemote<mojom::blink::CacheStorageCache> remote,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : Cache(fetcher, std::move(remote), std::move(task_runner)) {}
+
+  bool IsAborted() const {
+    return abort_controller_ && abort_controller_->signal()->aborted();
+  }
+
+  void Trace(Visitor* visitor) const override {
+    visitor->Trace(abort_controller_);
+    Cache::Trace(visitor);
+  }
+
+ protected:
+  AbortController* CreateAbortController(ExecutionContext* context) override {
+    if (!abort_controller_)
+      abort_controller_ = AbortController::Create(context);
+    return abort_controller_;
+  }
+
+ private:
+  Member<blink::AbortController> abort_controller_;
+};
+
 class CacheStorageTest : public PageTestBase {
  public:
   void SetUp() override { PageTestBase::SetUp(IntSize(1, 1)); }
 
-  Cache* CreateCache(ScopedFetcherForTests* fetcher,
-                     std::unique_ptr<ErrorCacheForTests> cache) {
+  TestCache* CreateCache(ScopedFetcherForTests* fetcher,
+                         std::unique_ptr<ErrorCacheForTests> cache) {
     mojo::AssociatedRemote<mojom::blink::CacheStorageCache> cache_remote;
     cache_ = std::move(cache);
     receiver_ = std::make_unique<
         mojo::AssociatedReceiver<mojom::blink::CacheStorageCache>>(
         cache_.get(), cache_remote.BindNewEndpointAndPassDedicatedReceiver());
-    return MakeGarbageCollected<Cache>(
+    return MakeGarbageCollected<TestCache>(
         fetcher, cache_remote.Unbind(),
         blink::scheduler::GetSingleThreadTaskRunnerForTesting());
   }
@@ -754,6 +783,35 @@ TEST_F(CacheStorageTest, Add) {
             test_cache()->GetAndClearLastErrorWebCacheMethodCalled());
 }
 
+// Verify an error response causes Cache::addAll() to trigger its associated
+// AbortController to cancel outstanding requests.
+TEST_F(CacheStorageTest, AddAllAbort) {
+  ScriptState::Scope scope(GetScriptState());
+  DummyExceptionStateForTesting exception_state;
+  auto* fetcher = MakeGarbageCollected<ScopedFetcherForTests>();
+  const String url = "http://www.cacheadd.test/";
+  const String content_type = "text/plain";
+  const String content = "hello cache";
+
+  TestCache* cache =
+      CreateCache(fetcher, std::make_unique<NotImplementedErrorCache>());
+
+  Request* request = NewRequestFromUrl(url);
+  fetcher->SetExpectedFetchUrl(&url);
+
+  Response* response = Response::error(GetScriptState());
+  fetcher->SetResponse(response);
+
+  HeapVector<RequestInfo> info_list;
+  info_list.push_back(RequestToRequestInfo(request));
+
+  ScriptPromise promise =
+      cache->addAll(GetScriptState(), info_list, exception_state);
+
+  EXPECT_EQ("TypeError: Request failed", GetRejectString(promise));
+  EXPECT_TRUE(cache->IsAborted());
+}
+
 }  // namespace
 
 }  // namespace blink