PPAPI: Purposely leak ProxyLock, fix shutdown race

This CL: - Makes callbacks returned by RunWhileLocked acquire the lock when they are destroyed if they are not run. This eliminates some potential race conditions. - Because MessageLoops can be destroyed after the PpapiGlobals (and thus the ProxyLock), this means we need to make the ProxyLock live longer. So we leak it so that it lives all the way through shutdown. This issue depends on: https://codereview.chromium.org/19492014/ BUG= R=yzshen@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222829 Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=222840 (static initializer + content_unittests failure) Review URL: https://chromiumcodereview.appspot.com/19492014 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@223738 0039d316-1c4b-4281-b951-d872f2087c98
Denger-Network · Sep 17, 2013 · b1d571a · b1d571a
1 parent f1cbadc
commit b1d571a
Show file tree

Hide file tree

Showing 21 changed files with 183 additions and 59 deletions.
diff --git a/content/renderer/pepper/host_globals.cc b/content/renderer/pepper/host_globals.cc
@@ -17,6 +17,7 @@
 #include "content/renderer/render_thread_impl.h"
 #include "ppapi/shared_impl/api_id.h"
 #include "ppapi/shared_impl/id_assignment.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/web/WebConsoleMessage.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
@@ -81,13 +82,9 @@ HostGlobals::HostGlobals()
       resource_tracker_(ResourceTracker::SINGLE_THREADED) {
   DCHECK(!host_globals_);
   host_globals_ = this;
-}
-
-HostGlobals::HostGlobals(
-    ppapi::PpapiGlobals::PerThreadForTest per_thread_for_test)
-    : ppapi::PpapiGlobals(per_thread_for_test),
-      resource_tracker_(ResourceTracker::SINGLE_THREADED) {
-  DCHECK(!host_globals_);
+  // We do not support calls off of the main thread on the host side, and thus
+  // do not lock.
+  ppapi::ProxyLock::DisableLocking();
 }
 
 HostGlobals::~HostGlobals() {
@@ -141,11 +138,6 @@ void HostGlobals::PreCacheFontForFlash(const void* logfontw) {
   // Not implemented in-process.
 }
 
-base::Lock* HostGlobals::GetProxyLock() {
-  // We do not lock on the host side.
-  return NULL;
-}
-
 void HostGlobals::LogWithSource(PP_Instance instance,
                                 PP_LogLevel level,
                                 const std::string& source,

diff --git a/content/renderer/pepper/host_globals.h b/content/renderer/pepper/host_globals.h
@@ -20,7 +20,6 @@ class PluginModule;
 class HostGlobals : public ppapi::PpapiGlobals {
  public:
   HostGlobals();
-  explicit HostGlobals(ppapi::PpapiGlobals::PerThreadForTest);
   virtual ~HostGlobals();
 
   // Getter for the global singleton. Generally, you should use
@@ -43,7 +42,6 @@ class HostGlobals : public ppapi::PpapiGlobals {
   virtual PP_Module GetModuleForInstance(PP_Instance instance) OVERRIDE;
   virtual std::string GetCmdLine() OVERRIDE;
   virtual void PreCacheFontForFlash(const void* logfontw) OVERRIDE;
-  virtual base::Lock* GetProxyLock() OVERRIDE;
   virtual void LogWithSource(PP_Instance instance,
                              PP_LogLevel level,
                              const std::string& source,

diff --git a/content/renderer/pepper/pepper_file_chooser_host_unittest.cc b/content/renderer/pepper/pepper_file_chooser_host_unittest.cc
@@ -19,6 +19,7 @@
 #include "ppapi/proxy/resource_message_test_sink.h"
 #include "ppapi/shared_impl/ppapi_permissions.h"
 #include "ppapi/shared_impl/ppb_file_ref_shared.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/shared_impl/test_globals.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -36,6 +37,7 @@ class PepperFileChooserHostTest : public RenderViewTest {
   virtual void SetUp() {
     SetContentClient(&client_);
     RenderViewTest::SetUp();
+    ppapi::ProxyLock::DisableLockingOnThreadForTest();
 
     globals_.GetResourceTracker()->DidCreateInstance(pp_instance_);
   }

diff --git a/content/renderer/pepper/pepper_url_request_unittest.cc b/content/renderer/pepper/pepper_url_request_unittest.cc
@@ -7,6 +7,7 @@
 #include "content/renderer/pepper/url_request_info_util.h"
 #include "ppapi/proxy/connection.h"
 #include "ppapi/proxy/url_request_info_resource.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/test_globals.h"
 #include "ppapi/shared_impl/url_request_info_data.h"
 #include "ppapi/thunk/thunk.h"
@@ -60,6 +61,7 @@ class URLRequestInfoTest : public RenderViewTest {
 
   virtual void SetUp() OVERRIDE {
     RenderViewTest::SetUp();
+    ppapi::ProxyLock::DisableLockingOnThreadForTest();
 
     test_globals_.GetResourceTracker()->DidCreateInstance(pp_instance_);
 

diff --git a/content/renderer/pepper/v8_var_converter_unittest.cc b/content/renderer/pepper/v8_var_converter_unittest.cc
@@ -175,6 +175,7 @@ class V8VarConverterTest : public testing::Test {
     converter_->FromV8Value(val, context, base::Bind(
         &V8VarConverterTest::FromV8ValueComplete, base::Unretained(this),
         loop.QuitClosure()));
+    loop.Run();
     if (conversion_success_)
       *result = conversion_result_;
     return conversion_success_;

diff --git a/ppapi/proxy/plugin_globals.cc b/ppapi/proxy/plugin_globals.cc
@@ -128,10 +128,6 @@ void PluginGlobals::PreCacheFontForFlash(const void* logfontw) {
   plugin_proxy_delegate_->PreCacheFont(logfontw);
 }
 
-base::Lock* PluginGlobals::GetProxyLock() {
-  return &proxy_lock_;
-}
-
 void PluginGlobals::LogWithSource(PP_Instance instance,
                                   PP_LogLevel level,
                                   const std::string& source,

diff --git a/ppapi/proxy/plugin_globals.h b/ppapi/proxy/plugin_globals.h
@@ -9,7 +9,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/synchronization/lock.h"
 #include "base/threading/thread_local_storage.h"
 #include "ppapi/proxy/connection.h"
 #include "ppapi/proxy/plugin_resource_tracker.h"
@@ -64,7 +63,6 @@ class PPAPI_PROXY_EXPORT PluginGlobals : public PpapiGlobals {
   virtual PP_Module GetModuleForInstance(PP_Instance instance) OVERRIDE;
   virtual std::string GetCmdLine() OVERRIDE;
   virtual void PreCacheFontForFlash(const void* logfontw) OVERRIDE;
-  virtual base::Lock* GetProxyLock() OVERRIDE;
   virtual void LogWithSource(PP_Instance instance,
                              PP_LogLevel level,
                              const std::string& source,
@@ -143,8 +141,6 @@ class PPAPI_PROXY_EXPORT PluginGlobals : public PpapiGlobals {
   PluginVarTracker plugin_var_tracker_;
   scoped_refptr<CallbackTracker> callback_tracker_;
 
-  base::Lock proxy_lock_;
-
   scoped_ptr<base::ThreadLocalStorage::Slot> msg_loop_slot_;
   // Note that loop_for_main_thread's constructor sets msg_loop_slot_, so it
   // must be initialized after msg_loop_slot_ (hence the order here).

diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc
@@ -227,8 +227,12 @@ void PluginProxyTestHarness::CreatePluginGlobals() {
   if (globals_config_ == PER_THREAD_GLOBALS) {
     plugin_globals_.reset(new PluginGlobals(PpapiGlobals::PerThreadForTest()));
     PpapiGlobals::SetPpapiGlobalsOnThreadForTest(GetGlobals());
+    // Enable locking in case some other unit test ran before us and disabled
+    // locking.
+    ProxyLock::EnableLockingOnThreadForTest();
   } else {
     plugin_globals_.reset(new PluginGlobals());
+    ProxyLock::EnableLockingOnThreadForTest();
   }
 }
 
@@ -471,7 +475,10 @@ void HostProxyTestHarness::CreateHostGlobals() {
   if (globals_config_ == PER_THREAD_GLOBALS) {
     host_globals_.reset(new TestGlobals(PpapiGlobals::PerThreadForTest()));
     PpapiGlobals::SetPpapiGlobalsOnThreadForTest(GetGlobals());
+    // The host side of the proxy does not lock.
+    ProxyLock::DisableLockingOnThreadForTest();
   } else {
+    ProxyLock::DisableLockingOnThreadForTest();
     host_globals_.reset(new TestGlobals());
   }
 }

diff --git a/ppapi/proxy/raw_var_data_unittest.cc b/ppapi/proxy/raw_var_data_unittest.cc
@@ -37,6 +37,7 @@ class RawVarDataTest : public testing::Test {
 
   // testing::Test implementation.
   virtual void SetUp() {
+    ProxyLock::EnableLockingOnThreadForTest();
     ProxyLock::Acquire();
   }
   virtual void TearDown() {

diff --git a/ppapi/shared_impl/ppapi_globals.h b/ppapi/shared_impl/ppapi_globals.h
@@ -17,7 +17,6 @@
 #include "ppapi/shared_impl/ppapi_shared_export.h"
 
 namespace base {
-class Lock;
 class MessageLoopProxy;
 class TaskRunner;
 }
@@ -69,8 +68,6 @@ class PPAPI_SHARED_EXPORT PpapiGlobals {
   virtual CallbackTracker* GetCallbackTrackerForInstance(
       PP_Instance instance) = 0;
 
-  virtual base::Lock* GetProxyLock() = 0;
-
   // Logs the given string to the JS console. If "source" is empty, the name of
   // the current module will be used, if it can be determined.
   virtual void LogWithSource(PP_Instance instance,

diff --git a/ppapi/shared_impl/proxy_lock.cc b/ppapi/shared_impl/proxy_lock.cc
@@ -11,14 +11,33 @@
 
 namespace ppapi {
 
+base::LazyInstance<base::Lock>::Leaky
+    g_proxy_lock = LAZY_INSTANCE_INITIALIZER;
+
+bool g_disable_locking = false;
+base::LazyInstance<base::ThreadLocalBoolean>::Leaky
+    g_disable_locking_for_thread = LAZY_INSTANCE_INITIALIZER;
+
 // Simple single-thread deadlock detector for the proxy lock.
 // |true| when the current thread has the lock.
 base::LazyInstance<base::ThreadLocalBoolean>::Leaky
     g_proxy_locked_on_thread = LAZY_INSTANCE_INITIALIZER;
 
+// static
+base::Lock* ProxyLock::Get() {
+  if (g_disable_locking || g_disable_locking_for_thread.Get().Get())
+    return NULL;
+  return &g_proxy_lock.Get();
+}
+
+// Functions below should only access the lock via Get to ensure that they don't
+// try to use the lock on the host side of the proxy, where locking is
+// unnecessary and wrong (because we haven't coded the host side to account for
+// locking).
+
 // static
 void ProxyLock::Acquire() {
-  base::Lock* lock(PpapiGlobals::Get()->GetProxyLock());
+  base::Lock* lock = Get();
   if (lock) {
     // This thread does not already hold the lock.
     const bool deadlock = g_proxy_locked_on_thread.Get().Get();
@@ -31,7 +50,7 @@ void ProxyLock::Acquire() {
 
 // static
 void ProxyLock::Release() {
-  base::Lock* lock(PpapiGlobals::Get()->GetProxyLock());
+  base::Lock* lock = Get();
   if (lock) {
     // This thread currently holds the lock.
     const bool locked = g_proxy_locked_on_thread.Get().Get();
@@ -44,7 +63,7 @@ void ProxyLock::Release() {
 
 // static
 void ProxyLock::AssertAcquired() {
-  base::Lock* lock(PpapiGlobals::Get()->GetProxyLock());
+  base::Lock* lock = Get();
   if (lock) {
     // This thread currently holds the lock.
     const bool locked = g_proxy_locked_on_thread.Get().Get();
@@ -54,6 +73,25 @@ void ProxyLock::AssertAcquired() {
   }
 }
 
+// static
+void ProxyLock::DisableLocking() {
+  // Note, we don't DCHECK that this flag isn't already set, because multiple
+  // unit tests may run in succession and all set it.
+  g_disable_locking = true;
+}
+
+// static
+void ProxyLock::DisableLockingOnThreadForTest() {
+  // Note, we don't DCHECK that this flag isn't already set, because multiple
+  // unit tests may run in succession and all set it.
+  g_disable_locking_for_thread.Get().Set(true);
+}
+
+// static
+void ProxyLock::EnableLockingOnThreadForTest() {
+  g_disable_locking_for_thread.Get().Set(false);
+}
+
 void CallWhileUnlocked(const base::Closure& closure) {
   ProxyAutoUnlock lock;
   closure.Run();

diff --git a/ppapi/shared_impl/proxy_lock.h b/ppapi/shared_impl/proxy_lock.h
@@ -16,6 +16,10 @@ namespace base {
 class Lock;
 }
 
+namespace content {
+class HostGlobals;
+}
+
 namespace ppapi {
 
 // This is the one lock to rule them all for the ppapi proxy. All PPB interface
@@ -28,6 +32,12 @@ namespace ppapi {
 // tracker, etc.
 class PPAPI_SHARED_EXPORT ProxyLock {
  public:
+  // Return the global ProxyLock. Normally, you should not access this
+  // directly but instead use ProxyAutoLock or ProxyAutoUnlock. But sometimes
+  // you need access to the ProxyLock, for example to create a condition
+  // variable.
+  static base::Lock* Get();
+
   // Acquire the proxy lock. If it is currently held by another thread, block
   // until it is available. If the lock has not been set using the 'Set' method,
   // this operation does nothing. That is the normal case for the host side;
@@ -41,7 +51,23 @@ class PPAPI_SHARED_EXPORT ProxyLock {
   // process). Does nothing when running in-process (or in the host process).
   static void AssertAcquired();
 
+  // We have some unit tests where one thread pretends to be the host and one
+  // pretends to be the plugin. This allows the lock to do nothing on only one
+  // thread to support these tests. See TwoWayTest for more information.
+  static void DisableLockingOnThreadForTest();
+
+  // Enables locking on the current thread. Although locking is enabled by
+  // default, unit tests that rely on the lock being enabled should *still*
+  // call this, since a previous test may have disabled locking.
+  static void EnableLockingOnThreadForTest();
+
  private:
+  friend class content::HostGlobals;
+  // On the host side, we do not lock. This must be called at most once at
+  // startup, before other threads that may access the ProxyLock have had a
+  // chance to run.
+  static void DisableLocking();
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(ProxyLock);
 };
 
@@ -164,6 +190,35 @@ class RunWhileLockedHelper<void ()> {
     }
   }
 
+  ~RunWhileLockedHelper() {
+    // Check that the Callback is destroyed on the same thread as where
+    // CallWhileLocked happened (if CallWhileLocked happened).
+    DCHECK(thread_checker_.CalledOnValidThread());
+    // Here we read callback_ without the lock. This is why the callback must be
+    // destroyed on the same thread where it runs. There are 2 cases where
+    // callback_ will be NULL:
+    //   1) This is the original RunWhileLockedHelper that RunWhileLocked
+    //      created. When it was copied somewhere else (e.g., to a MessageLoop
+    //      queue), callback_ was passed to the new copy, and the original
+    //      RunWhileLockedHelper's callback_ was set to NULL (since scoped_ptrs
+    //      only ever have 1 owner). In this case, we don't want to acquire the
+    //      lock, because we already have it.
+    //   2) callback_ has already been run via CallWhileLocked. In this case,
+    //      there's no need to acquire the lock, because we don't touch any
+    //      shared data.
+    if (callback_) {
+      // If the callback was not run, we still need to have the lock when we
+      // destroy the callback in case it had a Resource bound to it. This
+      // ensures that the Resource's destructor is invoked only with the lock
+      // held.
+      //
+      // Also: Resource and Var inherit RefCounted (not ThreadSafeRefCounted),
+      // and these callbacks need to be usable on any thread. So we need to lock
+      // when releasing the callback to avoid ref counting races.
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
  private:
   scoped_ptr<CallbackType> callback_;
 
@@ -188,7 +243,13 @@ class RunWhileLockedHelper<void (P1)> {
       temp_callback->Run(p1);
     }
   }
-
+  ~RunWhileLockedHelper() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (callback_) {
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
  private:
   scoped_ptr<CallbackType> callback_;
   base::ThreadChecker thread_checker_;
@@ -211,7 +272,13 @@ class RunWhileLockedHelper<void (P1, P2)> {
       temp_callback->Run(p1, p2);
     }
   }
-
+  ~RunWhileLockedHelper() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (callback_) {
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
  private:
   scoped_ptr<CallbackType> callback_;
   base::ThreadChecker thread_checker_;
@@ -234,7 +301,13 @@ class RunWhileLockedHelper<void (P1, P2, P3)> {
       temp_callback->Run(p1, p2, p3);
     }
   }
-
+  ~RunWhileLockedHelper() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (callback_) {
+      ProxyAutoLock lock;
+      callback_.reset();
+    }
+  }
  private:
   scoped_ptr<CallbackType> callback_;
   base::ThreadChecker thread_checker_;
@@ -270,8 +343,6 @@ class RunWhileLockedHelper<void (P1, P2, P3)> {
 // and run there). The callback must be destroyed on the same thread where it
 // was run (but can be destroyed with or without the proxy lock acquired). Or
 // (3) destroyed without the proxy lock acquired.
-// TODO(dmichael): This won't actually fail until
-//                 https://codereview.chromium.org/19492014/ lands.
 template <class FunctionType>
 inline base::Callback<FunctionType>
 RunWhileLocked(const base::Callback<FunctionType>& callback) {