diff --git a/content/renderer/pepper/host_globals.cc b/content/renderer/pepper/host_globals.cc index 64a989ca616f0c..e35ea320791e69 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 2f67b9e23603bd..761fa915743eb4 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 b228cd72f5b1de..caa70752eed54b 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 080c884726d625..d884665ba587cc 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 195210afed8eae..878c39bc623c8d 100644 --- 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 index 07ac68856018a4..83217f0d96ba05 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 044cac7e25c049..31adef558412d4 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 callback_tracker_; - base::Lock proxy_lock_; - scoped_ptr 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 5e4b67bc4f3150..87aa22b2860a7f 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 2ee69144f61cd0..c45ca9d6967ec5 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 66275e50a79665..fdc939a2880bae 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 7b9bc9137c1ef9..3c1c3473ae420b 100644 --- a/ppapi/shared_impl/proxy_lock.cc +++ b/ppapi/shared_impl/proxy_lock.cc @@ -11,14 +11,33 @@ namespace ppapi { +base::LazyInstance::Leaky + g_proxy_lock = LAZY_INSTANCE_INITIALIZER; + +bool g_disable_locking = false; +base::LazyInstance::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::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 index 935247189813b1..9a81a13baa1366 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 { } } + ~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 callback_; @@ -188,7 +243,13 @@ class RunWhileLockedHelper { temp_callback->Run(p1); } } - + ~RunWhileLockedHelper() { + DCHECK(thread_checker_.CalledOnValidThread()); + if (callback_) { + ProxyAutoLock lock; + callback_.reset(); + } + } private: scoped_ptr callback_; base::ThreadChecker thread_checker_; @@ -211,7 +272,13 @@ class RunWhileLockedHelper { temp_callback->Run(p1, p2); } } - + ~RunWhileLockedHelper() { + DCHECK(thread_checker_.CalledOnValidThread()); + if (callback_) { + ProxyAutoLock lock; + callback_.reset(); + } + } private: scoped_ptr callback_; base::ThreadChecker thread_checker_; @@ -234,7 +301,13 @@ class RunWhileLockedHelper { temp_callback->Run(p1, p2, p3); } } - + ~RunWhileLockedHelper() { + DCHECK(thread_checker_.CalledOnValidThread()); + if (callback_) { + ProxyAutoLock lock; + callback_.reset(); + } + } private: scoped_ptr callback_; base::ThreadChecker thread_checker_; @@ -270,8 +343,6 @@ class RunWhileLockedHelper { // 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 inline base::Callback RunWhileLocked(const base::Callback& callback) { diff --git a/ppapi/shared_impl/resource_tracker_unittest.cc b/ppapi/shared_impl/resource_tracker_unittest.cc index fb00d761d6c334..3c7f90167cc8ed 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 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 133b9437dd0b9d..f4d58cfd8d09cd 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 082af34b26c2de..709438ec0991c7 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 23e953851ca522..f3b8bb9820d705 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 9ceb96e7cf7734..55db0aaf93ee5f 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(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 23e2f59d94872d..0fe1a03b39686e 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 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 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 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 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 88d645ad19769b..f8a0a3129e8cf1 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 bd955b2ca8f81d..47889dd15776c7 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 a5a00720fe0fd4..4641cbb626468b 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| before all other base classes. This ensures @@ -63,12 +60,12 @@ struct LockOnEntry { #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 };