New issue
Advanced search Search tips

Issue 720919 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 426928

Blocking:
issue 655479
issue 877737



Sign in to add a comment

cache.addAll() should reject if requests would overwrite each other

Project Member Reported by jsb...@chromium.org, May 10 2017

Issue description

window, worker, and serviceworker variants all fail with:

*/cache-add.https-expected.txt: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

Determine if this is a legitimate bug or if the test is broken (i.e. compare spec and test, and see what Firefox does here)

 

Comment 1 by jsb...@chromium.org, May 10 2017

Blocking: 655479

Comment 2 by jsb...@chromium.org, May 10 2017

Blockedon: 426928
Spec has:

https://w3c.github.io/ServiceWorker/#batch-cache-operations

"If the result of running Query Cache algorithm passing operation.request, operation.options, and addedRecords as the arguments is not an empty array, throw an "InvalidStateError" exception."

So I believe this is blocked on  issue 426928  "Implement the CacheBatchOperation"

Labels: -Pri-3 Hotlist-Interop Pri-2
Per contacts on Edge, it looks like some sites may be starting to rely on the buggy behavior (i.e. passing the same request twice), so we should try and and fix soon before we end up having to alter the spec.

We may be able to do a scoped fix w/o doing all of the CacheBatchOperation work, i.e. build a temporary hashset at the point of the call.

Comment 4 by jsb...@chromium.org, Jan 19 2018

Labels: Hotlist-GoodFirstBug

Comment 5 by jsb...@chromium.org, Feb 21 2018

Hrm, was poking at this. The simplest implementation would be to jam code into Cache::FetchResolvedForAdd::Call() which processes the requests/responses. 

The "Query Cache" algorithm does require respecting the Vary header for matches, so this isn't as simple as building a HashSet of URLs. One plausible approach:

* Start off with an empty HashMap<KURL, Vector<int>> where the int represents the index in the (requests_, responses) lists.
* For each request/response pair:
  * Grab the request KURL, discarding the fragment.
  * If it's not in the map, add the KURL with a new vector of length 1 with the index, continue
  * If it is in the map, iterate the vector, and do a Vary-aware compare of the request with each indexed response. If a hit, throw
  * Otherwise, append the index to the vector


Comment 6 by jsb...@chromium.org, Feb 21 2018

Status: Started (was: Available)

Comment 7 by jsb...@chromium.org, Feb 21 2018

correction for #c5 - the compare is between *request* pairs with the indexed response providing the vary parameters guiding the comparison.

I have an impl that works/passes the tests, but it needs unit tests.

Comment 8 by jsb...@chromium.org, Feb 21 2018

Also, we've been failing this for long enough we should probably do an I2S for it. :P
Owner: wanderview@chromium.org
Josh's WIP from #c7 seems to be here:

https://chromium-review.googlesource.com/c/chromium/src/+/927833

Not sure what unit tests are needed, though.
After talking it over I'm going to work on implementing this check on the browser process side.  That way we can reuse the existing VaryMatches() method and get shared testing, etc.  It also has the benefit of not allowing the renderer to lie to us about this validity check.
New CL that implements the check on the browser side:

https://chromium-review.googlesource.com/c/chromium/src/+/1162362

Still need to do some minor clean-up.  Ran out of time today.
I checked the other browsers.  Currently FF and safari match the spec.  Edge currently matches chrome by allowing duplicate requests in an addAll().

I'll file a bug against edge.

