New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 648295 link

Starred by 26 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 889798
issue 661604


Participants' hotlists:
UpdateImportedScripts

Show other hotlists

Other hotlists containing this issue:
Hotlist-1


Sign in to add a comment

Extend byte-for-byte update check to all service worker importScripts() resources

Project Member Reported by jeffy@google.com, Sep 19 2016

Issue description

Taken from the Mozilla bug tracking the implementation in Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=1290951):

See:

https://github.com/slightlyoff/ServiceWorker/issues/839#issuecomment-236256162

Basically this means iterating over every entry in the service worker's associated CacheStorage and performing a byte-for-byte check.  This can be done in parallel.

Design doc: https://docs.google.com/document/d/1p_2axsp96GfME5lE4Zd-p5ei80-9OD_brFUjLYyN8B0/edit
 
Project Member

Comment 1 by sheriffbot@chromium.org, Sep 20 2016

Labels: Hotlist-Google
Labels: -Pri-3 Pri-2
Status: Available (was: Untriaged)
Labels: M-56
Blocking: 661604

Comment 5 by falken@chromium.org, Dec 19 2016

Labels: -M-56 M-57
Owner: falken@chromium.org
Status: Started (was: Available)

Comment 6 by falken@chromium.org, Jan 26 2017

Labels: -M-57 M-58
Status: Assigned (was: Started)
"Started" is no longer accurate but it's still high priority. I started looking at this and ended up fixing  Issue 485900 , which should set some of the stage for this.

Comment 7 by falken@chromium.org, Feb 16 2017

Labels: WorkerPainPoint

Comment 8 by bke...@mozilla.com, Mar 30 2017

FWIW, we're close to landing this in FF55.  We have a WPT test written in:

https://bugzilla.mozilla.org/show_bug.cgi?id=1290951

Comment 9 by falken@chromium.org, Jul 14 2017

Labels: -M-58
Sorry, didn't get to this in 58 at all...

We probably won't get to this until servicification settles down.
Cc: shimazu@chromium.org
Note: I'm thinking this is suitable for an intern project. 
external/wpt/service-workers/service-worker/update-bytecheck.https.html is failing due to this.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 3 2018

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

commit b849563d597779aa87924d39d28e04c012919dfd
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Tue Apr 03 08:02:25 2018

Update test expectations for update-bytecheck.https.html

This is failing because we don't support byte-for-byte update check
to all service worker importScript() resources. Update test expectations
to point an appropriate bug entry and remove the entry from S13nSW
expectation as this failure isn't specific to S13nSW.

Bug: 648295
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I65dca2a31baa500ae8f1d347af8c77d9e9fd8c29
Reviewed-on: https://chromium-review.googlesource.com/991472
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547643}
[modify] https://crrev.com/b849563d597779aa87924d39d28e04c012919dfd/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[modify] https://crrev.com/b849563d597779aa87924d39d28e04c012919dfd/third_party/WebKit/LayoutTests/TestExpectations

Cc: -shimazu@chromium.org
Owner: shimazu@chromium.org
shimazu has a plan here.
Cc: shimazu@chromium.org
Owner: momohatt@google.com
Let me pass this to my intern. She will work on this as her main project.
Labels: Target-71
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 29

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

commit fc3522926869bf9cbaca9111eaacebaf412ad038
Author: momohatt <momohatt@google.com>
Date: Wed Aug 29 05:12:36 2018

Add ServiceWorkerImportedScriptUpdateCheck flag

This CL tentatively adds ServiceWorkerImportedScriptUpdateCheck flag.
It is for the implementation of byte-for-byte update check for
service worker importScripts() resources. We are going to implement
the feature under this flag and flip it over once it is completed.

Bug: 648295
Change-Id: Iabb6140af84382ecd0d07ee28283490c303e8c4d
Reviewed-on: https://chromium-review.googlesource.com/1192571
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587017}
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/chrome/browser/about_flags.cc
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/third_party/blink/common/features.cc
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/third_party/blink/common/service_worker/service_worker_utils.cc
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/third_party/blink/public/common/features.h
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/third_party/blink/public/common/service_worker/service_worker_utils.h
[modify] https://crrev.com/fc3522926869bf9cbaca9111eaacebaf412ad038/tools/metrics/histograms/enums.xml

Comment 17 Deleted

Project Member

Comment 18 by bugdroid1@chromium.org, Sep 7

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

commit aec50d02dd761d59e93c4c9ddd45e534959fcfe9
Author: momohatt <momohatt@google.com>
Date: Fri Sep 07 11:33:59 2018

Add pause_when_not_identical option for ServiceWorkerCacheWriter

This patch adds pause_when_not_identical option for making
ServiceWorkerCacheWriter::MaybeWriteData() return before writing the
new body into the storage when the scripts are not identical.
This option is necessary for extending byte-for-byte update check
of service worker importScripts() resources.

