Add a list of BackgroundFetchRecord objects to BackgroundFetchRegistration.

When we allow access to active background fetches, BackgroundFetchRecords for
the fetch can be accessed from multiple places. It simplifies matters if
we return the same record for a request whenever it's accessed.

This CL adds a (private) list of BackgroundFetchRecords to the
BackgroundFetchRegistration object, and updates it every time match and
matchAll() are called.

The second change introduced here is to not immediately resolve
responseReady() if the fetch is active and a response for the request
isn't yet available. Once the fetch has completed, or there's a response
available for the request, we resolve pending promises. We also make sure to
return the same promise (resolved or unresolved) for a given record,
every time responseReady is called.

For a more detailed discussion, see the following doc:
https://docs.google.com/document/d/1CrbWrnnshhyp_SfiAeuODpnQX36GK3Bsi19rXQGez6Q/edit?usp=sharing

Bug: 875201

Change-Id: I8cb386efd19086c0993ad2be2fb2691ad90597ec
Reviewed-on: https://chromium-review.googlesource.com/c/1336151
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610410}

third_party/WebKit/LayoutTests/external/wpt/background-fetch/fetch.https.window-expected.txt

@@ -14,5 +14,6 @@ PASS Using Background Fetch to fetch a non-existent resource should fail.
 PASS Fetches with mixed content should fail.
 PASS Matching to a single request should work
 PASS Matching to a non-existing request should work
+PASS Matching multiple times on the same request works as expected.
 Harness: the test ran to completion.
 

third_party/WebKit/LayoutTests/external/wpt/background-fetch/fetch.https.window.js

@@ -300,3 +300,27 @@ backgroundFetchTest(async (test, backgroundFetch) => {
 
 }, 'Matching to a non-existing request should work');
 
+backgroundFetchTest(async (test, backgroundFetch) => {
+  const registrationId = 'matchexistingrequesttwice';
+  const registration =
+    await backgroundFetch.fetch(registrationId, 'resources/feature-name.txt');
+
+  assert_equals(registration.id, registrationId);
+
+  const {type, eventRegistration, results} = await getMessageFromServiceWorker();
+  assert_equals('backgroundfetchsuccess', type);
+  assert_equals(results.length, 2);
+
+  assert_equals(eventRegistration.id, registration.id);
+  assert_equals(eventRegistration.result, 'success');
+  assert_equals(eventRegistration.failureReason, '');
+
+  assert_true(results[0].url.includes('resources/feature-name.txt'));
+  assert_equals(results[0].status, 200);
+  assert_equals(results[0].text, 'Background Fetch');
+
+  assert_true(results[1].url.includes('resources/feature-name.txt'));
+  assert_equals(results[1].status, 200);
+  assert_equals(results[1].text, 'error');
+
+}, 'Matching multiple times on the same request works as expected.');

third_party/WebKit/LayoutTests/external/wpt/background-fetch/service_workers/sw.js

