Terminate plugin processes on a delayed task to allow the plugins to shutdown gracefully, i.e. NP...

Don't terminate plugin processes from the browser during browser shutdown. This is to allow the plugins to
shutdown gracefully, i.e. NP_Shutdown gets called. To ensure that we handle the case of a hung plugin, we handle
the OnChannelError notification in the IPC message filter implementation in the plugin process and post a delayed 
task to kill the process.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=48178

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

[jam, 2011-05-23 22:32:59]

http://codereview.chromium.org/6992006/diff/1/content/browser/child_process_l...
File content/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/6992006/diff/1/content/browser/child_process_l...
content/browser/child_process_launcher.cc:219: if (process_type_ ==
switches::kPluginProcess) {
ChildProcessLauncher is the base class that's used by all child processes.  It
seems out of place to add this logic to it, which is specific to plugins.

Either way, what happens if we post a task to kill the plugin, and the browser
shuts before the task runs?  Then we can end up with a zombie plugin process,
which would be very bad (i.e. wouldn't allow auto-update etc).

[jam, 2011-05-24 20:46:13]

lgtm

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
File content/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:92: // shutdown. Default behavior is
to terminate the child..
nit: one period at end.  also, comments usually go by the variable, and simple
setters/getters are left without a comment

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:220: if
(!terminate_child_on_shutdown_) {
nit: this file doesn't use brace brackets for one line statements

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:351: if (context_) {
ditto

http://codereview.chromium.org/6992006/diff/12/content/browser/plugin_process...
File content/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/6992006/diff/12/content/browser/plugin_process...
content/browser/plugin_process_host.cc:224: SetTerminateChildOnShutdown(false);
nit: please add a comment explaining why this is ok

http://codereview.chromium.org/6992006/diff/12/content/plugin/plugin_channel.cc
File content/plugin/plugin_channel.cc (right):

http://codereview.chromium.org/6992006/diff/12/content/plugin/plugin_channel....
content/plugin/plugin_channel.cc:97: 
nit: extra line

http://codereview.chromium.org/6992006/diff/12/content/plugin/plugin_channel....
content/plugin/plugin_channel.cc:103: // Ensure that we don't wait indefinitely
for the plugin to shutdown.
nit: can you please add just one extra line to say why we're doing this, since
it may not be obvious to someone reading this.  i.e.


// Ensure that we don't wait indefinitely for the plugin to shutdown, since the
browser doesn't kill plugin processes on shutdown to allow plugins to be
gracefully shutdown.

[ananta, 2011-05-24 20:56:26]

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
File content/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:92: // shutdown. Default behavior is
to terminate the child..
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> nit: one period at end.  also, comments usually go by the variable, and simple
> setters/getters are left without a comment

Done.

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:220: if
(!terminate_child_on_shutdown_) {
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> nit: this file doesn't use brace brackets for one line statements

Done.

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:220: if
(!terminate_child_on_shutdown_) {
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> nit: this file doesn't use brace brackets for one line statements

Done.

http://codereview.chromium.org/6992006/diff/12/content/browser/child_process_...
content/browser/child_process_launcher.cc:351: if (context_) {
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/6992006/diff/12/content/browser/plugin_process...
File content/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/6992006/diff/12/content/browser/plugin_process...
content/browser/plugin_process_host.cc:224: SetTerminateChildOnShutdown(false);
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> nit: please add a comment explaining why this is ok

Done.

http://codereview.chromium.org/6992006/diff/12/content/plugin/plugin_channel.cc
File content/plugin/plugin_channel.cc (right):

http://codereview.chromium.org/6992006/diff/12/content/plugin/plugin_channel....
content/plugin/plugin_channel.cc:97: 
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/6992006/diff/12/content/plugin/plugin_channel....
content/plugin/plugin_channel.cc:103: // Ensure that we don't wait indefinitely
for the plugin to shutdown.
On 2011/05/24 20:46:13, John Abd-El-Malek wrote:
> nit: can you please add just one extra line to say why we're doing this, since
> it may not be obvious to someone reading this.  i.e.
> 
> 
> // Ensure that we don't wait indefinitely for the plugin to shutdown, since
the
> browser doesn't kill plugin processes on shutdown to allow plugins to be
> gracefully shutdown.

Done.