Stop ServiceWorker context when no controllee is associated (and when all refs are dropped)

Stop ServiceWorker context when no controllee is associated (and when all refs are dropped)

Currently we stop the ServiceWorker thread/context when:
1. After all documents that are controlled by the SW are closed, with a delay (5 min)
2. All references to the version are dropped (i.e. in dtor)

In the current impl/codebase a worker is immediately stopped by 2
in most cases.

The same stop timer (ScheduleStopWorker()) could be probably called
each time handling event is finished to stop the worker more 
aggressively, though this patch doesn't do so.

BUG=372673
TEST=tested manually with chrome://serviceworker-internals/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270971

[kinuko, 2014-05-14 09:20:31]

PTL, adding a few very basic callsites to stop workers. (You could wonder why we
hadn't called StopWorker at all other than from sw-internals until now)

[horo, 2014-05-14 11:05:02]

lgtm.

But to fix the bug http://crbug.com/368570 we should also call StopWorker when
RenderProcessHostImpl::FastShutdownIfPossible is called.

[michaeln, 2014-05-14 19:44:23]

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:193:
DCHECK(embedded_worker_);
why remove these?

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:116:
embedded_worker_->Stop();
What happens when a running worker instance is deleted without having been
stopped? Seems like that should cause it to stop if its running.

[kinuko, 2014-05-14 21:56:29]

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:193:
DCHECK(embedded_worker_);
On 2014/05/14 19:44:23, michaeln wrote:
> why remove these?

Since we no longer reset embedded_worker_ anywhere other than in dtor.. (these
were added when we had Shutdown())).

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:116:
embedded_worker_->Stop();
On 2014/05/14 19:44:23, michaeln wrote:
> What happens when a running worker instance is deleted without having been
> stopped? Seems like that should cause it to stop if its running.

