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}

content/browser/cache_storage/cache_storage_cache.cc

@@ -168,6 +168,63 @@ 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();
@@ -544,6 +601,26 @@ 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

@@ -910,15 +910,17 @@ 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_, CreateBlobBodyResponse(),
-                                        nullptr /* match_params */);
+      blink::mojom::BatchOperation::New(
+          blink::mojom::OperationType::kPut, body_request_with_query_,
+          CreateBlobBodyResponse(), nullptr /* match_params */);
   operation2->response->blob->size = std::numeric_limits<uint64_t>::max();
 
   std::vector<blink::mojom::BatchOperationPtr> operations;
@@ -945,7 +947,7 @@ TEST_P(CacheStorageCacheTestP, PutReplace) {
   EXPECT_FALSE(callback_response_->blob);
 }
 
-TEST_P(CacheStorageCacheTestP, PutReplaceInBatch) {
+TEST_P(CacheStorageCacheTestP, PutReplaceInBatchFails) {
   blink::mojom::BatchOperationPtr operation1 =
       blink::mojom::BatchOperation::New();
   operation1->operation_type = blink::mojom::OperationType::kPut;
@@ -962,11 +964,11 @@ TEST_P(CacheStorageCacheTestP, PutReplaceInBatch) {
   operations.push_back(std::move(operation1));
   operations.push_back(std::move(operation2));
 
-  EXPECT_EQ(CacheStorageError::kSuccess, BatchOperation(std::move(operations)));
+  EXPECT_EQ(CacheStorageError::kErrorDuplicateOperation,
+            BatchOperation(std::move(operations)));
 
-  // |operation2| should win.
-  EXPECT_TRUE(Match(body_request_));
-  EXPECT_TRUE(callback_response_->blob);
+  // Neither operation should have completed.
+  EXPECT_FALSE(Match(body_request_));
 }
 
 TEST_P(CacheStorageCacheTestP, MatchNoBody) {

third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/resources/vary.py

+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

@@ -267,4 +267,84 @@ 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

-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

-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

-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

@@ -21,6 +21,7 @@ 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

@@ -34,6 +34,9 @@ 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

@@ -161,6 +161,8 @@ 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;