I'll also create a chromestatus entry and send an I2I/S.  Since two other browsers have shipped this (many years ago for FF), I don't plan to collect any usage data right now.
Summary: cache.addAll() should reject if requests would overwrite each other (was: Failures in WPT's service-workers/cache-storage/*/cache-add.https.html)
Cc: pwnall@chromium.org
Note, after speaking with Rick Byers we decided this is more of a bug fix and does not need an intent email to blink-dev.

I did talk with the Edge folks and they are interested in aligning to the spec as well.  They had a few sites with compat issues in the past.  I checked them out again and could not see a current issue.  Either their service worker was changed, completely gone, or was clearly a test script with no production capability.
For the record: agreeing with consensus that this can be treated as a bug fix. Thanks for investigating/discussing!
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/74fd352e51322e823070088c7d5c72e911966580

commit 74fd352e51322e823070088c7d5c72e911966580
Author: Ben Kelly <wanderview@chromium.org>
Date: Mon Aug 13 23:14:36 2018

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}
[modify] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/content/browser/cache_storage/cache_storage_cache_unittest.cc
[add] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/resources/vary.py
[modify] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/script-tests/cache-add.js
[delete] https://crrev.com/2d4420008d2c3a2cb08ecfb4be6a997fc10df7b7/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/serviceworker/cache-add.https-expected.txt
[delete] https://crrev.com/2d4420008d2c3a2cb08ecfb4be6a997fc10df7b7/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/window/cache-add.https-expected.txt
[delete] https://crrev.com/2d4420008d2c3a2cb08ecfb4be6a997fc10df7b7/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/worker/cache-add.https-expected.txt
[modify] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom
[modify] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/third_party/blink/renderer/modules/cache_storage/cache_storage_error.cc
[modify] https://crrev.com/74fd352e51322e823070088c7d5c72e911966580/third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc

Labels: Target-70
Status: Fixed (was: Started)
Labels: M-70
Status: Assigned (was: Fixed)
I'm not quite done here.  I want to add a UMA histogram and improve the devtools logging when the error is encountered.
After even further discussion we are going to move this under the experimental flag and do a proper intent.
Project Member

Comment 26 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0bac1c328c8d6918274df795cb563cc4e9db3578

commit 0bac1c328c8d6918274df795cb563cc4e9db3578
Author: Ben Kelly <wanderview@chromium.org>
Date: Thu Aug 16 22:15:13 2018

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

This reverts commit 74fd352e51322e823070088c7d5c72e911966580.

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/1178502
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583849}
[modify] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/content/browser/cache_storage/cache_storage_cache_unittest.cc
[delete] https://crrev.com/685952aa5199fdec0a75b2e0bc8f1b35340beb49/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/resources/vary.py
[modify] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/script-tests/cache-add.js
[add] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/serviceworker/cache-add.https-expected.txt
[add] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/window/cache-add.https-expected.txt
[add] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/worker/cache-add.https-expected.txt
[modify] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom
[modify] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/blink/renderer/modules/cache_storage/cache_storage_error.cc
[modify] https://crrev.com/0bac1c328c8d6918274df795cb563cc4e9db3578/third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Aug 17

Project Member

Comment 28 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05

commit 83d94ab1af02bcad12fb52c7ada434d5bc9d1a05
Author: Ben Kelly <wanderview@chromium.org>
Date: Mon Aug 20 18:10:41 2018

Experimentally reject cache.addAll() calls with duplicate entries.

This implements step 4.2.3 of the BatchCacheOperations algorithm:

https://w3c.github.io/ServiceWorker/#batch-cache-operations-algorithm

Its currently behind the experimental web features flag until web
compatibility can be assessed.

Bug: 720919
Change-Id: Id35b6f45ab4c02b9e2225d740a257ea5a4fe6739
Reviewed-on: https://chromium-review.googlesource.com/1179943
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584499}
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/content/browser/cache_storage/cache_storage_cache.h
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/content/browser/cache_storage/cache_storage_cache_unittest.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/content/browser/cache_storage/cache_storage_manager_unittest.cc
[delete] https://crrev.com/b55c521d1fe9c2544961e2e6f6b380e3bd3a456c/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/serviceworker/cache-add.https-expected.txt
[delete] https://crrev.com/b55c521d1fe9c2544961e2e6f6b380e3bd3a456c/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/window/cache-add.https-expected.txt
[delete] https://crrev.com/b55c521d1fe9c2544961e2e6f6b380e3bd3a456c/third_party/WebKit/LayoutTests/external/wpt/service-workers/cache-storage/worker/cache-add.https-expected.txt
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/third_party/blink/renderer/modules/cache_storage/cache_storage_error.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/third_party/blink/renderer/modules/cache_storage/cache_test.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc
[modify] https://crrev.com/83d94ab1af02bcad12fb52c7ada434d5bc9d1a05/third_party/blink/renderer/platform/runtime_enabled_features.json5

Working on adding console reporting in this CL to help developers more easily catch and fix duplicates:

https://chromium-review.googlesource.com/c/chromium/src/+/1174659

After that I will add the UMA.  I plan to do that as an enumeration like "Success", "SuccessWithDuplicate", "Error", etc.  We are mainly interested in successful addAll() calls with duplicates that would become errors.
Project Member

