Skip to content

GitLab

T
tangled
Commit 0bac1c32 authored by Ben Kelly's avatar Ben Kelly Committed by Commit Bot
Browse files
Options

Revert "Cache Storage: Check for duplicate entries in Cache.addAll()." 

This reverts commit 74fd352e.

Reason for revert: Moving behind experimental feature flag until web compat can be assessed.

Original change's description:
> Cache Storage: Check for duplicate entries in Cache.addAll().
> 
> This implements step 4.2.3 of the BatchCacheOperations algorithm:
> 
> https://w3c.github.io/ServiceWorker/#batch-cache-operations-algorithm
> 
> Bug: 720919
> Change-Id: I679f786441b813ed816a183522021c417f4ab7b8
> Reviewed-on: https://chromium-review.googlesource.com/1162362
> Commit-Queue: Ben Kelly <wanderview@chromium.org>
> Reviewed-by: Joshua Bell <jsbell@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Victor Costan <pwnall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#582738}

TBR=jsbell@chromium.org,kinuko@chromium.org,pwnall@chromium.org,wanderview@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 720919
Change-Id: I08736e4ec638a65c48f329966f378698a6d9b045
Reviewed-on: https://chromium-review.googlesource.com/1178502Reviewed-by: default avatarVictor Costan <pwnall@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583849}
parent 685952aa
Hide whitespace changes
Inline Side-by-side
Showing with 72 additions and 198 deletions
content/browser/cache_storage/cache_storage_cache.cc
View file @ 0bac1c32
......@@ -168,63 +168,6 @@ bool VaryMatches(const ServiceWorkerHeaderMap& request,
return true;
}
// Check batch operation list for duplicate entries.
bool HasDuplicateOperations(
const std::vector<blink::mojom::BatchOperationPtr>& operations) {
using blink::mojom::BatchOperation;
if (operations.size() < 2) {
return false;
}
// Create a temporary sorted vector of the operations to support quickly
// finding potentially duplicate entries. Multiple entries may have the
// same URL, but differ by VARY header, so a sorted list is easier to
// work with than a map.
//
// Note, this will use 512 bytes of stack space on 64-bit devices. The
// static size attempts to accommodate most typical Cache.addAll() uses in
// service worker install events while not blowing up the stack too much.
base::StackVector<BatchOperation*, 64> sorted;
sorted->reserve(operations.size());
for (const auto& op : operations) {
sorted->push_back(op.get());
}
std::sort(sorted->begin(), sorted->end(),
[](BatchOperation* left, BatchOperation* right) {
return left->request.url < right->request.url;
});
// Check each entry in the sorted vector for any duplicates. Since the
// list is sorted we only need to inspect the immediate neighbors that
// have the same URL. This results in an average complexity of O(n log n).
// If the entire list has entries with the same URL and different VARY
// headers then this devolves into O(n^2).
for (auto outer = sorted->cbegin(); outer != sorted->cend(); ++outer) {
const BatchOperation* outer_op = *outer;
for (auto inner = std::next(outer); inner != sorted->cend(); ++inner) {
const BatchOperation* inner_op = *inner;
// Since the list is sorted we can stop looking at neighbors after
// the first different URL.
if (outer_op->request.url != inner_op->request.url) {
break;
}
// VaryMatches() is asymmetric since the operation depends on the VARY
// header in the target response. Since we only visit each pair of
// entries once we need to perform the VaryMatches() call in both
// directions.
if (VaryMatches(outer_op->request.headers, inner_op->request.headers,
inner_op->response->headers) ||
VaryMatches(outer_op->request.headers, inner_op->request.headers,
outer_op->response->headers)) {
return true;
}
}
}
return false;
}
GURL RemoveQueryParam(const GURL& url) {
url::Replacements<char> replacements;
replacements.ClearQuery();
......@@ -601,26 +544,6 @@ void CacheStorageCache::BatchOperation(
return;
}
// From BatchCacheOperations:
//
// https://w3c.github.io/ServiceWorker/#batch-cache-operations-algorithm
//
// "If the result of running Query Cache with operation’s request,
// operation’s options, and addedItems is not empty, throw an
// InvalidStateError DOMException."
//
// Note, the spec checks CacheQueryOptions like ignoreSearch, etc, but
// currently there is no way for script to trigger a batch operation with
// those set. The only exposed API is addAll() which does not allow
// options to be passed. Therefore, assume default options below. This
// is enforced in the mojo interface by not passing any options arguments.
if (HasDuplicateOperations(operations)) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback),
CacheStorageError::kErrorDuplicateOperation));
return;
}
// Estimate the required size of the put operations. The size of the deletes
// is unknown and not considered.
base::CheckedNumeric<uint64_t> safe_space_required = 0;
......
content/browser/cache_storage/cache_storage_cache_unittest.cc
View file @ 0bac1c32
......@@ -910,17 +910,15 @@ TEST_P(CacheStorageCacheTestP, PutBodyDropBlobRef) {
}
TEST_P(CacheStorageCacheTestP, PutBadMessage) {
// Two unique puts that will collectively overflow unit64_t size of the
// batch operation.
blink::mojom::BatchOperationPtr operation1 =
blink::mojom::BatchOperation::New(blink::mojom::OperationType::kPut,
body_request_, CreateBlobBodyResponse(),
nullptr /* match_params */);
operation1->response->blob->size = std::numeric_limits<uint64_t>::max();
blink::mojom::BatchOperationPtr operation2 =
blink::mojom::BatchOperation::New(
blink::mojom::OperationType::kPut, body_request_with_query_,
CreateBlobBodyResponse(), nullptr /* match_params */);
blink::mojom::BatchOperation::New(blink::mojom::OperationType::kPut,
body_request_, CreateBlobBodyResponse(),
nullptr /* match_params */);
operation2->response->blob->size = std::numeric_limits<uint64_t>::max();
std::vector<blink::mojom::BatchOperationPtr> operations;
......@@ -947,7 +945,7 @@ TEST_P(CacheStorageCacheTestP, PutReplace) {
EXPECT_FALSE(callback_response_->blob);
}
TEST_P(CacheStorageCacheTestP, PutReplaceInBatchFails) {
TEST_P(CacheStorageCacheTestP, PutReplaceInBatch) {
blink::mojom::BatchOperationPtr operation1 =
blink::mojom::BatchOperation::New();
operation1->operation_type = blink::mojom::OperationType::kPut;
......@@ -964,11 +962,11 @@ TEST_P(CacheStorageCacheTestP, PutReplaceInBatchFails) {
operations.push_back(std::move(operation1));
operations.push_back(std::move(operation2));
EXPECT_EQ(CacheStorageError::kErrorDuplicateOperation,
BatchOperation(std::move(operations)));
EXPECT_EQ(CacheStorageError::kSuccess, BatchOperation(std::move(operations)));
// Neither operation should have completed.
EXPECT_FALSE(Match(body_request_));
// |operation2| should win.
EXPECT_TRUE(Match(body_request_));
EXPECT_TRUE(callback_response_->blob);
}
TEST_P(CacheStorageCacheTestP, MatchNoBody) {
......
third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/resources/vary.py deleted 100644 → 0
View file @ 685952aa
def main(request, response):
if "clear-vary-value-override-cookie" in request.GET:
response.unset_cookie("vary-value-override")
return "vary cookie cleared"
set_cookie_vary = request.GET.first("set-vary-value-override-cookie",
default="")
if set_cookie_vary:
response.set_cookie("vary-value-override", set_cookie_vary)
return "vary cookie set"
# If there is a vary-value-override cookie set, then use its value
# for the VARY header no matter what the query string is set to. This
# override is necessary to test the case when two URLs are identical
# (including query), but differ by VARY header.
cookie_vary = request.cookies.get("vary-value-override");
if cookie_vary:
response.headers.set("vary", cookie_vary)
else:
# If there is no cookie, then use the query string value, if present.
query_vary = request.GET.first("vary", default="")
if query_vary:
response.headers.set("vary", query_vary)
return "vary response"
third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/script-tests/cache-add.js
View file @ 0bac1c32
......@@ -267,84 +267,4 @@ cache_test(function(cache, test) {
'twice.');
}, 'Cache.addAll called with the same Request object specified twice');
cache_test(async function(cache, test) {
const url = '../resources/vary.py?vary=x-shape';
let requests = [
new Request(url, { headers: { 'x-shape': 'circle' }}),
new Request(url, { headers: { 'x-shape': 'square' }}),
];
let result = await cache.addAll(requests);
assert_equals(result, undefined, 'Cache.addAll() should succeed');
}, 'Cache.addAll should succeed when entries differ by vary header');
cache_test(async function(cache, test) {
const url = '../resources/vary.py?vary=x-shape';
let requests = [
new Request(url, { headers: { 'x-shape': 'circle' }}),
new Request(url, { headers: { 'x-shape': 'circle' }}),
];
await promise_rejects(
test,
'InvalidStateError',
cache.addAll(requests),
'Cache.addAll() should reject when entries are duplicate by vary header');
}, 'Cache.addAll should reject when entries are duplicate by vary header');
// VARY header matching is asymmetric. Determining if two entries are duplicate
// depends on which entry's response is used in the comparison. The target
// response's VARY header determines what request headers are examined. This
// test verifies that Cache.addAll() duplicate checking handles this asymmetric
// behavior correctly.
cache_test(async function(cache, test) {
const base_url = '../resources/vary.py';
// Define a request URL that sets a VARY header in the
// query string to be echoed back by the server.
const url = base_url + '?vary=x-size';
// Set a cookie to override the VARY header of the response
// when the request is made with credentials. This will
// take precedence over the query string vary param. This
// is a bit confusing, but it's necessary to construct a test
// where the URL is the same, but the VARY headers differ.
//
// Note, the test could also pass this information in additional
// request headers. If the cookie approach becomes too unwieldy
// this test could be rewritten to use that technique.
await fetch(base_url + '?set-vary-value-override-cookie=x-shape');
test.add_cleanup(_ => fetch(base_url + '?clear-vary-value-override-cookie'));
let requests = [
// This request will result in a Response with a "Vary: x-shape"
// header. This *will not* result in a duplicate match with the
// other entry.
new Request(url, { headers: { 'x-shape': 'circle',
'x-size': 'big' },
credentials: 'same-origin' }),
// This request will result in a Response with a "Vary: x-size"
// header. This *will* result in a duplicate match with the other
// entry.
new Request(url, { headers: { 'x-shape': 'square',
'x-size': 'big' },
credentials: 'omit' }),
];
await promise_rejects(
test,
'InvalidStateError',
cache.addAll(requests),
'Cache.addAll() should reject when one entry has a vary header ' +
'matching an earlier entry.');
// Test the reverse order now.
await promise_rejects(
test,
'InvalidStateError',
cache.addAll(requests.reverse()),
'Cache.addAll() should reject when one entry has a vary header ' +
'matching a later entry.');
}, 'Cache.addAll should reject when one entry has a vary header ' +
'matching another entry');
done();
third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/serviceworker/cache-add.https-expected.txt 0 → 100644
View file @ 0bac1c32
This is a testharness.js-based test.
PASS Cache.add and Cache.addAll
PASS Cache.add called with no arguments
PASS Cache.add called with relative URL specified as a string
PASS Cache.add called with non-HTTP/HTTPS URL
PASS Cache.add called with Request object
PASS Cache.add called with POST request
PASS Cache.add called twice with the same Request object
PASS Cache.add with request with null body (not consumed)
PASS Cache.add with 206 response
PASS Cache.addAll with 206 response
PASS Cache.add with request that results in a status of 404
PASS Cache.add with request that results in a status of 500
PASS Cache.addAll with no arguments
PASS Cache.addAll with a mix of valid and undefined arguments
PASS Cache.addAll with an empty array
PASS Cache.addAll with string URL arguments
PASS Cache.addAll with Request arguments
PASS Cache.addAll with a mix of succeeding and failing requests
FAIL Cache.addAll called with the same Request object specified twice assert_unreached: Should have rejected: Cache.addAll should throw InvalidStateError if the same request is added twice. Reached unreachable code
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/window/cache-add.https-expected.txt 0 → 100644
View file @ 0bac1c32
This is a testharness.js-based test.
PASS Cache.add called with no arguments
PASS Cache.add called with relative URL specified as a string
PASS Cache.add called with non-HTTP/HTTPS URL
PASS Cache.add called with Request object
PASS Cache.add called with POST request
PASS Cache.add called twice with the same Request object
PASS Cache.add with request with null body (not consumed)
PASS Cache.add with 206 response
PASS Cache.addAll with 206 response
PASS Cache.add with request that results in a status of 404
PASS Cache.add with request that results in a status of 500
PASS Cache.addAll with no arguments
PASS Cache.addAll with a mix of valid and undefined arguments
PASS Cache.addAll with an empty array
PASS Cache.addAll with string URL arguments
PASS Cache.addAll with Request arguments
PASS Cache.addAll with a mix of succeeding and failing requests
FAIL Cache.addAll called with the same Request object specified twice assert_unreached: Should have rejected: Cache.addAll should throw InvalidStateError if the same request is added twice. Reached unreachable code
Harness: the test ran to completion.
third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/worker/cache-add.https-expected.txt 0 → 100644
View file @ 0bac1c32
This is a testharness.js-based test.
PASS Cache.add called with no arguments
PASS Cache.add called with relative URL specified as a string
PASS Cache.add called with non-HTTP/HTTPS URL
PASS Cache.add called with Request object
PASS Cache.add called with POST request
PASS Cache.add called twice with the same Request object
PASS Cache.add with request with null body (not consumed)
PASS Cache.add with 206 response
PASS Cache.addAll with 206 response
PASS Cache.add with request that results in a status of 404
PASS Cache.add with request that results in a status of 500
PASS Cache.addAll with no arguments
PASS Cache.addAll with a mix of valid and undefined arguments
PASS Cache.addAll with an empty array
PASS Cache.addAll with string URL arguments
PASS Cache.addAll with Request arguments
PASS Cache.addAll with a mix of succeeding and failing requests
FAIL Cache.addAll called with the same Request object specified twice assert_unreached: Should have rejected: Cache.addAll should throw InvalidStateError if the same request is added twice. Reached unreachable code
Harness: the test ran to completion.
third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom
View file @ 0bac1c32
......@@ -21,7 +21,6 @@ enum CacheStorageError {
kErrorQueryTooLarge = 6,
// TODO(cmumford): kErrorNotImplemented is deprecated. Remove use in code.
kErrorNotImplemented = 7,
kErrorDuplicateOperation = 8,
// Add new values here.
};
......
third_party/blink/renderer/modules/cache_storage/cache_storage_error.cc
View file @ 0bac1c32
......@@ -34,9 +34,6 @@ DOMException* CacheStorageError::CreateException(
case mojom::CacheStorageError::kErrorStorage:
return DOMException::Create(DOMExceptionCode::kUnknownError,
"Unexpected internal error.");
case mojom::CacheStorageError::kErrorDuplicateOperation:
return DOMException::Create(DOMExceptionCode::kInvalidStateError,
"Duplicate operation.");
case mojom::CacheStorageError::kSuccess:
// This function should only be called with an error.
break;
......
third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc
View file @ 0bac1c32
......@@ -161,8 +161,6 @@ CString CacheStorageErrorString(mojom::blink::CacheStorageError error) {
return CString("operation too large.");
case mojom::blink::CacheStorageError::kErrorStorage:
return CString("storage failure.");
case mojom::blink::CacheStorageError::kErrorDuplicateOperation:
return CString("duplicate operation.");
case mojom::blink::CacheStorageError::kSuccess:
// This function should only be called upon error.
break;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment