New issue
Advanced search Search tips

Issue 842563 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 715640



Sign in to add a comment

NetS13nServiceWorker: Implement 24-hour/devtools cache busting for update checks.

Project Member Reported by falken@chromium.org, May 14 2018

Issue description

Service worker update checks bypass the browser cache if the last check was >24 hours ago or if DevTools "Update" button or "Update on reload" is checked.

In non-NetS13nSW this is implemented in service_worker_context_request_handler.cc:

  if (ServiceWorkerUtils::ShouldBypassCacheDueToUpdateViaCache(
          is_main_script, registration->update_via_cache()) ||
      time_since_last_check > kServiceWorkerScriptMaxCacheAge ||
      version_->force_bypass_cache_for_scripts()) {
    extra_load_flags = net::LOAD_BYPASS_CACHE;
  }


For NetS13nSW we probably need to implement it in service_worker_new_script_loader.cc.

Regarding testing, it looks like there are no browser tests for this, just SWContextRequestHandler unit tests, so NetS13nSW currently doesn't fail any tests about this. We probably can just fix it and add a unit test, although having a browser test or other integration test would be nice as well. There is one WPT test but it requires a browser flag that Chrome doesn't know, see  bug 691944 .
 

Comment 1 by falken@chromium.org, May 24 2018

Blockedon: -715640
Blocking: 715640
Labels: Proj-Servicification-Canary
Owner: falken@chromium.org
Status: Started (was: Available)
I'll take this.
ServiceWorkerNewScriptLoader needs to look at the registration's last update check time and bypass cache if 24 hours elapsed.

It then needs to bump ServiceWorkerRegistration's set_last_update_check if it accessed network (ResourceResponsInfo::network_accessed) and then persist to storage UpdateLastUpdateCheckTime().

Finally we need a browser_test likely. ServiceWorkerBrowserTest can make an active version, trigger an update that hits cache, set update time to a time in the past, trigger an update that hits network.


Project Member

Comment 4 by bugdroid1@chromium.org, Jun 4 2018

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

commit dd9b077c2eb4a5de2d272e43d771ade3aff852b5
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jun 04 09:56:40 2018

service worker: Add a browser test for 24 hour cache bypassing.

This adds a test which fails with NetworkService, which demonstrates the
linked bug. The bug will be fixed in a next patch.

Bug:  842563 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I2b33cd61ee2502db6ad85c44885112fc6edbc5e1
Reviewed-on: https://chromium-review.googlesource.com/1084418
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564042}
[modify] https://crrev.com/dd9b077c2eb4a5de2d272e43d771ade3aff852b5/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/dd9b077c2eb4a5de2d272e43d771ade3aff852b5/content/browser/service_worker/service_worker_context_core.h
[modify] https://crrev.com/dd9b077c2eb4a5de2d272e43d771ade3aff852b5/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 5 2018

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

commit 308e0265e962aef0e637c20705c1fa8f2c95fc1e
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jun 05 08:39:39 2018

NetS13nServiceWorker: Implement 24-hour/devtools cache busting for update checks.

Bug:  842563 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I14be788c93d0c9be75feb6faa0712285a70034ce
Reviewed-on: https://chromium-review.googlesource.com/1086668
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564409}
[modify] https://crrev.com/308e0265e962aef0e637c20705c1fa8f2c95fc1e/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/308e0265e962aef0e637c20705c1fa8f2c95fc1e/content/browser/service_worker/service_worker_new_script_loader_unittest.cc
[modify] https://crrev.com/308e0265e962aef0e637c20705c1fa8f2c95fc1e/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 5 2018

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

commit 7af9db98834e785d378f66c5b8a1707ed0a79f42
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jun 05 14:24:58 2018

NetS13nServiceWorker: Bump the last update check timestamp when network is accessed.

This sets the starting time for the 24 hour cache bypass. This matches
the legacy implementation.

Bug:  842563 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Iac3bad4534c1480083815cedb09a886bad7e8c3b
Reviewed-on: https://chromium-review.googlesource.com/1086826
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564485}
[modify] https://crrev.com/7af9db98834e785d378f66c5b8a1707ed0a79f42/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/7af9db98834e785d378f66c5b8a1707ed0a79f42/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/7af9db98834e785d378f66c5b8a1707ed0a79f42/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/7af9db98834e785d378f66c5b8a1707ed0a79f42/content/browser/service_worker/service_worker_new_script_loader_unittest.cc

Status: Fixed (was: Started)
Labels: M-69

Sign in to add a comment