Comment 30 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35594ff19f14b05e17dc50552aa5121bda288a7a

commit 35594ff19f14b05e17dc50552aa5121bda288a7a
Author: Ben Kelly <wanderview@chromium.org>
Date: Fri Aug 24 16:32:12 2018

Report duplicate URLs when cache.addAll() fails.

The spec requires rejecting cache.addAll() with an InvalidStateError.
This CL improves the exception message by describing which requests
were found to be duplicates.  If duplicate rejection is turned off
via the feature flag then the list of duplicates will be reported
to the web console.

Bug: 720919
Change-Id: I7a9faaa45f8ce2a8b2b3b4006e0ae1d2b7ae8754
Reviewed-on: https://chromium-review.googlesource.com/1174659
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585865}
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/content/browser/background_fetch/background_fetch_data_manager_unittest.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/content/browser/cache_storage/cache_storage_cache.h
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/content/browser/cache_storage/cache_storage_cache_unittest.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/content/browser/cache_storage/cache_storage_manager_unittest.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/renderer/modules/cache_storage/cache.h
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/renderer/modules/cache_storage/cache_storage_error.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/renderer/modules/cache_storage/cache_storage_error.h
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/renderer/modules/cache_storage/cache_test.cc
[modify] https://crrev.com/35594ff19f14b05e17dc50552aa5121bda288a7a/third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8219bbd75cb633925a28f692a5dabb2c749e58c1

commit 8219bbd75cb633925a28f692a5dabb2c749e58c1
Author: Ben Kelly <wanderview@chromium.org>
Date: Fri Aug 24 21:46:59 2018

DCHECK() that options are not passed for multi-operation batch calls.

R=jsbell@chromium.org

Bug: 720919
Change-Id: I192aaba2b58ac7edd9b59212dd48f4fa0cb16379
Reviewed-on: https://chromium-review.googlesource.com/1188850
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586002}
[modify] https://crrev.com/8219bbd75cb633925a28f692a5dabb2c749e58c1/content/browser/cache_storage/cache_storage_cache.cc

Blocking: 877737
Project Member

Comment 33 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6731b4d07208faeafaba0a95b6a99a77fdc39c3b

commit 6731b4d07208faeafaba0a95b6a99a77fdc39c3b
Author: Ben Kelly <wanderview@chromium.org>
Date: Mon Aug 27 23:02:55 2018

Add a UserCounter to measure cache.addAll() with duplicate requests.

R=jsbell@chromium.org

Bug: 720919
Change-Id: I9a5cf22455b1d98fa7e3660f233d81abc90e1a71
Reviewed-on: https://chromium-review.googlesource.com/1189203
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586471}
[modify] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/common/BUILD.gn
[add] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/common/cache_storage/OWNERS
[add] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/common/cache_storage/cache_storage_utils.cc
[modify] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/public/common/BUILD.gn
[add] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/public/common/cache_storage/OWNERS
[add] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/public/common/cache_storage/cache_storage_utils.h
[modify] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/public/platform/web_feature.mojom
[modify] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/6731b4d07208faeafaba0a95b6a99a77fdc39c3b/tools/metrics/histograms/enums.xml

Labels: -M-70
Labels: -Target-70 Target-71
Labels: -Target-71 Target-72
We are going to deprecate in 71 and unship in 72.
Project Member

Comment 38 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f03fa88ca6bd99f1244dbbb6e740bce356bf75d

commit 5f03fa88ca6bd99f1244dbbb6e740bce356bf75d
Author: Ben Kelly <wanderview@chromium.org>
Date: Mon Oct 15 14:11:52 2018

Ship removal of duplicate requests in cache.addAll().

This enables removes the feature previously deprecated in 71.  See
the blink-dev discussion here:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/l2YVa_M4ODs/o0PqQOl6CgAJ

Bug: 720919
Change-Id: I2e4ee40b34385ccf88fbdcbf97702aea11f7e2b1
Reviewed-on: https://chromium-review.googlesource.com/c/1278905
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599631}
[modify] https://crrev.com/5f03fa88ca6bd99f1244dbbb6e740bce356bf75d/third_party/blink/renderer/platform/runtime_enabled_features.json5

Sign in to add a comment