Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(264)

Unified Diff: ppapi/shared_impl/proxy_lock.h

Issue 19492014: PPAPI: Purposely leak ProxyLock, fix shutdown race (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: test cleanup Created 7 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: ppapi/shared_impl/proxy_lock.h
diff --git a/ppapi/shared_impl/proxy_lock.h b/ppapi/shared_impl/proxy_lock.h
index 935247189813b18552749c852770140fb080d512..389cbd279677f8dc8707af1f5508c5f70d055a55 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,11 @@ namespace ppapi {
// tracker, etc.
class PPAPI_SHARED_EXPORT ProxyLock {
public:
+ // Return the globally unique ProxyLock. Normally, you should not access this
bbudge 2013/07/29 21:02:18 s/globally unique/global ?
dmichael (off chromium) 2013/07/29 21:24:03 Done.
+ // directly but instead use ProxyAutoLock or ProxyAutoUnlock. But sometimes
+ // you need access to the ProxyLock for e.g. a condition variable.
bbudge 2013/07/29 21:02:18 s/ for e.g./e.g. to create
dmichael (off chromium) 2013/07/29 21:24:03 Done.
+ 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 +50,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();
+
+ // Locking will be on by default, but unit tests should *still* call this if
+ // they rely on the lock being enabled, because unit tests run in the same
+ // process in succession, and a previous test may have disabled locking.
+ static void EnableLockingOnThreadForTest();
bbudge 2013/07/29 21:02:18 This is hard to understand. How about this? // En
dmichael (off chromium) 2013/07/29 21:24:03 Much better, thanks
+
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 +189,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 +242,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 +271,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 +300,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 +342,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) {

Powered by Google App Engine
This is Rietveld 408576698