Fix the GC of workers. When Worker object is GC'ed in the renderer, we need t...

Fix the GC of workers. When Worker object is GC'ed in the renderer, we need to terminate the worker process.
BUG=15647, 15759
TEST=new ui_test is part of this CL

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20034

[Dmitry Titov, 2009-07-01 00:08:49]

We already had code that terminated the worker process as response to a direct
'terminate' request coming from the Worker object in the renderer, but the
milder 'workerObjectDestroyed()' was not implemented. Terminating WorkerContext
in responce to that exposed the fact that WebWorkerProxy did not removed the
listener after deleting itself. Also, made the "child process crashed" bit
exposed so we can clearly differentiate when to show a infobar about worker
death.
I'll work on ui_test later, wanted to get this up to review and trybot.

[jam, 2009-07-01 02:24:09]

Actually ~WebWorkerProxy is called, the problem is that terminateWorkerContext()
had already set route_id_ to MSG_ROUTING_NONE which means the cancel queued
worker message doesn't get sent.  The fix should be a one-liner:
http://codereview.chromium.org/150152

On 2009/07/01 00:08:49, Dmitry Titov wrote:
> We already had code that terminated the worker process as response to a direct
> 'terminate' request coming from the Worker object in the renderer, but the
> milder 'workerObjectDestroyed()' was not implemented. Terminating
WorkerContext
> in responce to that exposed the fact that WebWorkerProxy did not removed the
> listener after deleting itself. Also, made the "child process crashed" bit
> exposed so we can clearly differentiate when to show a infobar about worker
> death.
> I'll work on ui_test later, wanted to get this up to review and trybot.

[Dmitry Titov, 2009-07-01 02:50:18]

I am not sure fix for what is that 1-liner...
The ~WebWorkerProxy is get called, that just doesn't terminate the worker
process. 
In addition, 'terminate' is not called in the case when Worker object is
destroyed not by calling 'worker.terminate()' or lets say refresh - but when
regular GC happens. 
And this is correct - we need to preserve the distinction because in case GC
happens (but no 'terminate') we need to be using 'kill a worker' algorithm
rather then 'terminate a worker' one. In the future, we'll do just that.

So there are 2 ways for ~WebWorkerProxy to be called - via  'terminate' path and
without it. In second case, the dying WorkerContext fires back a
'WorkerContextDestroyed" which, if route_it_ is not removed (remember
'terminate' was not called and this is right) - will try to find a listener in
the renderer process for the route_id that was not removed, finds deleted object
and tries to use it. This is the reason to have that Disconnect() method - to do
both CancelCreate and RemoveRoute in both cases, and to do it only once.

Other changes are also needed - the worker process does not terminate in GC case
because workerObjectDestroyed is not implemented there (empty). It should call
'terminate' (in the future - 'kill'). The 'did_crash()' check is needed because
worker process may very well terminate while renderer is running and it is
incorrect to always indicate it's a crash.

Please read beyond the related bug. The root issue is that worker process is not
terminated when Worker object is GCed.

[Dmitry Titov, 2009-07-01 18:08:26]

Please use the review tool for comments.

Added the second bug reference, with a GC-related repro.

Also addded a check to not send the ViewHostMsg_CancelCreateDedicatedWorker
message if not needed in WebWorkerProxy::Disconnect().

[Dmitry Titov, 2009-07-01 22:48:37]

Uploaded updated version.

Works fine as I can tell.

Lets keep replies in this tool if possible, sometimes I myself look at the
issues here to find both what changed and a discussion abut it.

[jam, 2009-07-02 01:22:57]

http://codereview.chromium.org/151125/diff/49/53
File chrome/renderer/webworker_proxy.cc (right):

http://codereview.chromium.org/151125/diff/49/53#newcode30
Line 30: if (queued_messages_.empty())
this check isn't needed anymore

http://codereview.chromium.org/151125/diff/49/53#newcode38
Line 38: if (route_id_ != MSG_ROUTING_NONE) {
nit: we usually try to early return when we can to avoid tabbing

http://codereview.chromium.org/151125/diff/49/50
File webkit/glue/webworker_impl.cc (right):

http://codereview.chromium.org/151125/diff/49/50#newcode190
Line 190: asked_to_terminate_ = true;
is this really needed?  WorkerThread::stop looks like it's safe to call multiple
times.

[Dmitry Titov, 2009-07-02 01:39:42]

On 2009/07/02 01:22:57, John Abd-El-Malek wrote:
> http://codereview.chromium.org/151125/diff/49/53
> File chrome/renderer/webworker_proxy.cc (right):
> 
> http://codereview.chromium.org/151125/diff/49/53#newcode30
> Line 30: if (queued_messages_.empty())
> this check isn't needed anymore

Done.

> http://codereview.chromium.org/151125/diff/49/53#newcode38
> Line 38: if (route_id_ != MSG_ROUTING_NONE) {
> nit: we usually try to early return when we can to avoid tabbing

Done.

> File webkit/glue/webworker_impl.cc (right):
> 
> http://codereview.chromium.org/151125/diff/49/50#newcode190
> Line 190: asked_to_terminate_ = true;
> is this really needed?  WorkerThread::stop looks like it's safe to call
multiple
> times.

Seems a bit safer, although has a bool. I'll sleep better at night :-)

[jam, 2009-07-02 01:57:25]

lgtm

[jam, 2009-07-02 02:43:37]

On 2009/07/02 01:57:25, John Abd-El-Malek wrote:
> lgtm

btw, given that we've regressed on this bug once already, I think a UI test
should be added (should be easy to do given the test case you had).  Doesn't
have to be in this changelist, but if not, then a bug should be filed to track
it.