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

ppapi/proxy/plugin_main_nacl.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 <map>
 #include <set>
 
 #include "build/build_config.h"
 // Need to include this before most other files because it defines
 // IPC_MESSAGE_LOG_ENABLED. We need to use it to define
 // IPC_MESSAGE_MACROS_LOG_ENABLED so ppapi_messages.h will generate the
 // ViewMsgLog et al. functions.
 
 #include "base/command_line.h"
+#include "base/debug/trace_event.h"
 #include "base/message_loop/message_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "components/tracing/child_trace_message_filter.h"
 #include "ipc/ipc_channel_handle.h"
 #include "ipc/ipc_logging.h"
 #include "ipc/ipc_message.h"
 #include "native_client/src/shared/srpc/nacl_srpc.h"
 #include "native_client/src/untrusted/irt/irt_ppapi.h"
 #include "ppapi/c/ppp.h"
@@ skipping 234 matching lines @@
 
 }  // namespace
 
 void PpapiPluginRegisterThreadCreator(
     const struct PP_ThreadFunctions* thread_functions) {
   // Initialize all classes that need to create threads that call back into
   // user code.
   ppapi::PPB_Audio_Shared::SetThreadFunctions(thread_functions);
 }
 
+const int checkIntervalInSeconds = 1;

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

nit: kCheckIntervalInSeconds

Some rationale about why 1 second is appropriate might be good (it's not obvious
to me)

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

Done.

+bool last_was_active = false;

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

Could this be a parameter to CheckActivity instead of a global?

Also...  kCheckIntervalInSeconds and g_last_was_active (if left global) should
probably move to the unnamed namespace.

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

Done.

+
+void CheckActivity(base::MessageLoop* loop, PpapiDispatcher* dispatcher) {

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

You're not using dispatcher, and it will become invalid at process shutdown. I
think you want to just remove the argument.

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

Done.

+  bool was_active = PluginGlobals::Get()->plugin_has_been_active();

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

At shutdown, PluginGlobals()::Get() will be NULL. Maybe you should just break
early in that case.

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

Done.

+  PluginGlobals::Get()->set_plugin_has_been_active(false);
+ 
+  if (last_was_active != was_active) {
+    PluginGlobals::Get()->GetBrowserSender()->Send(
+        new PpapiHostMsg_IdleState(!was_active));

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

So we're only sending a message if the state changes? If the plugin says it's
active (not idle), and then doesn't send a message for a long time, does the
browser then assume that it's active? Is that what we want?

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

I think it is what we want. Browser side we are tracking activity by
incrementing / decrementing a counter. There's no value to sending more IPC
traffic I think. If the plugin goes dark while we think it's active that's an
orthogonal problem I believe is already solved by a watchdog that will notice a
frozen plugin process.

[dmichael (off chromium), 2013/11/07 17:11:55]

Sounds good. I wrote this comment before I'd read the rest of the CL.

+  }
+  last_was_active = was_active;
+
+  loop->PostDelayedTask(FROM_HERE, base::Bind(&CheckActivity, loop, dispatcher),

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

Why not use MessageLoop::current()? I'm not used to seeing a bare MessageLoop
pointer passed around; if a parameter was necessary,
scoped_refptr<MessageLoopProxy> would seem better. But it looks like
MessageLoop::current() will do the trick here...

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

Done.

+                        base::TimeDelta::FromSeconds(checkIntervalInSeconds));
+}
+
 int PpapiPluginMain() {
   // Though it isn't referenced here, we must instantiate an AtExitManager.
   base::AtExitManager exit_manager;
   base::MessageLoop loop;
   IPC::Logging::set_log_function_map(&g_log_function_mapping);
   ppapi::proxy::PluginGlobals plugin_globals;
   base::Thread io_thread("Chrome_NaClIOThread");
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
   io_thread.StartWithOptions(options);
 
   // Start up the SRPC server on another thread. Otherwise, when it blocks
   // on an RPC, the PPAPI proxy will hang. Do this before we initialize the
   // module and start the PPAPI proxy so that the NaCl plugin can continue
   // loading the app.
   static struct NaClSrpcHandlerDesc srpc_methods[] = { { NULL, NULL } };
   if (!NaClSrpcAcceptClientOnThread(srpc_methods)) {
     return 1;
   }
 
   PpapiDispatcher ppapi_dispatcher(io_thread.message_loop_proxy());
   plugin_globals.set_plugin_proxy_delegate(&ppapi_dispatcher);
 
+  io_thread.message_loop()->PostDelayedTask(FROM_HERE,
+                                            base::Bind(&CheckActivity,

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

CheckActivity needs to have the global proxy lock, since it uses shared stuff
like PluginGlobals. You can use "RunWhileLocked" from
ppapi/shared_impl/proxy_lock.h here to make that happen cleanly.
(https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...)

Example usage:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_va...

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

Done via: ppapi::ProxyAutoLock lock;

+                                                       io_thread.message_loop(),
+                                                       &ppapi_dispatcher),
+                       base::TimeDelta::FromSeconds(checkIntervalInSeconds));

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

(nit: all the arguments should just be indented by 4 so this one can fit)

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

Done.

   loop.Run();
 
   return 0;
 }