Revert of PPAPI: Fix GetBrowserInterface race conditions (patchset #6 id:100001 of https://codereview.chromium.org/568793002/)

Reason for revert:
Sorry to revert but I randomly noticed this to be causing a top crasher e.g. go/crash/75090c1e31f33cfd

The reason is because the patch explicitly calls PluginGlobals::SetPluginProxyDelegate with NULL from PpapiThread::Shutdown but the pointer is used immediately inside the function.

I guess it should be a simple fix. I was going to put up a fix which just reset the browser_sender_ to NULL in that case, but then I noticed there were cases where we call GetBrowserSender() and use it without checking whether it is NULL (e.g. https://code.google.com/p/chromium/codesearch#search/&q=%22PluginGlobals::Get()-%3EGetBrowserSender()-%3ESend%22&sq=package:chromium&type=cs)

I felt it would be better to just revert for now, sorry :(

Original issue's description:
> PPAPI: Fix GetBrowserInterface race conditions
>
> BUG=413513
>
> Committed: https://crrev.com/d1b2c8f719b0ab471a476bf53911a3657bb4c06a
> Cr-Commit-Position: refs/heads/master@{#294715}

TBR=teravest@chromium.org,piman@chromium.org,dmichael@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=413513

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

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

7 files changed, 44 insertions, 67 deletions

diff --git a/ppapi/nacl_irt/plugin_main.cc b/ppapi/nacl_irt/plugin_main.cc
index 8f0498c..668f67a 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.SetPluginProxyDelegate(&ppapi_dispatcher);
+  plugin_globals.set_plugin_proxy_delegate(&ppapi_dispatcher);
 
   loop.Run();
 
diff --git a/ppapi/proxy/interface_list.cc b/ppapi/proxy/interface_list.cc
index 75138dc..295069e 100644
--- a/ppapi/proxy/interface_list.cc
+++ b/ppapi/proxy/interface_list.cc
@@ -319,8 +319,6 @@ 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();
 }
 
@@ -339,19 +337,20 @@ 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.
-    found->second->LogWithUmaOnce(
-        PluginGlobals::Get()->GetBrowserSender(), name);
-    return found->second->iface();
+    if (!found->second.interface_logged) {
+      PluginGlobals::Get()->GetBrowserSender()->Send(
+          new PpapiHostMsg_LogInterfaceUsage(HashInterfaceName(name)));
+      found->second.interface_logged = true;
+    }
+    return found->second.iface;
   }
   return NULL;
 }
@@ -361,20 +360,7 @@ 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();
-}
-
-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));
+  return found->second.iface;
 }
 
 void InterfaceList::AddProxy(ApiID id,
@@ -397,18 +383,16 @@ 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_.add(
-      name, scoped_ptr<InterfaceInfo>(new InterfaceInfo(iface, perm)));
+  name_to_browser_info_[name] = 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_.add(
-      name,
-      scoped_ptr<InterfaceInfo>(new InterfaceInfo(iface, PERMISSION_NONE)));
+  name_to_plugin_info_[name] = 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 dc109e5..a2b4569c 100644
--- a/ppapi/proxy/interface_list.h
+++ b/ppapi/proxy/interface_list.h
@@ -9,8 +9,6 @@
 #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"
@@ -49,39 +47,30 @@ class PPAPI_PROXY_EXPORT InterfaceList {
  private:
   friend class InterfaceListTest;
 
-  class InterfaceInfo {
-   public:
+  struct InterfaceInfo {
+    InterfaceInfo()
+        : iface(NULL),
+          required_permission(PERMISSION_NONE),
+          interface_logged(false) {
+    }
     InterfaceInfo(const void* in_interface, Permission in_perm)
-        : iface_(in_interface),
-          required_permission_(in_perm),
-          sent_to_uma_(false) {
+        : iface(in_interface),
+          required_permission(in_perm),
+          interface_logged(false) {
     }
 
-    const void* iface() { return iface_; }
+    const void* 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() { 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_;
+    Permission required_permission;
 
-    bool sent_to_uma_;
-    base::Lock sent_to_uma_lock_;
+    // Interface usage is logged just once per-interface-per-plugin-process.
+    bool interface_logged;
   };
-  // Give friendship for HashInterfaceName.
-  friend class InterfaceInfo;
 
-  typedef base::ScopedPtrHashMap<std::string, InterfaceInfo>
-      NameToInterfaceInfoMap;
+  typedef std::map<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 ad3e7a0..f6448d3 100644
--- a/ppapi/proxy/plugin_dispatcher.cc
+++ b/ppapi/proxy/plugin_dispatcher.cc
@@ -108,8 +108,13 @@ PluginDispatcher* PluginDispatcher::GetForResource(const Resource* resource) {
 
 // static
 const void* PluginDispatcher::GetBrowserInterface(const char* interface_name) {
-  // CAUTION: This function is called directly from the plugin, but we *don't*
-  // lock the ProxyLock to avoid excessive locking from C++ wrappers.
+  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;
+  }
+
   return InterfaceList::GetInstance()->GetInterfaceForPPB(interface_name);
 }
 
diff --git a/ppapi/proxy/plugin_globals.cc b/ppapi/proxy/plugin_globals.cc
index b372207..82e16ff 100644
--- a/ppapi/proxy/plugin_globals.cc
+++ b/ppapi/proxy/plugin_globals.cc
@@ -192,8 +192,11 @@ void PluginGlobals::MarkPluginIsActive() {
 }
 
 IPC::Sender* PluginGlobals::GetBrowserSender() {
-  // CAUTION: This function is called without the ProxyLock. See also
-  // InterfaceList::GetInterfaceForPPB.
+  if (!browser_sender_.get()) {
+    browser_sender_.reset(
+        new BrowserSender(plugin_proxy_delegate_->GetBrowserSender()));
+  }
+
   return browser_sender_.get();
 }
 
@@ -214,12 +217,6 @@ 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 53eb18a..5dc614d 100644
--- a/ppapi/proxy/plugin_globals.h
+++ b/ppapi/proxy/plugin_globals.h
@@ -99,8 +99,10 @@ class PPAPI_PROXY_EXPORT PluginGlobals : public PpapiGlobals {
     return &plugin_var_tracker_;
   }
 
-  // The embedder should call SetPluginProxyDelegate during startup.
-  void SetPluginProxyDelegate(PluginProxyDelegate* d);
+  // The embedder should call set_proxy_delegate during startup.
+  void set_plugin_proxy_delegate(PluginProxyDelegate* d) {
+    plugin_proxy_delegate_ = 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 81793d5..fa23bfd 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()->SetPluginProxyDelegate(&plugin_delegate_mock_);
+  PluginGlobals::Get()->set_plugin_proxy_delegate(&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()->SetPluginProxyDelegate(&plugin_delegate_mock_);
+  PluginGlobals::Get()->set_plugin_proxy_delegate(&plugin_delegate_mock_);
   plugin_dispatcher_->DidCreateInstance(pp_instance());
 }