New issue
Advanced search Search tips

Issue 814583 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 715640



Sign in to add a comment

S13nServiceWorker: external/wpt/service-workers/service-worker/import-scripts-resource-map.https.html crashes

Project Member Reported by falken@chromium.org, Feb 22 2018

Issue description

S13nServiceWorker: external/wpt/service-workers/service-worker/import-scripts-resource-map.https.html crashes
 

Comment 1 by falken@chromium.org, Feb 22 2018

Cc: shimazu@chromium.org
This is testing installing a service worker that does:

importScripts('dupe.js');
importScripts('dupe.js');  // again

The first importScript should be stored, and the second importScript should just read the stored one instead of going to network.

We have some comments in SWScriptURLLoaderFactory about this:
  // TODO: Make sure we come here only for new / unknown scripts
  // once script streaming manager in the renderer side stops sending
  // resource requests for the known script URLs, i.e. add DCHECK for
  // version->script_cache_map()->LookupResourceId(url) ==
  // kInvalidServiceWorkerResourceId.
  //
  // Currently this could be false for the installing worker that imports
  // the same script twice (e.g. importScripts('dupe.js');
  // importScripts('dupe.js');).

So what we want to do is read the installed script and send it to the renderer as a URLLoader.

I think what we need to do is factor something out of ServiceWorkerInstalledScriptSender that can be given a ServiceWorkerResponseReader, and writes the metadata/body to a given data pipe. Or maybe make it a URLLoader. Did we have any plans in this space?
I vaguely remember that we've planned to dynamically update the list of installed scripts and ask for the scripts to the installed script manager.
But making it a URLLoader seems simpler since we may be able to have the logic to determine whether it's installed or not on the browser side.

Comment 3 by falken@chromium.org, Feb 22 2018

Thanks. I'm working on a patch to refactor ServiceWorkerInstalledScriptSender::Sender into its own class. That does the work of reading from disk and providing data pipes.

With that I think we can make a new URLLoader class, which only gets used for this duplicate case. It should be very lightweight: all it'll do is take the data pipes from Sender and give them to URLLoaderClient.
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 23 2018

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

commit a39b8ba5d25b20356438380c80ed95dce90f75e3
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Feb 23 07:51:30 2018

service worker: Extract ServiceWorkerInstalledScriptsSender::Sender into its own class.

This will be useful to reuse for the linked bug: it's a class that
takes a ServiceWorkerResponseReader and gives back data pipes that will
be useful for URLLoader.

This patch is mostly mechanical. Other than just moving the code, it
adds Client interface and renames some functions in
ServiceWorkerInstalledScriptsSender to use that interface. It also
does minor renaming like |owner_| -> |client_|.

R=shimazu

Bug:  814583 
Change-Id: I236eb8aeb4b7a79ee8584a413260fdbe3f629dbf
Reviewed-on: https://chromium-review.googlesource.com/930107
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538724}
[modify] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/BUILD.gn
[add] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_installed_script_reader.cc
[add] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_installed_script_reader.h
[modify] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_installed_scripts_sender.cc
[modify] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_installed_scripts_sender.h
[modify] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_installed_scripts_sender_unittest.cc
[modify] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/a39b8ba5d25b20356438380c80ed95dce90f75e3/content/browser/service_worker/service_worker_metrics.h

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 27 2018

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

commit 10fde2da6b806828bea35eed97d05d69c26053b3
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Feb 27 17:17:56 2018

S13nServiceWorker: Allow importScript() twice for non-installed workers.

An installing service worker can call importScripts('dupe.js');
importScripts('dupe.js');. The first call should install the script, and
the second call should read the installed script. This CL implements
that. We detect if the script is already installed, and if so we use
a simple URLLoader that reads the installed script.

This fixes WPT test import-scripts-resource-map.https.html.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I30c7532cedffaad3d62b12239ab230a0708bb49c
Bug:  814583 
Reviewed-on: https://chromium-review.googlesource.com/934024
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539474}
[modify] https://crrev.com/10fde2da6b806828bea35eed97d05d69c26053b3/content/browser/BUILD.gn
[add] https://crrev.com/10fde2da6b806828bea35eed97d05d69c26053b3/content/browser/service_worker/service_worker_installed_script_loader.cc
[add] https://crrev.com/10fde2da6b806828bea35eed97d05d69c26053b3/content/browser/service_worker/service_worker_installed_script_loader.h
[modify] https://crrev.com/10fde2da6b806828bea35eed97d05d69c26053b3/content/browser/service_worker/service_worker_script_url_loader_factory.cc
[modify] https://crrev.com/10fde2da6b806828bea35eed97d05d69c26053b3/content/browser/service_worker/service_worker_script_url_loader_factory.h
[modify] https://crrev.com/10fde2da6b806828bea35eed97d05d69c26053b3/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Comment 6 by falken@chromium.org, Feb 27 2018

Status: Fixed (was: Started)
 Issue 771527  has been merged into this issue.

Sign in to add a comment