Bug: 648295
Change-Id: If33412cd6e63393ad3fdae514b1588a33f17399a
Reviewed-on: https://chromium-review.googlesource.com/1198651
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589492}
[modify] https://crrev.com/aec50d02dd761d59e93c4c9ddd45e534959fcfe9/content/browser/service_worker/service_worker_cache_writer.cc
[modify] https://crrev.com/aec50d02dd761d59e93c4c9ddd45e534959fcfe9/content/browser/service_worker/service_worker_cache_writer.h
[modify] https://crrev.com/aec50d02dd761d59e93c4c9ddd45e534959fcfe9/content/browser/service_worker/service_worker_cache_writer_unittest.cc
[modify] https://crrev.com/aec50d02dd761d59e93c4c9ddd45e534959fcfe9/content/browser/service_worker/service_worker_new_script_loader.cc
[modify] https://crrev.com/aec50d02dd761d59e93c4c9ddd45e534959fcfe9/content/browser/service_worker/service_worker_write_to_cache_job.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Sep 11

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

commit 9a4538ad8d0bac55bb81aed0b8181a6b5ed1048b
Author: momohatt <momohatt@google.com>
Date: Tue Sep 11 08:13:44 2018

Add ServiceWorkerCacheWriter::Resume() for paused cache writers

This method resumes a cache writer which is paused by enabling
pause_when_not_identical option. With this option enabled, a cache
writer pauses right after it finds any differences with the cached body
and the body from network, and calling Resume() to such
paused cache writers makes them resume its internal state machine and
complete the remaining read and write to the storage. This method takes
a callback, but it is triggered only when the remaining read and write
finishes asynchronously.

Bug: 648295
Change-Id: I1c01804151bff20fc69ba7d0993447f676e98548
Reviewed-on: https://chromium-review.googlesource.com/1214975
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590231}
[modify] https://crrev.com/9a4538ad8d0bac55bb81aed0b8181a6b5ed1048b/content/browser/service_worker/service_worker_cache_writer.cc
[modify] https://crrev.com/9a4538ad8d0bac55bb81aed0b8181a6b5ed1048b/content/browser/service_worker/service_worker_cache_writer.h
[modify] https://crrev.com/9a4538ad8d0bac55bb81aed0b8181a6b5ed1048b/content/browser/service_worker/service_worker_cache_writer_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Sep 11

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

commit f24d40940a76b0ec817d5e3c54ea0718e4ee887a
Author: momohatt <momohatt@google.com>
Date: Tue Sep 11 10:52:43 2018

Add ServiceWorkerUpdateChecker

This patch adds a new class called ServiceWorkerUpdateChecker and
makes ServiceWorkerRegisterJob use this class. The new class has a
method called Start(), which takes a callback as an argument and
triggers it only when the scripts had any changes to update. Currently
this method runs the callback unconditionally, but following patches
will implement script loading and comparison with the stored resources.

Bug: 648295
Change-Id: Iebbe85b6d5e5cdab27c23873b04811626b5abad0
Reviewed-on: https://chromium-review.googlesource.com/1215458
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590260}
[modify] https://crrev.com/f24d40940a76b0ec817d5e3c54ea0718e4ee887a/content/browser/BUILD.gn
[modify] https://crrev.com/f24d40940a76b0ec817d5e3c54ea0718e4ee887a/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/f24d40940a76b0ec817d5e3c54ea0718e4ee887a/content/browser/service_worker/service_worker_register_job.h
[add] https://crrev.com/f24d40940a76b0ec817d5e3c54ea0718e4ee887a/content/browser/service_worker/service_worker_update_checker.cc
[add] https://crrev.com/f24d40940a76b0ec817d5e3c54ea0718e4ee887a/content/browser/service_worker/service_worker_update_checker.h

Project Member

Comment 21 by bugdroid1@chromium.org, Sep 13

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

commit 130be1c380e444e53a2e25302f50629af94682a5
Author: momohatt <momohatt@google.com>
Date: Thu Sep 13 12:18:08 2018

Add ServiceWorkerSingleScriptUpdateChecker

This patch adds a new class called ServiceWorkerSingleScriptUpdateChecker,
and makes ServiceWorkerUpdateChecker call this class. This new class
currently calls UpdateChecker's method unconditionally, but following
patches will implement these to SWSingleScriptUpdateChecker:
* collect stored scripts which are used in the version to update
* load the counterpart of the stored scripts from network
* compare the stored scripts and the loaded scripts one by one
* when detected any changes, pause the comparison and stop the entire update check

