PPAPI: Fix GetBrowserInterface race conditions

- Previously, we had an unguarded bool flag for whether an interface lookup had been logged to UMA. Now each InterfaceInfo has a lock+flag. Should be near-zero contention.
- Previously, PpapiGlobals::GetBrowserSender did lazy creation with no synchronization. Now we create it at process startup and clear it at process shutdown, so there should be no race while the plugin is running.

BUG=413513,414150

Committed: https://crrev.com/d1b2c8f719b0ab471a476bf53911a3657bb4c06a
Cr-Commit-Position: refs/heads/master@{#294715}

Review URL: https://codereview.chromium.org/568793002

Cr-Commit-Position: refs/heads/master@{#295551}

content/ppapi_plugin/ppapi_thread.cc

@@ -107,7 +107,7 @@ PpapiThread::PpapiThread(const CommandLine& command_line, bool is_broker)
           base::RandInt(0, std::numeric_limits<PP_Module>::max())),
       next_plugin_dispatcher_id_(1) {
   ppapi::proxy::PluginGlobals* globals = ppapi::proxy::PluginGlobals::Get();
-  globals->set_plugin_proxy_delegate(this);
+  globals->SetPluginProxyDelegate(this);
   globals->set_command_line(
       command_line.GetSwitchValueASCII(switches::kPpapiFlashArgs));
 
@@ -127,7 +127,7 @@ PpapiThread::~PpapiThread() {
 void PpapiThread::Shutdown() {
   ChildThread::Shutdown();
 
-  ppapi::proxy::PluginGlobals::Get()->set_plugin_proxy_delegate(NULL);
+  ppapi::proxy::PluginGlobals::Get()->ResetPluginProxyDelegate();
   if (plugin_entry_points_.shutdown_module)
     plugin_entry_points_.shutdown_module();
   webkit_platform_support_->Shutdown();

ppapi/nacl_irt/plugin_main.cc

@@ -52,7 +52,7 @@ int PpapiPluginMain() {
       ppapi::GetShutdownEvent(),
       ppapi::GetBrowserIPCFileDescriptor(),
       ppapi::GetRendererIPCFileDescriptor());
-  plugin_globals.set_plugin_proxy_delegate(&ppapi_dispatcher);
+  plugin_globals.SetPluginProxyDelegate(&ppapi_dispatcher);
 
   loop.Run();
 

ppapi/proxy/interface_list.cc

@@ -320,6 +320,8 @@ InterfaceList::~InterfaceList() {
 
 // static
 InterfaceList* InterfaceList::GetInstance() {
+  // CAUTION: This function is called without the ProxyLock to avoid excessive
+  // excessive locking from C++ wrappers. (See also GetBrowserInterface.)
   return Singleton<InterfaceList>::get();
 }
 
@@ -338,20 +340,19 @@ InterfaceProxy::Factory InterfaceList::GetFactoryForID(ApiID id) const {
 }
 
 const void* InterfaceList::GetInterfaceForPPB(const std::string& name) {
+  // CAUTION: This function is called without the ProxyLock to avoid excessive
+  // excessive locking from C++ wrappers. (See also GetBrowserInterface.)
   NameToInterfaceInfoMap::iterator found =
       name_to_browser_info_.find(name);
   if (found == name_to_browser_info_.end())
     return NULL;
 
   if (g_process_global_permissions.Get().HasPermission(
-          found->second.required_permission)) {
+          found->second->required_permission())) {
     // Only log interface use once per plugin.
-    if (!found->second.interface_logged) {
-      PluginGlobals::Get()->GetBrowserSender()->Send(
-          new PpapiHostMsg_LogInterfaceUsage(HashInterfaceName(name)));
-      found->second.interface_logged = true;
-    }
-    return found->second.iface;
+    found->second->LogWithUmaOnce(
+        PluginGlobals::Get()->GetBrowserSender(), name);
+    return found->second->iface();
   }
   return NULL;
 }
@@ -361,7 +362,20 @@ const void* InterfaceList::GetInterfaceForPPP(const std::string& name) const {
       name_to_plugin_info_.find(name);
   if (found == name_to_plugin_info_.end())
     return NULL;
-  return found->second.iface;
+  return found->second->iface();
+}
+
+void InterfaceList::InterfaceInfo::LogWithUmaOnce(
+    IPC::Sender* sender, const std::string& name) {
+  {
+    base::AutoLock acquire(sent_to_uma_lock_);
+    if (sent_to_uma_)
+      return;
+    sent_to_uma_ = true;
+  }
+  int hash = InterfaceList::HashInterfaceName(name);
+  PluginGlobals::Get()->GetBrowserSender()->Send(
+      new PpapiHostMsg_LogInterfaceUsage(hash));
 }
 
 void InterfaceList::AddProxy(ApiID id,
@@ -384,16 +398,18 @@ void InterfaceList::AddPPB(const char* name,
                            const void* iface,
                            Permission perm) {
   DCHECK(name_to_browser_info_.find(name) == name_to_browser_info_.end());
-  name_to_browser_info_[name] = InterfaceInfo(iface, perm);
+  name_to_browser_info_.add(
+      name, scoped_ptr<InterfaceInfo>(new InterfaceInfo(iface, perm)));
 }
 
 void InterfaceList::AddPPP(const char* name,
                            const void* iface) {
   DCHECK(name_to_plugin_info_.find(name) == name_to_plugin_info_.end());
-  name_to_plugin_info_[name] = InterfaceInfo(iface, PERMISSION_NONE);
+  name_to_plugin_info_.add(
+      name,
+      scoped_ptr<InterfaceInfo>(new InterfaceInfo(iface, PERMISSION_NONE)));
 }
 
-// static
 int InterfaceList::HashInterfaceName(const std::string& name) {
   uint32 data = base::Hash(name.c_str(), name.size());
   // Strip off the signed bit because UMA doesn't support negative values,

ppapi/proxy/interface_list.h

@@ -9,6 +9,8 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/containers/scoped_ptr_hash_map.h"
+#include "base/synchronization/lock.h"
 #include "ppapi/proxy/interface_proxy.h"
 #include "ppapi/proxy/ppapi_proxy_export.h"
 #include "ppapi/shared_impl/ppapi_permissions.h"
@@ -47,30 +49,39 @@ class PPAPI_PROXY_EXPORT InterfaceList {
  private:
   friend class InterfaceListTest;
 
-  struct InterfaceInfo {
-    InterfaceInfo()
-        : iface(NULL),
-          required_permission(PERMISSION_NONE),
-          interface_logged(false) {
-    }
+  class InterfaceInfo {
+   public:
     InterfaceInfo(const void* in_interface, Permission in_perm)
-        : iface(in_interface),
-          required_permission(in_perm),
-          interface_logged(false) {
+        : iface_(in_interface),
+          required_permission_(in_perm),
+          sent_to_uma_(false) {
     }
 
-    const void* iface;
+    const void* iface() { return iface_; }
 
     // Permission required to return non-null for this interface. This will
     // be checked with the value set via SetProcessGlobalPermissionBits when
     // an interface is requested.
-    Permission required_permission;
+    Permission required_permission() { return required_permission_; };
+
+    // Call this any time the interface is requested. It will log a UMA count
+    // only the first time. This is safe to call from any thread, regardless of
+    // whether the proxy lock is held.
+    void LogWithUmaOnce(IPC::Sender* sender, const std::string& name);
+   private:
+    DISALLOW_COPY_AND_ASSIGN(InterfaceInfo);
+
+    const void* const iface_;
+    const Permission required_permission_;
 
-    // Interface usage is logged just once per-interface-per-plugin-process.
-    bool interface_logged;
+    bool sent_to_uma_;
+    base::Lock sent_to_uma_lock_;
   };
+  // Give friendship for HashInterfaceName.
+  friend class InterfaceInfo;
 
-  typedef std::map<std::string, InterfaceInfo> NameToInterfaceInfoMap;
+  typedef base::ScopedPtrHashMap<std::string, InterfaceInfo>
+      NameToInterfaceInfoMap;
 
   void AddProxy(ApiID id, InterfaceProxy::Factory factory);
 

ppapi/proxy/plugin_dispatcher.cc

@@ -108,13 +108,8 @@ PluginDispatcher* PluginDispatcher::GetForResource(const Resource* resource) {
 
 // static
 const void* PluginDispatcher::GetBrowserInterface(const char* interface_name) {
-  if (!interface_name) {
-    DLOG(WARNING) << "|interface_name| is null. Did you forget to add "
-        "the |interface_name()| template function to the interface's C++ "
-        "wrapper?";
-    return NULL;
-  }
-
+  // CAUTION: This function is called directly from the plugin, but we *don't*
+  // lock the ProxyLock to avoid excessive locking from C++ wrappers.
   return InterfaceList::GetInstance()->GetInterfaceForPPB(interface_name);
 }
 

ppapi/proxy/plugin_globals.cc

@@ -192,11 +192,13 @@ void PluginGlobals::MarkPluginIsActive() {
 }
 
 IPC::Sender* PluginGlobals::GetBrowserSender() {
-  if (!browser_sender_.get()) {
-    browser_sender_.reset(
-        new BrowserSender(plugin_proxy_delegate_->GetBrowserSender()));
-  }
-
+  // CAUTION: This function is called without the ProxyLock. See also
+  // InterfaceList::GetInterfaceForPPB.
+  //
+  // See also SetPluginProxyDelegate. That initializes browser_sender_ before
+  // the plugin can start threads, and it may be cleared after the
+  // plugin has torn down threads. So this pointer is expected to remain valid
+  // during the lifetime of the plugin.
   return browser_sender_.get();
 }
 
@@ -217,6 +219,19 @@ PP_Resource PluginGlobals::CreateBrowserFont(
       connection, instance, desc, prefs);
 }
 
+void PluginGlobals::SetPluginProxyDelegate(PluginProxyDelegate* delegate) {
+  DCHECK(delegate && !plugin_proxy_delegate_);
+  plugin_proxy_delegate_ = delegate;
+  browser_sender_.reset(
+      new BrowserSender(plugin_proxy_delegate_->GetBrowserSender()));
+}
+
+void PluginGlobals::ResetPluginProxyDelegate() {
+  DCHECK(plugin_proxy_delegate_);
+  plugin_proxy_delegate_ = NULL;
+  browser_sender_.reset();
+}
+
 MessageLoopResource* PluginGlobals::loop_for_main_thread() {
   return loop_for_main_thread_.get();
 }

ppapi/proxy/plugin_globals.h

@@ -99,10 +99,12 @@ class PPAPI_PROXY_EXPORT PluginGlobals : public PpapiGlobals {
     return &plugin_var_tracker_;
   }
 
-  // The embedder should call set_proxy_delegate during startup.
-  void set_plugin_proxy_delegate(PluginProxyDelegate* d) {
-    plugin_proxy_delegate_ = d;
-  }
+  // The embedder should call SetPluginProxyDelegate during startup.
+  void SetPluginProxyDelegate(PluginProxyDelegate* d);
+  // The embedder may choose to call ResetPluginProxyDelegate during shutdown.
+  // After that point, it's unsafe to call most members of PluginGlobals,
+  // and GetBrowserSender will return NULL.
+  void ResetPluginProxyDelegate();
 
   // Returns the TLS slot that holds the message loop TLS.
   //

ppapi/proxy/ppapi_proxy_test.cc

@@ -180,7 +180,7 @@ void PluginProxyTestHarness::SetUpHarness() {
   // browser. In this case we just use the |plugin_dispatcher_| as the channel
   // for test purposes.
   plugin_delegate_mock_.set_browser_sender(plugin_dispatcher_.get());
-  PluginGlobals::Get()->set_plugin_proxy_delegate(&plugin_delegate_mock_);
+  PluginGlobals::Get()->SetPluginProxyDelegate(&plugin_delegate_mock_);
   plugin_dispatcher_->DidCreateInstance(pp_instance());
 }
 
@@ -206,7 +206,7 @@ void PluginProxyTestHarness::SetUpHarnessWithChannel(
                                             channel_handle,
                                             is_client);
   plugin_delegate_mock_.set_browser_sender(plugin_dispatcher_.get());
-  PluginGlobals::Get()->set_plugin_proxy_delegate(&plugin_delegate_mock_);
+  PluginGlobals::Get()->SetPluginProxyDelegate(&plugin_delegate_mock_);
   plugin_dispatcher_->DidCreateInstance(pp_instance());
 }