New issue
Advanced search Search tips

Issue 774374 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 715640



Sign in to add a comment

S13nServiceWorker: Re-implement SW timers

Project Member Reported by kinuko@chromium.org, Oct 13 2017

Issue description

As we're changing how controllees talk to the controller from always-via-browser model to direct model, we'll need to re-implement following SW timers at least:

- Idle timer: browser no longer knows if the SW is busy or not (unless we propagate the events from controllee to the browser process too).  This can probably be implemented by the service worker itself, as it can tell when it's idle and can stop itself.

- Event timers for Fetch (and Message): ditto, controllees will start to send them directly to the controller.  This can probably be implemented in the controllee side.

We'll still need some of stuck-detection / last-resort timers in the browser process.


Design doc: https://docs.google.com/document/d/17Wrmzcy_gHeG5NSw5R92BrUD6JgdEgmBhr8W_DCqftI/edit#heading=h.dg831l8xtj2y
 

Comment 1 by kinuko@chromium.org, Oct 13 2017

Blocking: 715640

Comment 2 Deleted

Comment 3 by kinuko@chromium.org, Oct 13 2017

Description: Show this description

Comment 4 by kinuko@chromium.org, Oct 13 2017

(Rephrased the description to clarify things a bit)

Comment 5 by shimazu@google.com, Oct 19 2017

Owner: shimazu@chromium.org
Status: Started (was: Available)
Description: Show this description
I've roughly drawn figures and some explanation to show how to implement the timers. I'm now thinking we need timers (idle, event, ping-pong) as kinuko@ mentioned and also some mechanism for activation. 

If you have any thoughts or comments around timers, feel free to put your comments on the design doc:)

Updated the design doc to reflect the discussion we held offline. I'll start implementation of it based on that.

Comment 9 by laforge@google.com, Nov 7 2017

Components: Internals>Network>Service
Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
I've mostly implemented the idle timer and the long standing event timer. The remaining work is (1) SWContextClient ignores events from the renderer once it gets idle, and (2) the browser requests stopping to handle events afterwards to the renderer (for activation).
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 22 2017

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

commit 62316599fd6cb844fce8a814ce2b15bcf3b3851f
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Nov 22 09:30:23 2017

S13nServiceWorker: move idle timer to the renderer

After S13nServiceWorker, clients can directly fire events without the browser
process, so we need to move some codes which tracks events to the renderer. This
patch is to implement the idle timer on the renderer.

In this patch,
- SWContextClient tracks the number of inflight events by using SWEventTimer
  which is added by this CL
- SWContextClient::OnIdle() is called when 30 seconds passed after the end of
  the last event, and it requests termination to the corresponding EWInstance
- EWInstance stops the worker, or do nothing if there are inflight events which
  are dispatched just before RequestTermination.

Even after moving the idle timer to the renderer, the browser still has the
primarily control of the lifecycle of the worker thread. If the worker thread is
unresponsive, the browser will detect it by a ping-pong timer and terminate the
worker thread.

Bug:  774374 
Change-Id: I8d9cbb60cbf7583be3eb12a88860f74bbdcc949f
Reviewed-on: https://chromium-review.googlesource.com/758314
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518585}
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/embedded_worker_registry.cc
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/embedded_worker_registry.h
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/renderer/BUILD.gn
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/renderer/service_worker/service_worker_context_client.h
[add] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/renderer/service_worker/service_worker_event_timer.cc
[add] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/renderer/service_worker/service_worker_event_timer.h
[add] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/renderer/service_worker/service_worker_event_timer_unittest.cc
[modify] https://crrev.com/62316599fd6cb844fce8a814ce2b15bcf3b3851f/content/test/BUILD.gn

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 28 2017

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

commit 172fdb463cf290b6bd13ea47e935baff0285ce63
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Tue Nov 28 06:39:48 2017

S13nServiceWorker: Implement a timer for long standing events

This CL implements long standing event timer on the renderer. Events dispatched
to the service worker will be aborted in ServiceWorkerEventTimer::kEventTimeout.
ServiceWorkerEventTimer needs to track each event individually, so the event
timer issues an unique id to each event. For that reason, all of IDMaps used for
keeping the event callbacks are replaced to std::map.

Design doc: bit.ly/sw-timers-on-renderer

Bug:  774374 
Change-Id: I77f8c9cd01e1e02ff856306c42de298eebf318ca
Reviewed-on: https://chromium-review.googlesource.com/760083
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519593}
[modify] https://crrev.com/172fdb463cf290b6bd13ea47e935baff0285ce63/content/renderer/BUILD.gn
[modify] https://crrev.com/172fdb463cf290b6bd13ea47e935baff0285ce63/content/renderer/service_worker/service_worker_context_client.cc
[delete] https://crrev.com/794bf2c3112a2cdcecbbc6235ff3945e5c96536b/content/renderer/service_worker/service_worker_event_timer.cc
[delete] https://crrev.com/794bf2c3112a2cdcecbbc6235ff3945e5c96536b/content/renderer/service_worker/service_worker_event_timer.h
[delete] https://crrev.com/794bf2c3112a2cdcecbbc6235ff3945e5c96536b/content/renderer/service_worker/service_worker_event_timer_unittest.cc
[add] https://crrev.com/172fdb463cf290b6bd13ea47e935baff0285ce63/content/renderer/service_worker/service_worker_timeout_timer.cc
[add] https://crrev.com/172fdb463cf290b6bd13ea47e935baff0285ce63/content/renderer/service_worker/service_worker_timeout_timer.h
[add] https://crrev.com/172fdb463cf290b6bd13ea47e935baff0285ce63/content/renderer/service_worker/service_worker_timeout_timer_unittest.cc
[modify] https://crrev.com/172fdb463cf290b6bd13ea47e935baff0285ce63/content/test/BUILD.gn

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 7 2017

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

