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

content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/pepper/browser_ppapi_host_impl.h"
 
 #include "content/browser/renderer_host/pepper/pepper_message_filter.h"
 #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,
     IPC::ChannelProxy* channel,
     int render_process_id,
     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 */);
   browser_ppapi_host->set_plugin_process_handle(plugin_child_process);
 
   scoped_refptr<PepperMessageFilter> pepper_message_filter(
       new PepperMessageFilter());
   channel->AddFilter(pepper_message_filter->GetFilter());
   channel->AddFilter(browser_ppapi_host->message_filter());
   channel->AddFilter((new TraceMessageFilter())->GetFilter());
 
   return browser_ppapi_host;
 }
 
 BrowserPpapiHostImpl::BrowserPpapiHostImpl(
+    BrowserPpapiHost::Delegate* delegate,
     IPC::Sender* sender,
     const ppapi::PpapiPermissions& permissions,
     const std::string& plugin_name,
     const base::FilePath& plugin_path,
     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),
       profile_data_directory_(profile_data_directory),
       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)));
 }
 
 BrowserPpapiHostImpl::~BrowserPpapiHostImpl() {
   // Notify the filter so it won't foward messages to us.
   message_filter_->OnHostDestroyed();
 
   // Delete the host explicitly first. This shutdown will destroy the
   // resources, which may want to do cleanup in their destructors and expect
@@ skipping 66 matching lines @@
 
 void BrowserPpapiHostImpl::DeleteInstance(PP_Instance instance) {
   InstanceMap::iterator found = instance_map_.find(instance);
   if (found == instance_map_.end()) {
     NOTREACHED();
     return;
   }
   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