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

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

Created:
2 years, 1 month ago by nhiroki
Modified:
2 years, 1 month 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
Trybot results:  win_chromium_rel_ng   mac_chromium_gn_rel   ios_rel_device_ninja   mac_chromium_compile_dbg_ng   ios_dbg_simulator_ninja   mac_chromium_rel_ng   linux_chromium_chromeos_compile_dbg_ng   android_chromium_gn_compile_rel   linux_android_rel_ng   linux_chromium_asan_rel_ng   android_chromium_gn_compile_dbg   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   android_clang_dbg_recipe   chromeos_daisy_chromium_compile_only_ng   android_arm64_dbg_recipe   android_compile_dbg   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_compile_dbg_32_ng   linux_chromium_chromeos_rel_ng   linux_chromium_gn_chromeos_rel   cast_shell_android   win_chromium_compile_dbg_ng   win8_chromium_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   win8_chromium_ng   mac_chromium_rel_ng   mac_chromium_gn_rel   mac_chromium_compile_dbg_ng   ios_rel_device_ninja   ios_dbg_simulator_ninja   linux_chromium_rel_ng   linux_chromium_gn_chromeos_rel   linux_chromium_compile_dbg_32_ng   linux_chromium_clobber_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_android_rel_ng   linux_chromium_asan_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   cast_shell_android   android_clang_dbg_recipe   android_compile_dbg   android_chromium_gn_compile_rel   android_chromium_gn_compile_dbg   android_arm64_dbg_recipe 

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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2015-10-07 08:33:03 UTC) #7
nhiroki
Updated. Can you take another look? Thanks!
2 years, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2015-10-19 04:09:35 UTC) #16
horo
lgtm
2 years, 1 month 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, 1 month 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, 1 month ago (2015-10-19 09:51:43 UTC) #20
commit-bot: I haz the power
Committed patchset #2 (id:120001)
2 years, 1 month ago (2015-10-19 11:03:06 UTC) #21
commit-bot: I haz the power
2 years, 1 month 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 efc10ee0f