Stop Service Workers that execute JavaScript for too long.

Stop Service Workers that execute JavaScript for too long.

The motivation behind this change is to:
- not block the registration queue on a "while(true) {}" worker
- mitigate SW's from eating too much CPU/battery without being stopped

This adds a "ping" protocol from the browser to the worker thread. If the
worker thread is busy executing JS it will fail to ack the ping.

Each ping waits 30 seconds for a pong. If a pong is received, the next
ping is sent after 10 seconds. Otherwise, the worker is stopped.

This only stops workers that are busy executing synchronous JS. If the
worker does something like repeatedly extend a Promise chain, it will still
be able to ACK and live on. Remaining work is to set a time limit for an
inflight request.

Depends on https://codereview.chromium.org/930213002/

BUG=445150, 372436
Committed: https://crrev.com/b7f2b2f58a139038a970586c32b078b258519c5e
Cr-Commit-Position: refs/heads/master@{#317745}

[falken, 2015-02-10 08:22:37]

falken@chromium.org changed reviewers:
+ kinuko@chromium.org, ksakamoto@chromium.org

[falken, 2015-02-10 08:22:38]

PTL

[kinuko, 2015-02-10 09:15:52]

Looks good. Have you tried running this with actual SW? Did it work as expected
(no crashes)?

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_test_helper.h:114: int
next_thread_id_;
nit: could we have a simple method like GetNextThreadID() and use that
throughout the code?

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1416:
StopWorkerIfIdle();
In this case we should probably show some error or warning to the developer.
Should we file a bug and add a comment for that?

[michaeln, 2015-02-11 01:02:20]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2015-02-11 01:02:21]

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:819:
SchedulePingWorker();
Would it make sense to only turn the ping monitor on when HasInflightRequests()?
Might help avoid context switches and lighting up otherwise idle processes.

[falken, 2015-02-11 16:10:24]

Just replying to comments for now.

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:819:
SchedulePingWorker();
On 2015/02/11 01:02:21, michaeln wrote:
> Would it make sense to only turn the ping monitor on when
HasInflightRequests()?
> Might help avoid context switches and lighting up otherwise idle processes.

That's a good thing to think about. In practice, ping will essentially only
happen in that case anyway since the stop_worker_timer_ will kill a worker that
doesn't have in-flight requests. Additionally, we want ping to kill a worker
that's taking too long to startup, which doesn't currently count as an in-flight
request.

So I'm thinking it should be OK as coded. It might be possible to fold the stop
timer and ping timer into one, or have more explicit coupling between the two,
but they are kind of two different forces: the stop timer tries to kill "idle"
workers and ping tries to kill "busy" ones.

https://codereview.chromium.org/912753002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1416:
StopWorkerIfIdle();
On 2015/02/10 09:15:52, kinuko wrote:
> In this case we should probably show some error or warning to the developer.
> Should we file a bug and add a comment for that?

Yes I agree. I don't know a good place we'd show the error... maybe SW DevTools,
but if that's open we won't kill the worker.

[falken, 2015-02-12 10:30:43]

Status update: I tried testing some while(true) {} behavior, and sometimes we
never return from V8 even after TerminateExecution() is called. Interestingly, I
only see this issue for something like "while(true) { console('hello'); }". A
bare while(true) {} is reliably terminated.

The worker thread gets stuck in an infinite loop of 
blink::ConsoleBaseV8Internal::logMethodCallback so evidently JS keeps on
executing.

[falken, 2015-02-17 09:22:39]

Updated. I switched to browsertest as the unittest didn't really capture the
thread termination stuff.

It's slightly risky, so I plan to hold off on landing until after branch cut.

[kinuko, 2015-02-18 10:06:55]

On 2015/02/17 09:22:39, falken wrote:
> Updated. I switched to browsertest as the unittest didn't really capture the
> thread termination stuff.
> 
> It's slightly risky, so I plan to hold off on landing until after branch cut.

ServiceWorkerVersionBrowserTest.TimeoutWorkerInEvent seems failing?

[falken, 2015-02-18 10:10:34]

On 2015/02/18 10:06:55, kinuko wrote:
> On 2015/02/17 09:22:39, falken wrote:
> > Updated. I switched to browsertest as the unittest didn't really capture the
> > thread termination stuff.
> > 
> > It's slightly risky, so I plan to hold off on landing until after branch
cut.
> 
> ServiceWorkerVersionBrowserTest.TimeoutWorkerInEvent seems failing?

Yes, it needs https://codereview.chromium.org/930213002/

[kinuko, 2015-02-18 10:15:03]

On 2015/02/18 10:10:34, falken wrote:
> On 2015/02/18 10:06:55, kinuko wrote:
> > On 2015/02/17 09:22:39, falken wrote:
> > > Updated. I switched to browsertest as the unittest didn't really capture
the
> > > thread termination stuff.
> > > 
> > > It's slightly risky, so I plan to hold off on landing until after branch
> cut.
> > 
> > ServiceWorkerVersionBrowserTest.TimeoutWorkerInEvent seems failing?
> 
> Yes, it needs https://codereview.chromium.org/930213002/

Ah ok, of course.

[kinuko, 2015-02-18 13:06:25]

lgtm modulo nits

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:81:
base::MessageLoopProxy::current()));
nit: this could just call RunOnIOThreadWithDelay with null base::TimeDelta

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:91: const int
kPingTimeoutTime = 30;  // 30 secs.
nit: can you include these numbers in the CL description?

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1400: OnPingTimeout();
Call this here feels a bit weird to me, especially if we start to show a notice
message in the method?

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:399: void PingWorker();
Can we have a short comment about the ping protocol here (or somewhere in the
.cc file) too?

[falken, 2015-02-23 03:12:28]

New patchsets have been uploaded after l-g-t-m from kinuko@chromium.org

[falken, 2015-02-23 05:00:07]

Thanks!

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:81:
base::MessageLoopProxy::current()));
On 2015/02/18 13:06:24, kinuko wrote:
> nit: this could just call RunOnIOThreadWithDelay with null base::TimeDelta

Done.

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:91: const int
kPingTimeoutTime = 30;  // 30 secs.
On 2015/02/18 13:06:25, kinuko wrote:
> nit: can you include these numbers in the CL description?

Done.

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1400: OnPingTimeout();
On 2015/02/18 13:06:25, kinuko wrote:
> Call this here feels a bit weird to me, especially if we start to show a
notice
> message in the method?

Hmm, actually what do you think is best to do here? If we can't send Ping we
probably can't send Stop. Generally we may need StopWorkerIfIdle to handle
failure to send Stop and forcibly fail the callbacks so no one gets stuck
waiting for the worker.

I've added a TODO here and changed this to call StopWorkerIfIdle directly.

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/912753002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:399: void PingWorker();
On 2015/02/18 13:06:25, kinuko wrote:
> Can we have a short comment about the ping protocol here (or somewhere in the
> .cc file) too?

Good idea. Done. I also added a short comment for ScheduleStartWorker as it
could be confusing what the two timers are doing.

[falken, 2015-02-23 05:00:23]

falken@chromium.org changed reviewers:
+ tsepez@chromium.org

[falken, 2015-02-23 05:00:24]

+tsepez: messages

[Tom Sepez, 2015-02-23 17:49:28]

Messages LGTM.

[falken, 2015-02-24 01:36:13]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2015-02-24 01:37:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/912753002/120001

[commit-bot: I haz the power, 2015-02-24 02:42:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-24 02:43:07]

Patchset 7 (id:??) landed as
https://crrev.com/b7f2b2f58a139038a970586c32b078b258519c5e
Cr-Commit-Position: refs/heads/master@{#317745}