PPAPI: Fix GetBrowserInterface race conditions

BUG=413513

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

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

7 files changed, 67 insertions, 44 deletions

diff --git a/ppapi/nacl_irt/plugin_main.cc b/ppapi/nacl_irt/plugin_main.cc
index 668f67a..8f0498c 100644
--- a/ppapi/nacl_irt/plugin_main.cc
+++ b/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();
 
diff --git a/ppapi/proxy/interface_list.cc b/ppapi/proxy/interface_list.cc
index 295069e..75138dc 100644
--- a/ppapi/proxy/interface_list.cc
+++ b/ppapi/proxy/interface_list.cc
@@ -319,6 +319,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();
 }
 
@@ -337,20 +339,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;
 }
@@ -360,7 +361,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,
@@ -383,16 +397,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,
diff --git a/ppapi/proxy/interface_list.h b/ppapi/proxy/interface_list.h
index a2b4569c..dc109e5 100644
--- a/ppapi/proxy/interface_list.h
+++ b/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);
 
diff --git a/ppapi/proxy/plugin_dispatcher.cc b/ppapi/proxy/plugin_dispatcher.cc
index f6448d3..ad3e7a0 100644
--- a/ppapi/proxy/plugin_dispatcher.cc
+++ b/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);
 }
 
diff --git a/ppapi/proxy/plugin_globals.cc b/ppapi/proxy/plugin_globals.cc
index 82e16ff..b372207 100644
--- a/ppapi/proxy/plugin_globals.cc
+++ b/ppapi/proxy/plugin_globals.cc
@@ -192,11 +192,8 @@ 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.
   return browser_sender_.get();
 }
 
@@ -217,6 +214,12 @@ PP_Resource PluginGlobals::CreateBrowserFont(
       connection, instance, desc, prefs);
 }
 
+void PluginGlobals::SetPluginProxyDelegate(PluginProxyDelegate* delegate) {
+  plugin_proxy_delegate_ = delegate;
+  browser_sender_.reset(
+      new BrowserSender(plugin_proxy_delegate_->GetBrowserSender()));
+}
+
 MessageLoopResource* PluginGlobals::loop_for_main_thread() {
   return loop_for_main_thread_.get();
 }
diff --git a/ppapi/proxy/plugin_globals.h b/ppapi/proxy/plugin_globals.h
index 5dc614d..53eb18a 100644
--- a/ppapi/proxy/plugin_globals.h
+++ b/ppapi/proxy/plugin_globals.h
@@ -99,10 +99,8 @@ 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);
 
   // Returns the TLS slot that holds the message loop TLS.
   //
diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc
index fa23bfd..81793d5 100644
--- a/ppapi/proxy/ppapi_proxy_test.cc
+++ b/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());
 }