commit 21fa2d816260d0bf5de7a225d330276a560918cf
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Thu Dec 07 05:51:05 2017

Add an unit test for ServiceWorkerContextClient

SWContextClient is getting complicated rather than just passing through the
events, especially after adding renderer-side timers. This CL is for adding unit
tests for SWContextClient.

Bug:  774374 
Change-Id: Iaaf3944d0a11d812c9f9f3e2f9bfe2f334fcba19
Reviewed-on: https://chromium-review.googlesource.com/799518
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522355}
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_context_client.h
[add] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_context_client_unittest.cc
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_dispatcher.cc
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_dispatcher.h
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_network_provider.cc
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/renderer/service_worker/service_worker_network_provider.h
[modify] https://crrev.com/21fa2d816260d0bf5de7a225d330276a560918cf/content/test/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 13 2017

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

commit 1968460e63a39b04478fe5358570551457c09013
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Dec 13 03:44:20 2017

S13nServiceWorker: postpone fetch events for subresources when idle

This patch implements a part of idle timer's logic. Fetch events coming through
mojom::ControllerServiceWorker after the worker has requested termination to the
browser should be queued until the worker receives the next event or StopWorker
message from the browser. If the worker receives the next event from the
browser, the queued events should run before the event from the browser,
otherwise the order of the events would be messed up. If the worker receives
StopWorker message instead, the worker shouldn't invoke the callback for the
fetch event, and silently should disconnect the pipe to the clients.

Bug:  774374 
Change-Id: I6f3c90405ff037fe07df0310e2234cb81cb2ead9
Reviewed-on: https://chromium-review.googlesource.com/816458
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523677}
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/controller_service_worker_impl.cc
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/service_worker_context_client_unittest.cc
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/service_worker_timeout_timer.cc
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/service_worker_timeout_timer.h
[modify] https://crrev.com/1968460e63a39b04478fe5358570551457c09013/content/renderer/service_worker/service_worker_timeout_timer_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 21 2017

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

commit 299ff4495f744dbee352fdece8bc863b07cde064
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Thu Dec 21 00:42:26 2017

S13nServiceWorker: Set the idle timeout to zero when activation

This patch adds a method to set the renderer-side idle timeout time to zero and
calls it when a waiting version is ready. After the idle timeout termination,
the browser can ensure there is no tasks to run on the worker thread at the
time, so it can safely switch the active version to the latest one.

Bug:  774374 
Change-Id: Ia964d543b972ac6dc003222ac61e379445f57404
Reviewed-on: https://chromium-review.googlesource.com/823862
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525535}
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/browser/service_worker/service_worker_registration.cc
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/browser/service_worker/service_worker_registration.h
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/common/service_worker/service_worker_event_dispatcher.mojom
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/renderer/service_worker/service_worker_timeout_timer.cc
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/renderer/service_worker/service_worker_timeout_timer.h
[modify] https://crrev.com/299ff4495f744dbee352fdece8bc863b07cde064/content/renderer/service_worker/service_worker_timeout_timer_unittest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Dec 22 2017

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

commit cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Fri Dec 22 10:19:46 2017

S13nServiceWorker: Call OnNoWork when all stream responses finish

https://crrev.com/c/823862 has tweaked the idle timer for activation, but it
still doesn't work if the worker doesn't have any inflight event but is
streaming response to the browser. In that case, ServiceWorkerVersion needs to
detect the end of streaming.
This patch is adding the detection mechanism. We can say the worker is
completely idle if
1. the worker has requested termination and
2. the worker has no streaming tasks and
3. the browser doesn't start any events after the termination request.

This patch adds 2 and 3.

Bug:  774374 
Change-Id: If940f34f24749ec3c05bd6b04a08f303592f729e
Reviewed-on: https://chromium-review.googlesource.com/838901
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525967}
[modify] https://crrev.com/cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae/content/browser/service_worker/service_worker_registration.cc
[modify] https://crrev.com/cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae/content/browser/service_worker/service_worker_url_loader_job_unittest.cc
[modify] https://crrev.com/cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/cad7fcc1c6659e192ac78d5c4f7dd18e21d4dfae/content/browser/service_worker/service_worker_version.h

Status: Fixed (was: Started)
Project Member

Comment 19 by bugdroid1@chromium.org, Jan 9 2018

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

