Issue metadata
Sign in to add a comment
|
importScripts should throw an error when used to load new resource after `install` event
Reported by
m...@bocoup.com,
May 5 2017
|
||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/58.0.3029.81 Chrome/58.0.3029.81 Safari/537.36 Steps to reproduce the problem: Given the URL to a valid script that has not yet been loaded, invoke `importScripts` with that URL in a service worker's "message" event handler. What is the expected behavior? The function should throw a TypeError without making a network request. What went wrong? In Chromium today, no error is thrown and the script is loaded successfully. Did this work before? N/A Does this work in other browsers? N/A Chrome version: 58.0.3029.81 Channel: n/a OS Version: Flash Version: The behavior of the `importScripts` function is based on both the "script resource map" and the "imported scripts updated flag." [1] The "imported scripts updated" flag is set as part of the "install" algorithm [2]. If that function is invoked with a URL that has not previously been imported, the function should throw a TypeError. [1] From https://w3c.github.io/ServiceWorker/#importscripts > 1. Let serviceWorker be request’s client's global object's service worker. > 2. If serviceWorker’s imported scripts updated flag is unset, then: > [...] > 3. Else: > 1. If script resource map[url] exists, return script resource map[url]. > 2. Else, return a network error. [2] From https://w3c.github.io/ServiceWorker/#installation-algorithm > 14. Set registration’s installing worker’s imported scripts updated flag.
,
May 8 2017
This bug will cause import-scripts-updated-flag.https.html to fail once https://codereview.chromium.org/2864783002/ lands.
,
May 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9f34ed893dd0ab15ac4834becee26c4886620d7 commit e9f34ed893dd0ab15ac4834becee26c4886620d7 Author: mike <mike@mikepennisi.com> Date: Mon May 08 20:25:49 2017 Upstream service wrkr `importScripts` tests to WPT In order to contribute this test logic to the Web Platform Tests project, the invalid assertion it includes needed to be corrected. Because the test was previously formatted as a single sub-test, the resultant failure tended to obscure the results of assertions for independent conditions. Re-factor the test to support more fine-grained reporting, and white-list the specific failure as "allowed" in the Chromium build infrastructure. BUG= 688116 , 719052 R=mek@chromium.org Review-Url: https://codereview.chromium.org/2864783002 Cr-Commit-Position: refs/heads/master@{#470105} [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/import-scripts-resource-map.https.html [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/import-scripts-updated-flag.https-expected.txt [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/import-scripts-updated-flag.https.html [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/import-scripts-echo.py [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/import-scripts-resource-map-worker.js [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/import-scripts-updated-flag-worker.js [add] https://crrev.com/e9f34ed893dd0ab15ac4834becee26c4886620d7/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/import-scripts-version.py [delete] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/LayoutTests/http/tests/serviceworker/importScripts.html [delete] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/duplicate-import-worker.js [delete] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/echo.php [delete] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/get-version.php [delete] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/import-scripts-worker.js
,
Jun 29 2017
,
Jun 29 2017
,
Jun 29 2017
,
Mar 9 2018
,
Mar 9 2018
I started looking at this again. Unfortunately the usage is pretty widespread: 5- 10% of importScripts from an installed worker are hitting this deprecated path (https://uma.googleplex.com/p/chrome/timeline_v2/?sid=0edac543a677cfe961e6059f1ff069e2) I found some sites doing this. The ones I saw are doing something like importScripts('mycdn.com/sw.js?' + Math.random()) So they are trying to bust the HTTP cache. But it'll result in slow service workers. Long-term, we want to support updateViaCache enum and the byte-to-byte check over importScripts, that will help ensure the CDN script is updated frequently, but we are a bit a ways from that. OTOH we'd want to deprecate this before it gets even more widespread.
,
Jul 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2838dd71b6be5ec2a6dba100038351bcda2f5eb0 commit 2838dd71b6be5ec2a6dba100038351bcda2f5eb0 Author: Kenichi Ishibashi <bashi@chromium.org> Date: Mon Jul 09 03:53:17 2018 service worker: Add UseCounter for importScripts() after main script installation Currently we allow importScripts() to load a new script after service worker's installation. According to the spec we should throw a TypeError in such case. We want to change the behavior but it seems that some sites depend on the current behavior so changing the behavior is a bit risky at this point. This CL add a UseCounter to track such usage. Bug: 852666 , 719052 Change-Id: I3e5748a7ce037be75f4f2dfa965d8cb34df9b9bc Reviewed-on: https://chromium-review.googlesource.com/1126799 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Kenichi Ishibashi <bashi@chromium.org> Cr-Commit-Position: refs/heads/master@{#573209} [modify] https://crrev.com/2838dd71b6be5ec2a6dba100038351bcda2f5eb0/third_party/blink/public/platform/web_feature.mojom [modify] https://crrev.com/2838dd71b6be5ec2a6dba100038351bcda2f5eb0/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.cc [modify] https://crrev.com/2838dd71b6be5ec2a6dba100038351bcda2f5eb0/tools/metrics/histograms/enums.xml
,
Aug 10
,
Aug 16
Intent to deprecate was approved. bashi@ would you be able to add the deprecation warning and counter as per https://www.chromium.org/blink/removing-features? We should get it in for 70 in order to remove in 71.
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/71120bdbfbb59f6a896777d4ddfb75702c22c44d commit 71120bdbfbb59f6a896777d4ddfb75702c22c44d Author: Kenichi Ishibashi <bashi@chromium.org> Date: Fri Aug 17 00:14:23 2018 service worker: Show deprecation message for importScripts() after installation We are going to deprecate importScripts() of new scripts after service worker installation as described in the intent to deprecation[1]. This CL adds deprecation message for use of such importScripts(). [1] https://groups.google.com/a/chromium.org/d/msg/blink-dev/a6P-niHWgF4/CtJEHCnKDwAJ Bug: 719052 Change-Id: I6630b5c436605ab4fdbc9fb783c166b96dc4d760 Reviewed-on: https://chromium-review.googlesource.com/1177202 Reviewed-by: David Bokan <bokan@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Kenichi Ishibashi <bashi@chromium.org> Cr-Commit-Position: refs/heads/master@{#583894} [modify] https://crrev.com/71120bdbfbb59f6a896777d4ddfb75702c22c44d/third_party/blink/renderer/core/frame/deprecation.cc [modify] https://crrev.com/71120bdbfbb59f6a896777d4ddfb75702c22c44d/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc
,
Aug 17
Nice. Let's target the actual removal in 71. Setting next action to after the expected branch date for 70.
,
Sep 15
The NextAction date has arrived: 2018-09-15
,
Sep 27
bashi@: Did you want to continue this work to throw the error?
,
Sep 28
Yes, I'll prepare a CL.
,
Sep 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b393fe714922cde85602802e7c0a158d272f6c4 commit 3b393fe714922cde85602802e7c0a158d272f6c4 Author: Kenichi Ishibashi <bashi@chromium.org> Date: Sat Sep 29 10:32:00 2018 Disallow importScripts() of new scripts after service worker installation Intent to Deprecate and Remove: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/a6P-niHWgF4/CtJEHCnKDwAJ Bug: 719052 Change-Id: I7ed1af009b41adef50b2d73301d8577b47baa8c6 Reviewed-on: https://chromium-review.googlesource.com/1249466 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Commit-Queue: Kenichi Ishibashi <bashi@chromium.org> Cr-Commit-Position: refs/heads/master@{#595314} [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/content/browser/service_worker/embedded_worker_instance.cc [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/content/browser/service_worker/service_worker_context_request_handler.cc [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/content/browser/service_worker/service_worker_metrics.cc [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/content/browser/service_worker/service_worker_metrics.h [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/content/browser/service_worker/service_worker_script_loader_factory.cc [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/content/browser/service_worker/service_worker_script_loader_factory.h [delete] https://crrev.com/0681bd8883df94388fd52e559a8db581a308aee6/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/import-scripts-updated-flag.https-expected.txt [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/third_party/blink/renderer/core/frame/deprecation.cc [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/tools/metrics/histograms/enums.xml [modify] https://crrev.com/3b393fe714922cde85602802e7c0a158d272f6c4/tools/metrics/rappor/rappor.xml
,
Oct 1
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by falken@chromium.org
, May 8 2017