@@ -8,17 +8,25 @@ async function getFetchResult(record) {
   return {
     url: response.url,
     status: response.status,
-    text: await response.text(),
+    text: await response.text().catch(() => 'error'),
   };
 }
 
 function handleBackgroundFetchEvent(event) {
   let matchFunction = null;
+
   switch (event.registration.id) {
     case 'matchexistingrequest':
       matchFunction = event.registration.match.bind(
           event.registration, '/background-fetch/resources/feature-name.txt');
       break;
+    case 'matchexistingrequesttwice':
+      matchFunction = (async () => {
+        const match1 = await event.registration.match('/background-fetch/resources/feature-name.txt');
+        const match2 = await event.registration.match('/background-fetch/resources/feature-name.txt');
+        return [match1, match2];
+    }).bind(event.registration);
+      break;
     case 'matchmissingrequest':
       matchFunction = event.registration.match.bind(
           event.registration, '/background-fetch/resources/missing.txt');
@@ -38,7 +46,7 @@ function handleBackgroundFetchEvent(event) {
       })
       // Extract responses.
       .then(records =>
-            Promise.all(records.map(record => getFetchResult(record))))
+        Promise.all(records.map(record => getFetchResult(record))))
       // Clone registration and send message.
       .then(results => {
         const registrationCopy = cloneRegistration(event.registration);

third_party/blink/renderer/modules/background_fetch/background_fetch_record.cc

@@ -3,21 +3,56 @@
 // found in the LICENSE file.
 
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_record.h"
-
 #include "third_party/blink/renderer/core/fetch/request.h"
 #include "third_party/blink/renderer/core/fetch/response.h"
+#include "third_party/blink/renderer/platform/bindings/script_state.h"
 
 namespace blink {
 
 BackgroundFetchRecord::BackgroundFetchRecord(Request* request,
-                                             Response* response,
-                                             bool aborted)
-    : request_(request), response_(response), aborted_(aborted) {
+                                             ScriptState* script_state)
+    : request_(request), script_state_(script_state) {
   DCHECK(request_);
+  DCHECK(script_state_);
 }
 
 BackgroundFetchRecord::~BackgroundFetchRecord() = default;
 
+void BackgroundFetchRecord::ResolveResponseReadyProperty() {
+  if (!response_ready_property_ ||
+      response_ready_property_->GetState() !=
+          ScriptPromisePropertyBase::State::kPending) {
+    return;
+  }
+
+  switch (record_state_) {
+    case State::kPending:
+      return;
+    case State::kAborted:
+      response_ready_property_->Reject(DOMException::Create(
+          DOMExceptionCode::kAbortError,
+          "The fetch was aborted before the record was processed."));
+      return;
+    case State::kSettled:
+      if (response_) {
+        response_ready_property_->Resolve(response_);
+        return;
+      }
+
+      if (!script_state_->ContextIsValid())
+        return;
+
+      // TODO(crbug.com/875201):Per https://wicg.github.io/background-fetch/
+      // #background-fetch-response-exposed, this should be resolved with a
+      // TypeError. Figure out a way to do so.
+      // Rejecting this with a TypeError here doesn't work because the
+      // RejectedType is a DOMException. Update this with the correct error
+      // once confirmed, or change the RejectedType.
+      response_ready_property_->Reject(DOMException::Create(
+          DOMExceptionCode::kUnknownError, "The response is not available."));
+  }
+}
+
 ScriptPromise BackgroundFetchRecord::responseReady(ScriptState* script_state) {
   if (!response_ready_property_) {
     response_ready_property_ =
@@ -25,34 +60,47 @@ ScriptPromise BackgroundFetchRecord::responseReady(ScriptState* script_state) {
                                   ResponseReadyProperty::kResponseReady);
   }
 
-  if (response_) {
-    DCHECK(response_);
-    response_ready_property_->Resolve(response_);
-    return response_ready_property_->Promise(script_state->World());
-  }
-
-  if (aborted_) {
-    return ScriptPromise::RejectWithDOMException(
-        script_state,
-        DOMException::Create(
-            DOMExceptionCode::kAbortError,
-            "The fetch was aborted before the record was processed."));
-  }
+  ResolveResponseReadyProperty();
 
-  return ScriptPromise::Reject(
-      script_state,
-      V8ThrowException::CreateTypeError(script_state->GetIsolate(),
-                                        "The response is not available."));
+  return response_ready_property_->Promise(script_state->World());
 }
 
 Request* BackgroundFetchRecord::request() const {
   return request_;
 }
 
+void BackgroundFetchRecord::UpdateState(
+    BackgroundFetchRecord::State updated_state) {
+  DCHECK_EQ(record_state_, State::kPending);
+
+  record_state_ = updated_state;
+  ResolveResponseReadyProperty();
+}
+
+void BackgroundFetchRecord::SetResponse(
+    mojom::blink::FetchAPIResponsePtr& response) {
+  DCHECK(record_state_ == State::kPending);
+
+  if (!response_) {
+    if (!script_state_->ContextIsValid())
+      return;
+    response_ = Response::Create(script_state_, *response);
+  }
+
+  DCHECK(response_);
+
+  ResolveResponseReadyProperty();
+}
+
+bool BackgroundFetchRecord::IsRecordPending() {
+  return record_state_ == State::kPending;
+}
+
 void BackgroundFetchRecord::Trace(blink::Visitor* visitor) {
   visitor->Trace(request_);
   visitor->Trace(response_);
   visitor->Trace(response_ready_property_);
+  visitor->Trace(script_state_);
   ScriptWrappable::Trace(visitor);
 }
 

third_party/blink/renderer/modules/background_fetch/background_fetch_record.h

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_MODULES_BACKGROUND_FETCH_BACKGROUND_FETCH_RECORD_H_
 #define THIRD_PARTY_BLINK_RENDERER_MODULES_BACKGROUND_FETCH_BACKGROUND_FETCH_RECORD_H_
 
+#include "third_party/blink/public/platform/modules/background_fetch/background_fetch.mojom-blink.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_promise.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_promise_property.h"
 #include "third_party/blink/renderer/core/dom/dom_exception.h"
@@ -16,17 +17,35 @@ namespace blink {
 
 class Request;
 class Response;
+class ScriptState;
 
 class MODULES_EXPORT BackgroundFetchRecord final : public ScriptWrappable {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
-  BackgroundFetchRecord(Request* request, Response* response, bool aborted);
+  // The record can be in one of these states. |kSettled| can mean success or
+  // failure based on whether or not |response_| is a nullptr.
+  enum class State {
+    kPending,
+    kAborted,
+    kSettled,
+  };
+
+  BackgroundFetchRecord(Request* request, ScriptState* script_state);
   ~BackgroundFetchRecord() override;
 
   Request* request() const;
   ScriptPromise responseReady(ScriptState* script_state);
 
+  // Updates |record_state_| from kPending to kAborted or kSettled. Must be
+  // called when |record_state_| is kPending.
+  void UpdateState(State updated_state);
+
+  // Sets |response_| to response. Must be called when |record_state_| is
+  // kPending.
+  void SetResponse(mojom::blink::FetchAPIResponsePtr& response);
+
+  bool IsRecordPending();
   void Trace(blink::Visitor* visitor) override;
 
  private:
@@ -34,12 +53,23 @@ class MODULES_EXPORT BackgroundFetchRecord final : public ScriptWrappable {
       ScriptPromiseProperty<Member<BackgroundFetchRecord>,
                             Member<Response>,
                             Member<DOMException>>;
+
+  // Resolves a pending |response_read_property_| with |response_|, if it's not
+  // null.
+  // If |response_| is null, we do nothing if the record isn't final yet. If
+  // |record_final_| is true in this case, we reject the promise.
+  // This is because the record will not be updated with a valid |response_|.
+  void ResolveResponseReadyProperty();
+
   Member<Request> request_;
   Member<Response> response_;
   Member<ResponseReadyProperty> response_ready_property_;
 
-  // Whether this record belongs to a fetch that was aborted.
-  bool aborted_;
+  // Since BackgroundFetchRecord can only be accessed from the world that
+  // created it, there's no danger of ScriptState leaking across worlds.
+  Member<ScriptState> script_state_;
+
+  State record_state_ = State::kPending;
 };
 
 }  // namespace blink

third_party/blink/renderer/modules/background_fetch/background_fetch_registration.cc

@@ -19,6 +19,7 @@
 #include "third_party/blink/renderer/modules/manifest/image_resource.h"
 #include "third_party/blink/renderer/modules/service_worker/service_worker_registration.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
+#include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 
 namespace blink {
 
@@ -87,6 +88,8 @@ void BackgroundFetchRegistration::OnProgress(
   result_ = result;
   failure_reason_ = failure_reason;
 
+  // TODO(crbug.com/875201): Update records in |records_|.
+
   ExecutionContext* context = GetExecutionContext();
   if (!context || context->IsContextDestroyed())
     return;
@@ -232,25 +235,30 @@ void BackgroundFetchRegistration::DidGetMatchingRequests(
     ScriptPromiseResolver* resolver,
     bool return_all,
     Vector<mojom::blink::BackgroundFetchSettledFetchPtr> settled_fetches) {
+  DCHECK(resolver);
+
   ScriptState* script_state = resolver->GetScriptState();
   // Do not remove this, |scope| is needed for calling ToV8()
   ScriptState::Scope scope(script_state);
   HeapVector<Member<BackgroundFetchRecord>> to_return;
   to_return.ReserveInitialCapacity(settled_fetches.size());
-  for (const auto& fetch : settled_fetches) {
-    Request* request = Request::Create(script_state, *(fetch->request));
-
-    Response* response = fetch->response
-                             ? Response::Create(script_state, *fetch->response)
-                             : nullptr;
-
-    bool aborted =
-        failure_reason_ ==
-            mojom::BackgroundFetchFailureReason::CANCELLED_FROM_UI ||
-        failure_reason_ ==
-            mojom::BackgroundFetchFailureReason::CANCELLED_BY_DEVELOPER;
+  for (auto& fetch : settled_fetches) {
+    // If there isn't a record for this fetch in records_ already, create one.
+    auto iter = records_.find(fetch->request->url);
+    BackgroundFetchRecord* record = nullptr;
+    if (iter == records_.end()) {
+      Request* request = Request::Create(script_state, *(fetch->request));
+      auto* new_record = new BackgroundFetchRecord(request, script_state);
+      DCHECK(new_record);
+      records_.Set(request->url(), new_record);
+
+      record = new_record;
+    } else {
+      record = iter->value;
+    }
 
-    to_return.push_back(new BackgroundFetchRecord(request, response, aborted));
+    UpdateRecord(record, fetch->response);
+    to_return.push_back(record);
   }
 
   if (!return_all) {
@@ -267,6 +275,38 @@ void BackgroundFetchRegistration::DidGetMatchingRequests(
   resolver->Resolve(to_return);
 }
 
+void BackgroundFetchRegistration::UpdateRecord(
+    BackgroundFetchRecord* record,
+    mojom::blink::FetchAPIResponsePtr& response) {
+  DCHECK(record);
+
+  if (!record->IsRecordPending())
+    return;
+
+  // Per the spec, resolve with a valid response, if there is one available,
+  // even if the fetch has been aborted.
+  if (response) {
+    record->SetResponse(response);
+    record->UpdateState(BackgroundFetchRecord::State::kSettled);
+    return;
+  }
+
+  if (IsAborted()) {
+    record->UpdateState(BackgroundFetchRecord::State::kAborted);
+    return;
+  }
+
+  if (result_ != mojom::blink::BackgroundFetchResult::UNSET)
+    record->UpdateState(BackgroundFetchRecord::State::kSettled);
+}
+
+bool BackgroundFetchRegistration::IsAborted() {
+  return failure_reason_ ==
+             mojom::BackgroundFetchFailureReason::CANCELLED_FROM_UI ||
+         failure_reason_ ==
+             mojom::BackgroundFetchFailureReason::CANCELLED_BY_DEVELOPER;
+}
+
 void BackgroundFetchRegistration::DidAbort(
     ScriptPromiseResolver* resolver,
     mojom::blink::BackgroundFetchError error) {
@@ -333,6 +373,7 @@ void BackgroundFetchRegistration::Dispose() {
 
 void BackgroundFetchRegistration::Trace(Visitor* visitor) {
   visitor->Trace(registration_);
+  visitor->Trace(records_);
   EventTargetWithInlineData::Trace(visitor);
 }
 

third_party/blink/renderer/modules/background_fetch/background_fetch_registration.h

@@ -12,10 +12,12 @@
 #include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
 #include "third_party/blink/renderer/platform/heap/garbage_collected.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
+#include "third_party/blink/renderer/platform/weborigin/kurl_hash.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
 namespace blink {
 
+class BackgroundFetchRecord;
 class CacheQueryOptions;
 class ScriptPromiseResolver;
 class ScriptState;
@@ -111,8 +113,18 @@ class BackgroundFetchRegistration final
       bool return_all,
       Vector<mojom::blink::BackgroundFetchSettledFetchPtr> settled_fetches);
 
+  // Updates the |record| with a |response|, if one is available, else marks
+  // the |record|'s request as aborted or failed.
+  void UpdateRecord(BackgroundFetchRecord* record,
+                    mojom::blink::FetchAPIResponsePtr& response);
+
+  bool IsAborted();
+
   Member<ServiceWorkerRegistration> registration_;
 
+  // TODO(crbug.com/774054): Update the key once we support duplicate requests.
+  HeapHashMap<KURL, Member<BackgroundFetchRecord>> records_;
+
   // Corresponds to IDL 'id' attribute. Not unique - an active registration can
   // have the same |developer_id_| as one or more inactive registrations.
   String developer_id_;