commit dbc330c9c33f7fcce2154dd190dd3dca4b30fb04
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Tue Jan 09 10:39:37 2018

Move Mojo interfaces passed at StartWorker to EmbeddedWorkerStartParams

Previously mojom::EmbeddedWorkerInstanceClient::StartWorker had many arguments
for Mojo interfaces since EmbeddedWorkerStartParams was not Mojo struct. This
patch moves them into mojom::EmbeddedWorkerStartParams which is now Mojo struct
and can contain Mojo pipes.

Bug:  774374 
Change-Id: Ifea593203a6072c87fa6eb21f6a22ca5e3036271
Reviewed-on: https://chromium-review.googlesource.com/835934
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527956}
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/service_worker_context_unittest.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/service_worker_job_unittest.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/dbc330c9c33f7fcce2154dd190dd3dca4b30fb04/content/renderer/service_worker/embedded_worker_instance_client_impl.h

Great work! Is there any remaining work here? For example ServiceWorkerVersion::StartRequestWithCustomTimeout allows specifying non-default 
 timeout behavior and values, which IIRC the background sync feature needed. Do we need to reimplement this in the renderer?

Comment 21 by shimazu@google.com, Jan 10 2018

Status: Started (was: Fixed)
Oops, good catch! I've completely missed that...
I think that would be the final piece of the timers.
Project Member

Comment 22 by bugdroid1@chromium.org, Jan 18 2018

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

commit 25d4ea38b620941ca98a102e2f49b26e836c6480
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Thu Jan 18 09:48:22 2018

ServiceWorker: Remove custom timeout feature from fetch events

FetchEvent used to need the custom timeout for foreign fetch, but it has already
removed. This patch removes the custom timeout codes from SWFetchDispatcher and
some loading related stuff.

Bug:  774374 , 788604
Change-Id: Ifba12ad93f5572b10e0f75c60fcaec8e6fa954b1
Reviewed-on: https://chromium-review.googlesource.com/872514
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530103}
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_fetch_dispatcher.h
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_url_request_job.h
[modify] https://crrev.com/25d4ea38b620941ca98a102e2f49b26e836c6480/content/browser/service_worker/service_worker_url_request_job_unittest.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Jan 22 2018

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

commit a7a96e217861f61c0aa297a04447be39b43dce24
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Mon Jan 22 10:29:54 2018

S13nServiceWorker: Invoke idle timeout ASAP after an event timed out

In this CL, the renderer-side timeout timer tries to invoke the idle timeout as
soon as possible after an event timed out and was aborted. This is a replacement
of KILL_ON_TIMEOUT for the non-S13nServiceWorker path. Instead of killing the
worker immediately, this CL tries to terminate the worker gracefully. Once all
other events has finished and the worker gets into the idle state, the worker
will shut down soon.

Bug:  774374 
Change-Id: Iccf9da16c233fc820160e7aacdca48fce1cfc9ec
Reviewed-on: https://chromium-review.googlesource.com/877983
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530829}
[modify] https://crrev.com/a7a96e217861f61c0aa297a04447be39b43dce24/content/renderer/service_worker/service_worker_timeout_timer.cc
[modify] https://crrev.com/a7a96e217861f61c0aa297a04447be39b43dce24/content/renderer/service_worker/service_worker_timeout_timer.h
[modify] https://crrev.com/a7a96e217861f61c0aa297a04447be39b43dce24/content/renderer/service_worker/service_worker_timeout_timer_unittest.cc

One thing I just remembered is extensions keep SW alive with ServiceWorkerVersion::StartExternalRequest. I think we might need to support this in S13nSW also but currently the renderer side idle timeout is probably not taking it into account?
Re c#24:
Thanks, I wasn't aware of that! 
I think we've already implemented correctly since StartExternalRequest() adds an InflightRequest entry, and the stop worker request from the renderer will see it when the message is received on the browser.
Project Member

Comment 26 by bugdroid1@chromium.org, Jan 24 2018

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

commit 3fde1638ee934741043e9faaf2fe847b48bb0ef1
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Jan 24 07:37:43 2018

S13nServiceWorker: Move custom timeout to renderer

Background fetch and push API set custom expiration time. This CL implements the
customization feature to the renderer side timer.

After this patch, the last piece of the renderer timeout timer is the
replacement of KILL_ON_TIMEOUT. It'll be implemented by crrev.com/c/877983.

Bug:  774374 
Change-Id: I700de3aa5661f39ef3b745b8348fa20ccbf46f66
Reviewed-on: https://chromium-review.googlesource.com/872770
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531475}
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/browser/background_sync/background_sync_manager.cc
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/browser/push_messaging/push_messaging_router.cc
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/common/service_worker/service_worker_event_dispatcher.mojom
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/renderer/service_worker/service_worker_timeout_timer.cc
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/renderer/service_worker/service_worker_timeout_timer.h
[modify] https://crrev.com/3fde1638ee934741043e9faaf2fe847b48bb0ef1/content/renderer/service_worker/service_worker_timeout_timer_unittest.cc

Labels: M-66
Status: Fixed (was: Started)
I hope all movement of timers has done. Updated the design doc too.

Sign in to add a comment