New issue
Advanced search Search tips

Issue 852658 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: 2019-01-28
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 678798
issue 715640
issue 762357



Sign in to add a comment

Make a single event timeout mechanism

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

Issue description

As far as my code reading shows, if S13nSW/NetworkService is on, the "5 minute" event timeout happens in both the browser and renderer:

1. ServiceWorkerVersion's timeout timer
2. ServiceWorkerTimeoutTimer

The error codes are different on timeout. SWV will give the client a TIMEOUT while timeout timer gives an ABORT.

I tend to think TIMEOUT is better but we can revisit that separately.

We should at least have a consistent error code and avoid complexity. Probably we can  skip request expiration on the browser process. It's a bit scary because it means if there's a bug somehow that causes the renderer to not timeout the event, we will never terminate the service worker. We could potentially keep some big hammer, last resort mechanism on the browser (something like if an event's been running 10 minutes, just terminate). Not sure it's worth it though.

Doesn't block canary.

 

Comment 1 by falken@chromium.org, Jun 14 2018

Blocking: 762357
This will also affect UMA collection.

Comment 2 by falken@chromium.org, Jun 14 2018

Design doc and details are on  issue 774374 .

Comment 3 by dxie@google.com, Jun 19 2018

Labels: Hotlist-KnownIssue
We should also clarify the KILL_ON_TIMEOUT vs CONTINUE_ON_TIMEOUT enum. As far as I can tell, the renderer-side TimeoutTimer always does CONTINUE_ON_TIMEOUT behavior. But the browser-side looks at the enum value. 
Owner: shimazu@chromium.org
Status: Assigned (was: Available)
Tentatively assigning to shimazu@, would be good to clean this up. There's also two places you need to add a custom timeout value for now, both in the browser and renderer.
Labels: Target-73
NextAction: 2018-11-30
A simple way to unify them is self-terminating the worker in abort callbacks created in each DispatchXXXEvent().
I'm making a CL (https://crrev.com/c/1350388) but found that it'll make the timeout logic very complicated if we want to keep non-S13nSW path. Given that M72 branch cut is coming soon, I'll revisit this after fully shipping S13nSW.
The NextAction date has arrived: 2018-11-30
Labels: -Target-73 Target-74
NextAction: 2019-01-28
Blocking: 678798

Sign in to add a comment