Keep NaCl plugins used in app background pages alive when active.

content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc

Index: content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc
diff --git a/content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc b/content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc
index c3c2c6f39323a35caaf4f3a0da7cfab6e9a168b0..5d166483d1c2abd176a489641108d847a2ff4adf 100644
--- a/content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc
+++ b/content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc
@@ -8,13 +8,16 @@
 #include "content/browser/tracing/trace_message_filter.h"
 #include "content/common/pepper_renderer_instance_data.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/common/process_type.h"
 #include "ipc/ipc_message_macros.h"
+#include "ppapi/proxy/ppapi_messages.h"
 
 namespace content {
 
 // static
-BrowserPpapiHost* BrowserPpapiHost::CreateExternalPluginProcess(
+scoped_refptr<BrowserPpapiHost> BrowserPpapiHost::CreateExternalPluginProcess(
+    BrowserPpapiHost::Delegate* delegate,
     IPC::Sender* sender,
     ppapi::PpapiPermissions permissions,
     base::ProcessHandle plugin_child_process,
@@ -23,8 +26,8 @@ BrowserPpapiHost* BrowserPpapiHost::CreateExternalPluginProcess(
     int render_view_id,
     const base::FilePath& profile_directory) {
   // The plugin name and path shouldn't be needed for external plugins.
-  BrowserPpapiHostImpl* browser_ppapi_host =
-      new BrowserPpapiHostImpl(sender, permissions, std::string(),
+  scoped_refptr<BrowserPpapiHostImpl> browser_ppapi_host =
+      new BrowserPpapiHostImpl(delegate, sender, permissions, std::string(),
                                base::FilePath(), profile_directory,
                                false /* in_process */,
                                true /* external_plugin */);
@@ -40,6 +43,7 @@ BrowserPpapiHost* BrowserPpapiHost::CreateExternalPluginProcess(
 }
 
 BrowserPpapiHostImpl::BrowserPpapiHostImpl(
+    BrowserPpapiHost::Delegate* delegate,
     IPC::Sender* sender,
     const ppapi::PpapiPermissions& permissions,
     const std::string& plugin_name,
@@ -47,7 +51,8 @@ BrowserPpapiHostImpl::BrowserPpapiHostImpl(
     const base::FilePath& profile_data_directory,
     bool in_process,
     bool external_plugin)
-    : ppapi_host_(new ppapi::host::PpapiHost(sender, permissions)),
+    : delegate_(delegate),
+      ppapi_host_(new ppapi::host::PpapiHost(sender, permissions)),
       plugin_process_handle_(base::kNullProcessHandle),
       plugin_name_(plugin_name),
       plugin_path_(plugin_path),
@@ -55,7 +60,7 @@ BrowserPpapiHostImpl::BrowserPpapiHostImpl(
       in_process_(in_process),
       external_plugin_(external_plugin),
       ssl_context_helper_(new SSLContextHelper()) {
-  message_filter_ = new HostMessageFilter(ppapi_host_.get());
+  message_filter_ = new HostMessageFilter(this, ppapi_host_.get());

[dmichael (off chromium), 2013/11/06 22:39:21]

Maybe you should give the HostMessageFilter a callback bound to OnIdleState
instead, and it won't need the pointer to BrowserPpapiHostImpl?

I think you'd be able to use a WeakPtrFactory and bind the callback to a WeakPtr
instead of making BrowserPpapiHostImpl RefCountedThreadSafe. Since the callback
would only ever be invoked on the IO thread, that should be safe.

[scheib, 2013/11/15 01:22:17]

Done.

   ppapi_host_->AddHostFactoryFilter(scoped_ptr<ppapi::host::HostFactory>(
       new ContentBrowserPepperHostFactory(this)));
 }
@@ -142,26 +147,50 @@ void BrowserPpapiHostImpl::DeleteInstance(PP_Instance instance) {
   instance_map_.erase(found);
 }
 
+void BrowserPpapiHostImpl::OnIdleState(bool idle) {
+  if (!delegate_)
+    return;
+
+  for (InstanceMap::iterator i = instance_map_.begin();

[dmichael (off chromium), 2013/11/06 22:39:21]

Hmm...  it might be nice if this code was more instance-aware. As is, it should
be fine for your intended use case of NaCl (since there's 1 instance per
process), but it's a little bit weird if this code ran for trusted plugins.

Let's say 10 extensions embed some Pepper plugin, e.g. Flash (I'm assuming they
can do that?). There's 1 Flash process, but I presume 10 renderers (one per
extension). If just 1 of those Flash instances is active, does this mean that
we're treating all of the extensions as active, even if the rest are idle?

It seems like the "right" thing to do in that case would be to let the plugin
process live on (because 1 instance is active), but kill the 9 idle renderers.

I just looked at where you're calling MarkPluginAsIdle, and you *could* add a
PP_Instance argument there and plumb it through to here. The instance would
unfortunately not be available in all cases, however (PPB_Var, PPB_Core,
PPB_MessageLoop at least). You could just treat that as activity for all plugins
in the process, I guess.

Anyway...  this is all a bit hypothetical, since this is only being checked in
NaCl at present, where there's 1 PP_Instance per process anyway. So I'm leaning
towards recommending you do it basically how you have it, omitting PP_Instance.
But there should be a big fat comment here, probably...

[scheib, 2013/11/07 01:06:42]

Comment certainly warranted.  I'm not a fan of tracking and plumbing all the
instances through if there's really no need. But, I would also like to move this
loop to after the work that happens in chrome/browser. The blocker there is
mainly just building a set of all the info to push upwards without exposing
PepperRendererInstanceData outside of content.

[scheib, 2013/11/15 01:22:17]

I have moved the for loop to the handler in chrome/*, and added a comment here
acknowledging that idle is not tracked per instance.

+       i != instance_map_.end();
+       ++i) {
+    RenderViewHost* render_view_host = RenderViewHost::FromID(
+        i->second.render_process_id, i->second.render_view_id);
+
+    delegate_->OnIdleState(idle, profile_data_directory_, render_view_host,
+                           i->second.document_url);
+  }
+}
+
 bool BrowserPpapiHostImpl::HostMessageFilter::OnMessageReceived(
     const IPC::Message& msg) {
   // Don't forward messages if our owner object has been destroyed.
   if (!ppapi_host_)
     return false;
 
-  /* TODO(brettw) when we add messages, here, the code should look like this:
   bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(BrowserPpapiHostImpl, msg)
+  IPC_BEGIN_MESSAGE_MAP(BrowserPpapiHostImpl::HostMessageFilter, msg)
     // Add necessary message handlers here.
+    IPC_MESSAGE_HANDLER(PpapiHostMsg_IdleState,
+                        OnIdleState)
     IPC_MESSAGE_UNHANDLED(handled = ppapi_host_->OnMessageReceived(msg))
   IPC_END_MESSAGE_MAP();
   return handled;
-  */
-  return ppapi_host_->OnMessageReceived(msg);
 }
 
 void BrowserPpapiHostImpl::HostMessageFilter::OnHostDestroyed() {
+  browser_ppapi_host_impl_ = NULL;
   DCHECK(ppapi_host_);
   ppapi_host_ = NULL;
 }
 
+void BrowserPpapiHostImpl::HostMessageFilter::OnIdleState(bool idle) {
+  if (browser_ppapi_host_impl_) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(&BrowserPpapiHostImpl::OnIdleState,
+                                       browser_ppapi_host_impl_,
+                                       idle));
+  }
+}
+
 }  // namespace content