Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 1393783002: ServiceWorker: Schedule DeleteAndStartOver when failing to store resource ids (Closed)

Created:
2 years, 6 months ago by nhiroki
Modified:
2 years, 6 months ago
Reviewers:
falken, horo
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, blink-worker-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ServiceWorker: Schedule DeleteAndStartOver when failing to store resource ids Storing uncommitted or purgeable resource ids maybe bumps the next available resource id stored in ServiceWorkerDatabase. If these database operations fail, subsequent resources could use conflicted ids. To avoid the confliction, this CL makes ServiceWorkerStorage check the result of storing resource ids and schedule DeleteAndStartOver if there is an error. BUG=485900, 505322 Committed: https://crrev.com/979ef438eb2908e88c1963de44a0942cc978d79e Cr-Commit-Position: refs/heads/master@{#354758}

Patch Set 1 : #

Total comments: 3

Patch Set 2 : simplify the patch #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -42 lines) Patch
M content/browser/service_worker/service_worker_script_cache_map.cc View 1 2 chunks +2 lines, -2 lines 1 comment Download
M content/browser/service_worker/service_worker_storage.h View 1 3 chunks +12 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_storage.cc View 1 4 chunks +52 lines, -29 lines 6 comments Download
M content/browser/service_worker/service_worker_storage_unittest.cc View 3 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 22 (7 generated)
nhiroki
PTAL, thanks! https://codereview.chromium.org/1393783002/diff/40001/content/browser/service_worker/service_worker_script_cache_map.cc File content/browser/service_worker/service_worker_script_cache_map.cc (left): https://codereview.chromium.org/1393783002/diff/40001/content/browser/service_worker/service_worker_script_cache_map.cc#oldcode66 content/browser/service_worker/service_worker_script_cache_map.cc:66: context_->storage()->DoomUncommittedResponse(LookupResourceId(url)); Renamed this to ClearUncommittedResourceId "DoomUncommitted" sounds ...
2 years, 6 months ago (2015-10-07 06:10:27 UTC) #4
falken
My understanding was uncommitted resources are in the disk cache, but they don't yet belong ...
2 years, 6 months ago (2015-10-07 07:37:02 UTC) #5
nhiroki
Thank you for reviewing! https://codereview.chromium.org/1393783002/diff/40001/content/browser/service_worker/service_worker_script_cache_map.cc File content/browser/service_worker/service_worker_script_cache_map.cc (left): https://codereview.chromium.org/1393783002/diff/40001/content/browser/service_worker/service_worker_script_cache_map.cc#oldcode66 content/browser/service_worker/service_worker_script_cache_map.cc:66: context_->storage()->DoomUncommittedResponse(LookupResourceId(url)); On 2015/10/07 06:10:27, nhiroki ...
2 years, 6 months ago (2015-10-07 08:24:43 UTC) #6
nhiroki
On 2015/10/07 07:37:02, falken wrote: > My understanding was uncommitted resources are in the disk ...
2 years, 6 months ago (2015-10-07 08:33:03 UTC) #7
nhiroki
Updated. Can you take another look? Thanks!
2 years, 6 months ago (2015-10-14 10:09:13 UTC) #11
falken
Code change looks good. Do you think it's useful and feasible to make a test ...
2 years, 6 months ago (2015-10-15 05:03:40 UTC) #12
nhiroki
On 2015/10/15 05:03:40, falken wrote: > Code change looks good. Do you think it's useful ...
2 years, 6 months ago (2015-10-16 09:08:37 UTC) #13
nhiroki
Thank you for reviewing! https://codereview.chromium.org/1393783002/diff/120001/content/browser/service_worker/service_worker_storage.cc File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1393783002/diff/120001/content/browser/service_worker/service_worker_storage.cc#newcode540 content/browser/service_worker/service_worker_storage.cc:540: DCHECK_NE(kInvalidServiceWorkerResponseId, resource_id); On 2015/10/15 05:03:40, ...
2 years, 6 months ago (2015-10-16 09:15:40 UTC) #14
nhiroki
On 2015/10/16 09:08:37, nhiroki wrote: > On 2015/10/15 05:03:40, falken wrote: > > Code change ...
2 years, 6 months ago (2015-10-16 09:40:47 UTC) #15
falken
lgtm https://codereview.chromium.org/1393783002/diff/120001/content/browser/service_worker/service_worker_storage.cc File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1393783002/diff/120001/content/browser/service_worker/service_worker_storage.cc#newcode1156 content/browser/service_worker/service_worker_storage.cc:1156: if (status != ServiceWorkerDatabase::STATUS_OK) On 2015/10/16 09:15:40, nhiroki ...
2 years, 6 months ago (2015-10-19 04:09:35 UTC) #16
horo
lgtm
2 years, 6 months ago (2015-10-19 05:39:08 UTC) #17
nhiroki
Thank you! https://codereview.chromium.org/1393783002/diff/120001/content/browser/service_worker/service_worker_storage.cc File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1393783002/diff/120001/content/browser/service_worker/service_worker_storage.cc#newcode1156 content/browser/service_worker/service_worker_storage.cc:1156: if (status != ServiceWorkerDatabase::STATUS_OK) On 2015/10/19 04:09:35, ...
2 years, 6 months ago (2015-10-19 09:50:42 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393783002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393783002/120001
2 years, 6 months ago (2015-10-19 09:51:43 UTC) #20
commit-bot: I haz the power
Committed patchset #2 (id:120001)
2 years, 6 months ago (2015-10-19 11:03:06 UTC) #21
commit-bot: I haz the power
2 years, 6 months ago (2015-10-19 11:03:54 UTC) #22
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/979ef438eb2908e88c1963de44a0942cc978d79e
Cr-Commit-Position: refs/heads/master@{#354758}

Powered by Google App Engine
This is Rietveld 408576698