Yes, and this CL does so (stops the worker if it's running).

[kinuko, 2014-05-14 22:00:25]

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:116:
embedded_worker_->Stop();
On 2014/05/14 21:56:30, kinuko wrote:
> On 2014/05/14 19:44:23, michaeln wrote:
> > What happens when a running worker instance is deleted without having been
> > stopped? Seems like that should cause it to stop if its running.
> 
> Yes, and this CL does so (stops the worker if it's running).

(I could change this to call StopWorker() if it's clearer)

[michaeln, 2014-05-14 23:20:21]

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:193:
DCHECK(embedded_worker_);
> Since we no longer reset embedded_worker_ anywhere other than in dtor.. (these
> were added when we had Shutdown())).

k... got it

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:116:
embedded_worker_->Stop();
i was referring to EmbeddedWorkerInstance class, seems like the smarts to stop
the remote thread on destruction could/should be in
EmbeddedWorkerInstance::~EmbeddedWorkerInstance()

[kinuko, 2014-05-14 23:50:06]

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:116:
embedded_worker_->Stop();
On 2014/05/14 23:20:21, michaeln wrote:
> i was referring to EmbeddedWorkerInstance class, seems like the smarts to stop
> the remote thread on destruction could/should be in
> EmbeddedWorkerInstance::~EmbeddedWorkerInstance()

Hmm... ok will move the Stop part to EmbeddedWorkerInstance's dtor once I'm
online.

[kinuko, 2014-05-15 17:17:00]

> But to fix the bug http://crbug.com/368570 we should also call StopWorker when
> RenderProcessHostImpl::FastShutdownIfPossible is called.

Yes I kinda verified this doesn't fix http://crbug.com/368570.  (And therefore
BUG= line doesn't have it)

[kinuko, 2014-05-15 17:17:25]

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/272183005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:116:
embedded_worker_->Stop();
On 2014/05/14 23:20:21, michaeln wrote:
> i was referring to EmbeddedWorkerInstance class, seems like the smarts to stop
> the remote thread on destruction could/should be in
> EmbeddedWorkerInstance::~EmbeddedWorkerInstance()

Done.

[michaeln, 2014-05-15 19:02:20]

lgtm, please see the comment about possibly simplifiying the dtor

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

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:25: if (status_ ==
STARTING || status_ == RUNNING)
nice catch, STARTING too

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

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:115:
stop_worker_timer_.Stop();
base::Timer's destructor takes care of this

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:119:
embedded_worker_.reset();
is the call to .reset() needed for ordering reasons, if not might be nice to
remove it.

can this method could be simplified to...

ServiceWorkerVersion::~ServiceWorkerVersion() {
  embedded_worker_->RemoveListener(this);
  if (context_)
    context_->RemoveLiveVersion(version_id_);
}

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:583:
stop_worker_timer_.Reset();
nit: maybe prefer early return here

[Jeffrey Yasskin, 2014-05-15 19:16:44]

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

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:30: const int64
kStopWorkerDelay = 5 * 60;  // 5 mins.
We should probably make this faster. Extension/App event pages die after 15
seconds of no events, which indicates that a short-ish timeout works for some
people. The counter-indication is that extension/app authors can extend their
event page's lifetime by calling various callback APIs, which we don't want to
allow in SWs. Even so, we don't really want someone using 5 minutes of CPU after
their last tab is closed or they handle the most recent event.

We're also going to need to make this delay configurable from devtools.

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:340:
ScheduleStopWorker();
No need to handle this in this CL, but we're going to need non-controllee events
like 'push' and 'sync' to be able to keep a SW alive too.

[kinuko, 2014-05-16 04:13:51]

Thanks. We seem to have a few TODOs but let me land this now (so that we can put
up more patches on top of it)

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

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:30: const int64
kStopWorkerDelay = 5 * 60;  // 5 mins.
On 2014/05/15 19:16:44, Jeffrey Yasskin wrote:
> We should probably make this faster. Extension/App event pages die after 15
> seconds of no events, which indicates that a short-ish timeout works for some
> people. The counter-indication is that extension/app authors can extend their
> event page's lifetime by calling various callback APIs, which we don't want to
> allow in SWs. Even so, we don't really want someone using 5 minutes of CPU
after
> their last tab is closed or they handle the most recent event.

I don't have a strong opinion on this delay for now, let me change this to
default to 30 secs for now.

> We're also going to need to make this delay configurable from devtools.

Let us do this in a separate patch.

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:115:
stop_worker_timer_.Stop();
On 2014/05/15 19:02:20, michaeln wrote:
> base::Timer's destructor takes care of this

Oh yeah that's handy...

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:119:
embedded_worker_.reset();
On 2014/05/15 19:02:20, michaeln wrote:
> is the call to .reset() needed for ordering reasons, if not might be nice to
> remove it.
> 
> can this method could be simplified to...
> 
> ServiceWorkerVersion::~ServiceWorkerVersion() {
>   embedded_worker_->RemoveListener(this);
>   if (context_)
>     context_->RemoveLiveVersion(version_id_);
> }

Done.

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:340:
ScheduleStopWorker();
On 2014/05/15 19:16:44, Jeffrey Yasskin wrote:
> No need to handle this in this CL, but we're going to need non-controllee
events
> like 'push' and 'sync' to be able to keep a SW alive too.

Yeah we could add similar code in other events / upon completion of event
handlers too.

https://codereview.chromium.org/272183005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:583:
stop_worker_timer_.Reset();
On 2014/05/15 19:02:20, michaeln wrote:
> nit: maybe prefer early return here

Done.

[kinuko, 2014-05-16 04:14:02]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-16 04:14:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/272183005/40001

[commit-bot: I haz the power, 2014-05-16 07:28:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-16 09:16:21]

Change committed as 270971

[Adrian Kuegel, 2014-05-16 09:52:36]

A revert of this CL has been created in
https://codereview.chromium.org/286203008/ by akuegel@chromium.org.

The reason for reverting is: Has memory leaks in
EmbeddedWorkerInstanceTest.InstanceDestroyedBeforeStartFinishes.