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

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@222829 0039d316-1c4b-4281-b951-d872f2087c98

21 files changed, 182 insertions, 60 deletions

diff --git a/content/renderer/pepper/host_globals.cc b/content/renderer/pepper/host_globals.cc
index 64a989c..e35ea32 100644
--- 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
index 2f67b9e..761fa91 100644
--- 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
index b228cd7..caa7075 100644
--- 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
index 080c884..d884665 100644
--- 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
index 095d70c..e39f9df 100644
--- a/content/renderer/pepper/v8_var_converter_unittest.cc
+++ b/content/renderer/pepper/v8_var_converter_unittest.cc
@@ -159,7 +159,7 @@ class V8VarConverterTest : public testing::Test {
 
   // testing::Test implementation.
   virtual void SetUp() {
-    ProxyLock::Acquire();
+    ppapi::ProxyLock::DisableLockingOnThreadForTest();
     v8::HandleScope handle_scope(isolate_);
     v8::Handle<v8::ObjectTemplate> global = v8::ObjectTemplate::New();
     context_.Reset(isolate_, v8::Context::New(isolate_, NULL, global));
diff --git a/ppapi/proxy/plugin_globals.cc b/ppapi/proxy/plugin_globals.cc
index 07ac688..83217f0 100644
--- 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
index 044cac7..31adef5 100644
--- 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
index 5e4b67b..87aa22b 100644
--- 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
index 2ee6914..c45ca9d 100644
--- 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
index 66275e5..fdc939a 100644
--- 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
index 7b9bc91..c2ca701 100644
--- a/ppapi/shared_impl/proxy_lock.cc
+++ b/ppapi/shared_impl/proxy_lock.cc
@@ -11,14 +11,32 @@
 
 namespace ppapi {
 
+base::LazyInstance<base::Lock>::Leaky
+    g_proxy_lock = LAZY_INSTANCE_INITIALIZER;
+
+bool g_disable_locking = false;
+base::ThreadLocalBoolean g_disable_locking_for_thread;
+
 // 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())
+    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 +49,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 +62,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 +72,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.Set(true);
+}
+
+// static
+void ProxyLock::EnableLockingOnThreadForTest() {
+  g_disable_locking_for_thread.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
index 9352471..9a81a13 100644
--- 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) {
diff --git a/ppapi/shared_impl/resource_tracker_unittest.cc b/ppapi/shared_impl/resource_tracker_unittest.cc
index fb00d76..3c7f901 100644
--- a/ppapi/shared_impl/resource_tracker_unittest.cc
+++ b/ppapi/shared_impl/resource_tracker_unittest.cc
@@ -5,6 +5,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 #include "base/compiler_specific.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/shared_impl/test_globals.h"
@@ -59,6 +60,7 @@ class ResourceTrackerTest : public testing::Test {
 // deleted but the object lives on.
 TEST_F(ResourceTrackerTest, LastPluginRef) {
   PP_Instance instance = 0x1234567;
+  ProxyAutoLock lock;
   resource_tracker().DidCreateInstance(instance);
 
   scoped_refptr<MyMockResource> resource(new MyMockResource(instance));
@@ -81,6 +83,7 @@ TEST_F(ResourceTrackerTest, LastPluginRef) {
 TEST_F(ResourceTrackerTest, InstanceDeletedWithPluginRef) {
   // Make a resource with one ref held by the plugin, and delete the instance.
   PP_Instance instance = 0x2345678;
+  ProxyAutoLock lock;
   resource_tracker().DidCreateInstance(instance);
   MyMockResource* resource = new MyMockResource(instance);
   resource->GetReference();
@@ -99,6 +102,7 @@ TEST_F(ResourceTrackerTest, InstanceDeletedWithPluginRef) {
 TEST_F(ResourceTrackerTest, InstanceDeletedWithBothRefed) {
   // Create a new instance.
   PP_Instance instance = 0x3456789;
+  ProxyAutoLock lock;
 
   // Make a resource with one ref held by the plugin and one ref held by us
   // (outlives the plugin), and delete the instance.
diff --git a/ppapi/shared_impl/test_globals.cc b/ppapi/shared_impl/test_globals.cc
index 133b943..f4d58cf 100644
--- a/ppapi/shared_impl/test_globals.cc
+++ b/ppapi/shared_impl/test_globals.cc
@@ -55,10 +55,6 @@ std::string TestGlobals::GetCmdLine() {
 void TestGlobals::PreCacheFontForFlash(const void* /* logfontw */) {
 }
 
-base::Lock* TestGlobals::GetProxyLock() {
-  return NULL;
-}
-
 void TestGlobals::LogWithSource(PP_Instance instance,
                                 PP_LogLevel level,
                                 const std::string& source,
diff --git a/ppapi/shared_impl/test_globals.h b/ppapi/shared_impl/test_globals.h
index 082af34..709438e 100644
--- a/ppapi/shared_impl/test_globals.h
+++ b/ppapi/shared_impl/test_globals.h
@@ -61,7 +61,6 @@ class TestGlobals : 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,
diff --git a/ppapi/shared_impl/tracked_callback.cc b/ppapi/shared_impl/tracked_callback.cc
index 23e9538..f3b8bb9 100644
--- a/ppapi/shared_impl/tracked_callback.cc
+++ b/ppapi/shared_impl/tracked_callback.cc
@@ -62,7 +62,7 @@ TrackedCallback::TrackedCallback(
     tracker_->Add(make_scoped_refptr(this));
   }
 
-  base::Lock* proxy_lock = PpapiGlobals::Get()->GetProxyLock();
+  base::Lock* proxy_lock = ProxyLock::Get();
   if (proxy_lock) {
     // If the proxy_lock is valid, we're running out-of-process, and locking
     // is enabled.
diff --git a/ppapi/shared_impl/tracked_callback_unittest.cc b/ppapi/shared_impl/tracked_callback_unittest.cc
index 9ceb96e..55db0aa 100644
--- a/ppapi/shared_impl/tracked_callback_unittest.cc
+++ b/ppapi/shared_impl/tracked_callback_unittest.cc
@@ -8,6 +8,7 @@
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/shared_impl/callback_tracker.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/shared_impl/test_globals.h"
@@ -26,9 +27,12 @@ class TrackedCallbackTest : public testing::Test {
   PP_Instance pp_instance() const { return pp_instance_; }
 
   virtual void SetUp() OVERRIDE {
+    ProxyLock::EnableLockingOnThreadForTest();
+    ProxyAutoLock lock;
     globals_.GetResourceTracker()->DidCreateInstance(pp_instance_);
   }
   virtual void TearDown() OVERRIDE {
+    ProxyAutoLock lock;
     globals_.GetResourceTracker()->DidDeleteInstance(pp_instance_);
   }
 
@@ -89,6 +93,7 @@ class CallbackShutdownTest : public TrackedCallbackTest {
 
 // Tests that callbacks are properly aborted on module shutdown.
 TEST_F(CallbackShutdownTest, AbortOnShutdown) {
+  ProxyAutoLock lock;
   scoped_refptr<Resource> resource(new Resource(OBJECT_IS_IMPL, pp_instance()));
 
   // Set up case (1) (see above).
@@ -250,6 +255,7 @@ class CallbackMockResource : public Resource {
 
 // Test that callbacks get aborted on the last resource unref.
 TEST_F(CallbackResourceTest, AbortOnNoRef) {
+  ProxyAutoLock lock;
   ResourceTracker* resource_tracker =
       PpapiGlobals::Get()->GetResourceTracker();
 
@@ -273,23 +279,33 @@ TEST_F(CallbackResourceTest, AbortOnNoRef) {
   // Kill resource #1, spin the message loop to run posted calls, and check that
   // things are in the expected states.
   resource_tracker->ReleaseResource(resource_1_id);
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
   resource_1->CheckFinalState();
   resource_2->CheckIntermediateState();
 
   // Kill resource #2.
   resource_tracker->ReleaseResource(resource_2_id);
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
   resource_1->CheckFinalState();
   resource_2->CheckFinalState();
 
   // This shouldn't be needed, but make sure there are no stranded tasks.
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
 }
 
 // Test that "resurrecting" a resource (getting a new ID for a |Resource|)
 // doesn't resurrect callbacks.
 TEST_F(CallbackResourceTest, Resurrection) {
+  ProxyAutoLock lock;
   ResourceTracker* resource_tracker =
       PpapiGlobals::Get()->GetResourceTracker();
 
@@ -300,21 +316,33 @@ TEST_F(CallbackResourceTest, Resurrection) {
   // Unref it, spin the message loop to run posted calls, and check that things
   // are in the expected states.
   resource_tracker->ReleaseResource(resource_id);
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
   resource->CheckFinalState();
 
   // "Resurrect" it and check that the callbacks are still dead.
   PP_Resource new_resource_id = resource->GetReference();
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
   resource->CheckFinalState();
 
   // Unref it again and do the same.
   resource_tracker->ReleaseResource(new_resource_id);
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
   resource->CheckFinalState();
 
   // This shouldn't be needed, but make sure there are no stranded tasks.
-  base::MessageLoop::current()->RunUntilIdle();
+  {
+    ProxyAutoUnlock unlock;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
 }
 
 }  // namespace ppapi
diff --git a/ppapi/shared_impl/var_tracker_unittest.cc b/ppapi/shared_impl/var_tracker_unittest.cc
index 23e2f59..0fe1a03 100644
--- a/ppapi/shared_impl/var_tracker_unittest.cc
+++ b/ppapi/shared_impl/var_tracker_unittest.cc
@@ -5,6 +5,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 #include "base/compiler_specific.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/var.h"
 #include "ppapi/shared_impl/var_tracker.h"
 #include "ppapi/shared_impl/test_globals.h"
@@ -53,6 +54,7 @@ class VarTrackerTest : public testing::Test {
   // Test implementation.
   virtual void SetUp() OVERRIDE {
     ASSERT_EQ(0, mock_var_alive_count);
+    ProxyLock::EnableLockingOnThreadForTest();
   }
   virtual void TearDown() OVERRIDE {
   }
@@ -66,6 +68,7 @@ class VarTrackerTest : public testing::Test {
 // Test that ResetVarID is called when the last PP_Var ref was deleted but the
 // object lives on.
 TEST_F(VarTrackerTest, LastResourceRef) {
+  ProxyAutoLock lock;
   scoped_refptr<MockStringVar> var(new MockStringVar(std::string("xyz")));
   PP_Var pp_var = var->GetPPVar();
   EXPECT_TRUE(var->HasValidVarID());
@@ -82,6 +85,7 @@ TEST_F(VarTrackerTest, LastResourceRef) {
 }
 
 TEST_F(VarTrackerTest, GetPluginRefAgain) {
+  ProxyAutoLock lock;
   scoped_refptr<MockStringVar> var(new MockStringVar(std::string("xyz")));
   PP_Var pp_var = var->GetPPVar();
   EXPECT_TRUE(var_tracker().ReleaseVar(pp_var));
@@ -112,6 +116,7 @@ TEST_F(VarTrackerTest, GetPluginRefAgain) {
 // Tests when the plugin is holding a ref to a PP_Var when the instance is
 // owned only by VarTracker.
 TEST_F(VarTrackerTest, PluginRefWithoutVarRef) {
+  ProxyAutoLock lock;
   // Make a PP_Var with one ref held by the plugin, and release the reference.
   scoped_refptr<MockStringVar> var(new MockStringVar(std::string("zzz")));
   PP_Var pp_var = var->GetPPVar();
@@ -129,6 +134,7 @@ TEST_F(VarTrackerTest, PluginRefWithoutVarRef) {
 
 // Tests on Var having type of PP_VARTYPE_OBJECT.
 TEST_F(VarTrackerTest, ObjectRef) {
+  ProxyAutoLock lock;
   scoped_refptr<MockObjectVar> var(new MockObjectVar());
   PP_Var pp_var = var->GetPPVar();
   EXPECT_TRUE(var_tracker().ReleaseVar(pp_var));
diff --git a/ppapi/shared_impl/var_value_conversions_unittest.cc b/ppapi/shared_impl/var_value_conversions_unittest.cc
index 88d645a..f8a0a31 100644
--- a/ppapi/shared_impl/var_value_conversions_unittest.cc
+++ b/ppapi/shared_impl/var_value_conversions_unittest.cc
@@ -142,6 +142,7 @@ class VarValueConversionsTest : public testing::Test {
 
   // testing::Test implementation.
   virtual void SetUp() {
+    ProxyLock::EnableLockingOnThreadForTest();
     ProxyLock::Acquire();
   }
   virtual void TearDown() {
diff --git a/ppapi/thunk/enter.cc b/ppapi/thunk/enter.cc
index bd955b2..47889dd 100644
--- a/ppapi/thunk/enter.cc
+++ b/ppapi/thunk/enter.cc
@@ -29,14 +29,6 @@ namespace thunk {
 
 namespace subtle {
 
-void AssertLockHeld() {
-  base::Lock* proxy_lock = PpapiGlobals::Get()->GetProxyLock();
-  // The lock is only valid in the plugin side of the proxy, so it only makes
-  // sense to assert there. Otherwise, silently succeed.
-  if (proxy_lock)
-    proxy_lock->AssertAcquired();
-}
-
 EnterBase::EnterBase()
     : resource_(NULL),
       retval_(PP_OK) {
diff --git a/ppapi/thunk/enter.h b/ppapi/thunk/enter.h
index a5a0072..4641cbb 100644
--- a/ppapi/thunk/enter.h
+++ b/ppapi/thunk/enter.h
@@ -43,9 +43,6 @@ namespace thunk {
 
 namespace subtle {
 
-// Assert that we are holding the proxy lock.
-PPAPI_THUNK_EXPORT void AssertLockHeld();
-
 // This helps us define our RAII Enter classes easily. To make an RAII class
 // which locks the proxy lock on construction and unlocks on destruction,
 // inherit from |LockOnEntry<true>| before all other base classes. This ensures
@@ -63,12 +60,12 @@ struct LockOnEntry<false> {
 #if (!NDEBUG)
   LockOnEntry() {
     // You must already hold the lock to use Enter*NoLock.
-    AssertLockHeld();
+    ProxyLock::AssertAcquired();
   }
   ~LockOnEntry() {
     // You must not release the lock before leaving the scope of the
     // Enter*NoLock.
-    AssertLockHeld();
+    ProxyLock::AssertAcquired();
   }
 #endif
 };