Bug: 648295
Change-Id: I23835fa62838b9c1a979e7ae96f296de6b6ff074
Reviewed-on: https://chromium-review.googlesource.com/1220854
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590979}
[modify] https://crrev.com/130be1c380e444e53a2e25302f50629af94682a5/content/browser/BUILD.gn
[modify] https://crrev.com/130be1c380e444e53a2e25302f50629af94682a5/content/browser/service_worker/service_worker_register_job.cc
[add] https://crrev.com/130be1c380e444e53a2e25302f50629af94682a5/content/browser/service_worker/service_worker_single_script_update_checker.cc
[add] https://crrev.com/130be1c380e444e53a2e25302f50629af94682a5/content/browser/service_worker/service_worker_single_script_update_checker.h
[modify] https://crrev.com/130be1c380e444e53a2e25302f50629af94682a5/content/browser/service_worker/service_worker_update_checker.cc
[modify] https://crrev.com/130be1c380e444e53a2e25302f50629af94682a5/content/browser/service_worker/service_worker_update_checker.h

Project Member

Comment 22 by bugdroid1@chromium.org, Sep 28

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

commit a20c193cadb0b7f574550d833f3f01832a8f35b3
Author: momohatt <momohatt@google.com>
Date: Fri Sep 28 13:43:17 2018

Implement ServiceWorkerSingleScriptUpdateChecker

This patch enables ServiceWorkerSingleScriptUpdateChecker to check if
there are any differences between the stored scripts and its
counterparts loaded from the network. The comparison is only performed
for scripts that are used in the instance of requested SWVersion.
This impl doesn't yet allow SWNewScriptLoader to continue the aborted
comparison, but future patches will implement:
* passing the result of update check to SWNewScriptLoader
* record the info about scripts which are already compared and known to
be identical

Bug: 648295
Change-Id: If89dd415578b1c5809159f196d398447a96a812f
Reviewed-on: https://chromium-review.googlesource.com/1224610
Commit-Queue: Momoko Hattori <momohatt@google.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595072}
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_cache_writer_unittest.cc
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_single_script_update_checker.cc
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_single_script_update_checker.h
[add] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_single_script_update_checker_unittest.cc
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_test_utils.cc
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_test_utils.h
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_update_checker.cc
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/browser/service_worker/service_worker_update_checker.h
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/content/test/BUILD.gn
[modify] https://crrev.com/a20c193cadb0b7f574550d833f3f01832a8f35b3/tools/traffic_annotation/summary/annotations.xml

Cc: -shimazu@chromium.org momohatt@google.com
Labels: -Target-71 Target-72
Owner: shimazu@chromium.org
Status: Started (was: Assigned)
I'll take it over since her internship was over.
Blocking: 889798
Description: Show this description
Owner: ting.s...@intel.com
I started to work on this, thanks.
Cc: -momohatt@google.com momohat...@gmail.com shimazu@chromium.org
Thanks for taking it over :)
Project Member

Comment 28 by bugdroid1@chromium.org, Jan 14

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

commit e1bc015ec830751da0e3be1bb823281208cd9a61
Author: Ting Shao <ting.shao@intel.com>
Date: Mon Jan 14 03:49:14 2019

ServiceWorker: Collect update check result of scripts

Update check result of each script are collected and stored in
ServiceWorkerVersion. Following patches would use the data to
continue the update process if some script changed.

Bug: 648295
Change-Id: I2c62ab15db1618065199f948e47465530dded870
Reviewed-on: https://chromium-review.googlesource.com/c/1385858
Commit-Queue: Ting Shao <ting.shao@intel.com>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622379}
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_register_job.h
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_single_script_update_checker.cc
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_single_script_update_checker.h
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_single_script_update_checker_unittest.cc
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_update_checker.cc
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_update_checker.h
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/e1bc015ec830751da0e3be1bb823281208cd9a61/content/browser/service_worker/service_worker_version.h

Comment 29 by falken@chromium.org, Jan 17 (6 days ago)

ting.shao@: We talked about the spec algorithm a while back. I'm trying to improve it here: https://github.com/w3c/ServiceWorker/issues/1374 and https://github.com/w3c/ServiceWorker/pull/1377. Not sure if this work is blocking you.

Comment 30 by ting.s...@intel.com, Jan 17 (6 days ago)

falken@: Thanks for the note, I'm not blocked on this issue, I'm implementing the functionality for the compared identical scripts, so please go ahead. And I am also watching that issue.

Comment 31 by maddyro...@gmail.com, Jan 17 (5 days ago)

Would this bug fix be available in Chrome 72 release? If yes, which would be the beta build to test this change?

Comment 32 by ting.s...@intel.com, Jan 21 (2 days ago)

Labels: -Target-72 Target-74
maddyrockz@: I'm sorry that, as there are still more work to be done for this issue, so this bug can't be included in 72, I will update the target field.

Sign in to add a comment