Revert 86517 - Don't terminate plugin processes from the browser during browser shutdown. This is...

Revert 86517 - 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
Review URL: http://codereview.chromium.org/6992006

TBR=ananta@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86532

[Nicolas Sylvain, 2011-05-25 01:36:20]

this did not help, feel free to reland

sorry for the trouble

Nicolas

On Tue, May 24, 2011 at 5:19 PM, <nsylvain@chromium.org> wrote:

> Reviewers: ananta,
>
> Description:
> Revert 86517 - 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
> Review URL: http://codereview.chromium.org/6992006
>
> TBR=ananta@chromium.org
>
> Please review this at http://codereview.chromium.org/7065048/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     content/browser/browser_child_process_host.h
>  M     content/browser/browser_child_process_host.cc
>  M     content/browser/child_process_launcher.h
>  M     content/browser/child_process_launcher.cc
>  M     content/browser/plugin_process_host.cc
>  M     content/plugin/plugin_channel.cc
>
>
> Index: content/browser/browser_child_process_host.cc
> ===================================================================
> --- content/browser/browser_child_process_host.cc       (revision 86530)
> +++ content/browser/browser_child_process_host.cc       (working copy)
> @@ -109,11 +109,6 @@
>   ChildProcessHost::ForceShutdown();
>  }
>
> -void BrowserChildProcessHost::SetTerminateChildOnShutdown(
> -  bool terminate_on_shutdown) {
> -  child_process_->SetTerminateChildOnShutdown(terminate_on_shutdown);
> -}
> -
>  void BrowserChildProcessHost::Notify(NotificationType type) {
>   BrowserThread::PostTask(
>       BrowserThread::UI, FROM_HERE, new ChildNotificationTask(type, this));
> Index: content/browser/browser_child_process_host.h
> ===================================================================
> --- content/browser/browser_child_process_host.h        (revision 86530)
> +++ content/browser/browser_child_process_host.h        (working copy)
> @@ -92,10 +92,6 @@
>   // the host list. Calls ChildProcessHost::ForceShutdown
>   virtual void ForceShutdown();
>
> -  // Controls whether the child process should be terminated on browser
> -  // shutdown. Default is to always terminate.
> -  void SetTerminateChildOnShutdown(bool terminate_on_shutdown);
> -
>  private:
>   // By using an internal class as the ChildProcessLauncher::Client, we can
>   // intercept OnProcessLaunched and do our own processing before
> Index: content/browser/child_process_launcher.cc
> ===================================================================
> --- content/browser/child_process_launcher.cc   (revision 86530)
> +++ content/browser/child_process_launcher.cc   (working copy)
> @@ -44,8 +44,7 @@
>   Context()
>       : client_(NULL),
>         client_thread_id_(BrowserThread::UI),
> -        starting_(true),
> -        terminate_child_on_shutdown_(true)
> +        starting_(true)
>  #if defined(OS_LINUX)
>         , zygote_(false)
>  #endif
> @@ -88,10 +87,6 @@
>     client_ = NULL;
>   }
>
> -  void set_terminate_child_on_shutdown(bool terminate_on_shutdown) {
> -    terminate_child_on_shutdown_ = terminate_on_shutdown;
> -  }
> -
>  private:
>   friend class base::RefCountedThreadSafe<ChildProcessLauncher::Context>;
>   friend class ChildProcessLauncher;
> @@ -215,9 +210,6 @@
>     if (!process_.handle())
>       return;
>
> -    if (!terminate_child_on_shutdown_)
> -      return;
> -
>     // On Posix, EnsureProcessTerminated can lead to 2 seconds of sleep!
>  So
>     // don't this on the UI/IO threads.
>     BrowserThread::PostTask(
> @@ -265,9 +257,6 @@
>   BrowserThread::ID client_thread_id_;
>   base::Process process_;
>   bool starting_;
> -  // Controls whether the child process should be terminated on browser
> -  // shutdown. Default behavior is to terminate the child.
> -  bool terminate_child_on_shutdown_;
>
>  #if defined(OS_LINUX)
>   bool zygote_;
> @@ -344,10 +333,3 @@
>           &ChildProcessLauncher::Context::SetProcessBackgrounded,
>           background));
>  }
> -
> -void ChildProcessLauncher::SetTerminateChildOnShutdown(
> -  bool terminate_on_shutdown) {
> -  if (context_)
> -    context_->set_terminate_child_on_shutdown(terminate_on_shutdown);
> -}
> -
> Index: content/browser/child_process_launcher.h
> ===================================================================
> --- content/browser/child_process_launcher.h    (revision 86530)
> +++ content/browser/child_process_launcher.h    (working copy)
> @@ -60,10 +60,6 @@
>   // this after the process has started.
>   void SetProcessBackgrounded(bool background);
>
> -  // Controls whether the child process should be terminated on browser
> -  // shutdown.
> -  void SetTerminateChildOnShutdown(bool terminate_on_shutdown);
> -
>  private:
>   class Context;
>
> Index: content/browser/plugin_process_host.cc
> ===================================================================
> --- content/browser/plugin_process_host.cc      (revision 86530)
> +++ content/browser/plugin_process_host.cc      (working copy)
> @@ -221,12 +221,6 @@
>  #endif
>       cmd_line);
>
> -  // The plugin needs to be shutdown gracefully, i.e. NP_Shutdown needs to
> be
> -  // called on the plugin. The plugin process exits when it receives the
> -  // OnChannelError notification indicating that the browser plugin
> channel has
> -  // been destroyed.
> -  SetTerminateChildOnShutdown(false);
> -
>   content::GetContentClient()->browser()->PluginProcessHostCreated(this);
>   AddFilter(new ResolveProxyMsgHelper(NULL));
>
> Index: content/plugin/plugin_channel.cc
> ===================================================================
> --- content/plugin/plugin_channel.cc    (revision 86530)
> +++ content/plugin/plugin_channel.cc    (working copy)
> @@ -32,19 +32,9 @@
>   }
>  };
>
> -class PluginProcessExitTask : public Task {
> - public:
> -  void Run() {
> -    base::KillProcess(base::GetCurrentProcessHandle(), 0, false);
> -  }
> -};
> -
>  // How long we wait before releasing the plugin process.
>  const int kPluginReleaseTimeMs = 5 * 60 * 1000;  // 5 minutes
>
> -// How long we wait before forcibly shutting down the process.
> -const int kPluginProcessTerminateTimeoutMs = 3000;
> -
>  }  // namespace
>
>  // If a sync call to the renderer results in a modal dialog, we need to
> have a
> @@ -98,15 +88,6 @@
>  private:
>   void OnFilterAdded(IPC::Channel* channel) { channel_ = channel; }
>
> -  virtual void OnChannelError() {
> -    // Ensure that we don't wait indefinitely for the plugin to shutdown.
> -    // as the browser does not terminate plugin processes on shutdown.
> -    // We achieve this by posting an exit process task on the IO thread.
> -    MessageLoop::current()->PostDelayedTask(FROM_HERE,
> -                                            new PluginProcessExitTask(),
> -
>  kPluginProcessTerminateTimeoutMs);
> -  }
> -
>   bool OnMessageReceived(const IPC::Message& message) {
>     IPC_BEGIN_MESSAGE_MAP(PluginChannel::MessageFilter, message)
>       IPC_MESSAGE_HANDLER_DELAY_REPLY(PluginMsg_Init, OnInit)
>
>
>