[Background Fetch] Support registrations w/duplicate URLs.
This CL uses a workaraound to enable storing duplicate URLs in the Cache
Storage, until it is fully supported.
The way it works is that it appends the unique_id+request_index to the
URL query, and removes it again when extracted from the cache.
When a request is matched, we search the cache with ignored query mode,
then we recheck that the queries in fact match. This also allows to
store duplicate URLs with different methods.
This CL should be submitted with a second part, that performs a cache
migration from the old format to the new one.
Bug: 871174
Change-Id: Id801361424fbb9464016daba153155c3259b6671
Reviewed-on: https://chromium-review.googlesource.com/c/1414937
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Mugdha Lakhani <nator@google.com>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625239}diff --git a/content/browser/background_fetch/background_fetch_data_manager_unittest.cc b/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
index b011a2ff..07defde3 100644
--- a/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
+++ b/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
@@ -454,9 +454,11 @@
CacheStorageHandle cache_storage =
background_fetch_data_manager_->cache_manager()->OpenCacheStorage(
origin(), CacheStorageOwner::kBackgroundFetch);
+ auto match_params = blink::mojom::QueryParams::New();
+ match_params->ignore_search = true;
cache_storage.value()->MatchCache(
kExampleUniqueId, BackgroundFetchSettledFetch::CloneRequest(request),
- /* match_params= */ nullptr,
+ std::move(match_params),
base::BindOnce(&BackgroundFetchDataManagerTest::DidMatchCache,
base::Unretained(this), run_loop.QuitClosure(),
&result));
@@ -490,7 +492,8 @@
blink::mojom::OperationType::kDelete;
operation_ptr_vec[0]->request =
BackgroundFetchSettledFetch::CloneRequest(request);
-
+ operation_ptr_vec[0]->match_params = blink::mojom::QueryParams::New();
+ operation_ptr_vec[0]->match_params->ignore_search = true;
handle.value()->BatchOperation(
std::move(operation_ptr_vec), /* fail_on_duplicates= */ true,
base::BindOnce(&BackgroundFetchDataManagerTest::DidDeleteFromCache,
@@ -1941,11 +1944,91 @@
&settled_fetches);
ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
- // If the ASSERT below fails, the Cache Storage API implementation has likely
- // changed to distinguish keys by request data other than just the URL.
- // Thank you! Please can you update the 1u below to 2u, or file a bug against
- // component Background Fetch to do so.
- ASSERT_EQ(settled_fetches.size(), 1u);
+ ASSERT_EQ(settled_fetches.size(), 2u);
+}
+
+TEST_F(BackgroundFetchDataManagerTest, MatchRequestsWithDuplicates) {
+ int64_t sw_id = RegisterServiceWorker();
+ ASSERT_NE(blink::mojom::kInvalidServiceWorkerRegistrationId, sw_id);
+
+ std::vector<blink::mojom::FetchAPIRequestPtr> requests;
+ const std::string base_path = "https://example.com/foo";
+
+ // Add base request.
+ {
+ auto request = blink::mojom::FetchAPIRequest::New();
+ request->url = GURL(base_path);
+ request->method = "GET";
+ request->referrer = blink::mojom::Referrer::New();
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ }
+
+ // Add base request with a query.
+ {
+ auto request = blink::mojom::FetchAPIRequest::New();
+ request->method = "GET";
+ request->url = GURL(base_path + "?a=b");
+ request->referrer = blink::mojom::Referrer::New();
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ request->url = GURL(base_path + "?c=d");
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ }
+
+ // Add base request with different method.
+ {
+ auto request = blink::mojom::FetchAPIRequest::New();
+ request->url = GURL(base_path);
+ request->method = "POST";
+ request->referrer = blink::mojom::Referrer::New();
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ requests.push_back(BackgroundFetchSettledFetch::CloneRequest(request));
+ }
+
+ blink::mojom::BackgroundFetchError error;
+ BackgroundFetchRegistrationId registration_id(
+ sw_id, origin(), kExampleDeveloperId, kExampleUniqueId);
+
+ {
+ auto options = blink::mojom::BackgroundFetchOptions::New();
+ EXPECT_CALL(*this, OnRegistrationCreated(registration_id, _, _, _, _, _));
+
+ CreateRegistration(registration_id, CloneRequestVector(requests),
+ std::move(options), SkBitmap(), &error);
+ ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+ }
+
+ std::vector<blink::mojom::BackgroundFetchSettledFetchPtr> settled_fetches;
+ auto request_to_match =
+ BackgroundFetchSettledFetch::CloneRequest(requests[0]);
+ auto query_params = blink::mojom::QueryParams::New();
+
+ MatchRequests(registration_id,
+ BackgroundFetchSettledFetch::CloneRequest(request_to_match),
+ query_params->Clone(), /* match_all= */ true, &error,
+ &settled_fetches);
+ EXPECT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+ // Match only the GETs with the same url.
+ EXPECT_EQ(settled_fetches.size(), 2u);
+
+ query_params->ignore_search = true;
+ MatchRequests(registration_id,
+ BackgroundFetchSettledFetch::CloneRequest(request_to_match),
+ query_params->Clone(), /* match_all= */ true, &error,
+ &settled_fetches);
+ EXPECT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+ // Match only the GETs with the same url path.
+ EXPECT_EQ(settled_fetches.size(), 5u);
+
+ query_params->ignore_method = true;
+ MatchRequests(registration_id,
+ BackgroundFetchSettledFetch::CloneRequest(request_to_match),
+ query_params->Clone(), /* match_all= */ true, &error,
+ &settled_fetches);
+ EXPECT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+ // Match everything.
+ EXPECT_EQ(settled_fetches.size(), requests.size());
}
TEST_F(BackgroundFetchDataManagerTest, Cleanup) {
diff --git a/content/browser/background_fetch/background_fetch_request_match_params.h b/content/browser/background_fetch/background_fetch_request_match_params.h
index 3eab4913..8dab5fe 100644
--- a/content/browser/background_fetch/background_fetch_request_match_params.h
+++ b/content/browser/background_fetch/background_fetch_request_match_params.h
@@ -28,6 +28,10 @@
return request_to_match_;
}
+ const blink::mojom::QueryParamsPtr& cache_query_params() const {
+ return cache_query_params_;
+ }
+
blink::mojom::QueryParamsPtr cloned_cache_query_params() const {
if (!cache_query_params_)
return nullptr;
diff --git a/content/browser/background_fetch/background_fetch_service_unittest.cc b/content/browser/background_fetch/background_fetch_service_unittest.cc
index 3c2204e..d93e1e1 100644
--- a/content/browser/background_fetch/background_fetch_service_unittest.cc
+++ b/content/browser/background_fetch/background_fetch_service_unittest.cc
@@ -200,8 +200,8 @@
base::RunLoop run_loop;
service_->MatchRequests(
service_worker_registration_id, developer_id, unique_id,
- blink::mojom::FetchAPIRequest::New() /* request_to_match*/,
- nullptr /* cache_query_params*/, /* match_all= */ true,
+ /* request_to_match= */ nullptr,
+ /* cache_query_params= */ nullptr, /* match_all= */ true,
base::BindOnce(&BackgroundFetchServiceTest::DidMatchAllRequests,
base::Unretained(this), run_loop.QuitClosure(),
out_fetches));
diff --git a/content/browser/background_fetch/storage/create_metadata_task.cc b/content/browser/background_fetch/storage/create_metadata_task.cc
index d28d6ca..79e36b30 100644
--- a/content/browser/background_fetch/storage/create_metadata_task.cc
+++ b/content/browser/background_fetch/storage/create_metadata_task.cc
@@ -382,10 +382,12 @@
// Create batch PUT operations instead of putting them one-by-one.
std::vector<blink::mojom::BatchOperationPtr> operations;
operations.reserve(requests_.size());
- for (auto& request : requests_) {
+ for (size_t i = 0; i < requests_.size(); i++) {
auto operation = blink::mojom::BatchOperation::New();
operation->operation_type = blink::mojom::OperationType::kPut;
- operation->request = std::move(request);
+ requests_[i]->url =
+ MakeCacheUrlUnique(requests_[i]->url, registration_id_.unique_id(), i);
+ operation->request = std::move(requests_[i]);
// Empty response.
operation->response = blink::mojom::FetchAPIResponse::New();
operations.push_back(std::move(operation));
diff --git a/content/browser/background_fetch/storage/database_helpers.cc b/content/browser/background_fetch/storage/database_helpers.cc
index 41ed22f4..2071d04 100644
--- a/content/browser/background_fetch/storage/database_helpers.cc
+++ b/content/browser/background_fetch/storage/database_helpers.cc
@@ -165,6 +165,34 @@
return false;
}
+GURL MakeCacheUrlUnique(const GURL& url,
+ const std::string& unique_id,
+ int request_index) {
+ std::string query = url.query();
+ query += unique_id + base::NumberToString(request_index);
+
+ GURL::Replacements replacements;
+ replacements.SetQueryStr(query);
+
+ return url.ReplaceComponents(replacements);
+}
+
+GURL RemoveUniqueParamFromCacheURL(const GURL& url,
+ const std::string& unique_id) {
+ std::vector<std::string> split = base::SplitStringUsingSubstr(
+ url.query(), unique_id, base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+ GURL::Replacements replacements;
+ if (split.size() == 1u)
+ replacements.ClearQuery();
+ else if (split.size() == 2u)
+ replacements.SetQueryStr(split[0]);
+ else
+ NOTREACHED();
+
+ return url.ReplaceComponents(replacements);
+}
+
} // namespace background_fetch
} // namespace content
diff --git a/content/browser/background_fetch/storage/database_helpers.h b/content/browser/background_fetch/storage/database_helpers.h
index 4a43dc7..61e76cb 100644
--- a/content/browser/background_fetch/storage/database_helpers.h
+++ b/content/browser/background_fetch/storage/database_helpers.h
@@ -72,6 +72,16 @@
proto_failure_reason,
blink::mojom::BackgroundFetchFailureReason* failure_reason);
+// Utility functions to make sure the request URLs are unique, since
+// Cache Storage does not support duplicate URLs yet.
+// Use `MakeCacheUrlUnique` before writing to the cache, and
+// `RemoveUniqueParamFromCacheURL` when querying from the cache.
+CONTENT_EXPORT GURL MakeCacheUrlUnique(const GURL& url,
+ const std::string& unique_id,
+ int request_index);
+CONTENT_EXPORT GURL RemoveUniqueParamFromCacheURL(const GURL& url,
+ const std::string& unique_id);
+
} // namespace background_fetch
} // namespace content
diff --git a/content/browser/background_fetch/storage/database_helpers_unittest.cc b/content/browser/background_fetch/storage/database_helpers_unittest.cc
new file mode 100644
index 0000000..44346895
--- /dev/null
+++ b/content/browser/background_fetch/storage/database_helpers_unittest.cc
@@ -0,0 +1,39 @@
+// Copyright 2019 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.
+
+#include "content/browser/background_fetch/storage/database_helpers.h"
+
+#include <string>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace content {
+namespace background_fetch {
+namespace {
+
+constexpr char kExampleUniqueId[] = "7e57ab1e-c0de-a150-ca75-1e75f005ba11";
+
+bool CacheUrlRoundTrip(const std::string& url) {
+ GURL gurl(url);
+ GURL round_trip_url = RemoveUniqueParamFromCacheURL(
+ MakeCacheUrlUnique(gurl, kExampleUniqueId, 0), kExampleUniqueId);
+ return round_trip_url == gurl;
+}
+
+TEST(BackgroundFetchDatabaseHelpers, CacheUrlRoundTrip) {
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com/"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com?a=b"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com?a=b&c=d"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com/path"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com/path/"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com/path1/path2"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com/path?a=b&c=d"));
+ EXPECT_TRUE(CacheUrlRoundTrip("https://example.com/path/?a=b&c=d"));
+}
+
+} // namespace
+} // namespace background_fetch
+} // namespace content
diff --git a/content/browser/background_fetch/storage/mark_request_complete_task.cc b/content/browser/background_fetch/storage/mark_request_complete_task.cc
index 899e8f8..ac5dcc5 100644
--- a/content/browser/background_fetch/storage/mark_request_complete_task.cc
+++ b/content/browser/background_fetch/storage/mark_request_complete_task.cc
@@ -187,6 +187,9 @@
BackgroundFetchSettledFetch::CloneRequest(
request_info_->fetch_request_ptr());
+ request->url = MakeCacheUrlUnique(request->url, registration_id_.unique_id(),
+ request_info_->request_index());
+
// TODO(crbug.com/774054): The request blob stored in the cache is being
// overwritten here, it should be written back.
handle.value()->Put(
diff --git a/content/browser/background_fetch/storage/match_requests_task.cc b/content/browser/background_fetch/storage/match_requests_task.cc
index c82ae86e..7600f4c 100644
--- a/content/browser/background_fetch/storage/match_requests_task.cc
+++ b/content/browser/background_fetch/storage/match_requests_task.cc
@@ -56,8 +56,20 @@
request = blink::mojom::FetchAPIRequest::New();
}
+ auto query_params = match_params_->cloned_cache_query_params();
+ if (!query_params)
+ query_params = blink::mojom::QueryParams::New();
+
+ // Ignore the search params since we added query params to make the URL
+ // unique.
+ query_params->ignore_search = true;
+
+ // Ignore the method since Cache Storage assumes the request being matched
+ // against is a GET.
+ query_params->ignore_method = true;
+
handle_.value()->GetAllMatchedEntries(
- std::move(request), match_params_->cloned_cache_query_params(),
+ std::move(request), std::move(query_params),
base::BindOnce(&MatchRequestsTask::DidGetAllMatchedEntries,
weak_factory_.GetWeakPtr()));
}
@@ -79,13 +91,14 @@
return;
}
- size_t size = match_params_->match_all() ? entries.size() : 1u;
- settled_fetches_.reserve(size);
-
- for (size_t i = 0; i < size; i++) {
- auto& entry = entries[i];
+ for (auto& entry : entries) {
auto settled_fetch = blink::mojom::BackgroundFetchSettledFetch::New();
settled_fetch->request = std::move(entry.first);
+ settled_fetch->request->url = RemoveUniqueParamFromCacheURL(
+ settled_fetch->request->url, registration_id_.unique_id());
+
+ if (!ShouldMatchRequest(settled_fetch->request))
+ continue;
if (entry.second && entry.second->url_list.empty()) {
// We didn't process this empty response, so we should expose it
@@ -96,11 +109,36 @@
}
settled_fetches_.push_back(std::move(settled_fetch));
+ if (!match_params_->match_all())
+ break;
}
FinishWithError(blink::mojom::BackgroundFetchError::NONE);
}
+bool MatchRequestsTask::ShouldMatchRequest(
+ const blink::mojom::FetchAPIRequestPtr& request) {
+ // We were supposed to match everything anyway.
+ if (!match_params_->FilterByRequest())
+ return true;
+
+ // Ignore the request if the methods don't match.
+ if ((!match_params_->cache_query_params() ||
+ !match_params_->cache_query_params()->ignore_method) &&
+ request->method != match_params_->request_to_match()->method) {
+ return false;
+ }
+
+ // Ignore the request if the queries don't match.
+ if ((!match_params_->cache_query_params() ||
+ !match_params_->cache_query_params()->ignore_search) &&
+ request->url.query() != match_params_->request_to_match()->url.query()) {
+ return false;
+ }
+
+ return true;
+}
+
void MatchRequestsTask::FinishWithError(
blink::mojom::BackgroundFetchError error) {
if (HasStorageError())
diff --git a/content/browser/background_fetch/storage/match_requests_task.h b/content/browser/background_fetch/storage/match_requests_task.h
index 2e2e686..b1059fa 100644
--- a/content/browser/background_fetch/storage/match_requests_task.h
+++ b/content/browser/background_fetch/storage/match_requests_task.h
@@ -47,6 +47,9 @@
blink::mojom::CacheStorageError error,
std::vector<CacheStorageCache::CacheEntry> entries);
+ // Checks whether |request| shuld be matched given the provided query params.
+ bool ShouldMatchRequest(const blink::mojom::FetchAPIRequestPtr& request);
+
void FinishWithError(blink::mojom::BackgroundFetchError error) override;
std::string HistogramName() const override;
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn
index df55581..73806c80 100644
--- a/content/test/BUILD.gn
+++ b/content/test/BUILD.gn
@@ -1355,6 +1355,7 @@
"../browser/background_fetch/background_fetch_registration_notifier_unittest.cc",
"../browser/background_fetch/background_fetch_scheduler_unittest.cc",
"../browser/background_fetch/background_fetch_service_unittest.cc",
+ "../browser/background_fetch/storage/database_helpers_unittest.cc",
"../browser/background_fetch/storage/image_helpers_unittest.cc",
"../browser/background_sync/background_sync_manager_unittest.cc",
"../browser/background_sync/background_sync_network_observer_unittest.cc",
