PPAPI: Make TrackedCallback more threadsafe

ppapi/shared_impl/tracked_callback.cc

Index: ppapi/shared_impl/tracked_callback.cc
diff --git a/ppapi/shared_impl/tracked_callback.cc b/ppapi/shared_impl/tracked_callback.cc
index 7ddfd39c13cb9b2edc626146d64e98238c97f6e9..7a2122bf746c74458d5ed709fe7da56472d3c4f4 100644
--- a/ppapi/shared_impl/tracked_callback.cc
+++ b/ppapi/shared_impl/tracked_callback.cc
@@ -30,12 +30,19 @@ bool IsMainThread() {
 
 int32_t RunCompletionTask(TrackedCallback::CompletionTask completion_task,
                           int32_t result) {
+  ProxyLock::AssertAcquired();
   int32_t task_result = completion_task.Run(result);
   if (result != PP_ERROR_ABORTED)
     result = task_result;
   return result;
 }
 
+void AcquireProxyLockAndRun(const base::Closure& closure) {
+  ProxyAutoLock acquire;
+  closure.Run();
+}
+
+
 }  // namespace
 
 // TrackedCallback -------------------------------------------------------------
@@ -64,6 +71,7 @@ TrackedCallback::TrackedCallback(Resource* resource,
 
   base::Lock* proxy_lock = ProxyLock::Get();
   if (proxy_lock) {
+    ProxyLock::AssertAcquired();
     // If the proxy_lock is valid, we're running out-of-process, and locking
     // is enabled.
     if (is_blocking()) {
@@ -82,17 +90,26 @@ TrackedCallback::TrackedCallback(Resource* resource,
 
 TrackedCallback::~TrackedCallback() {}
 
-void TrackedCallback::Abort() { Run(PP_ERROR_ABORTED); }
+void TrackedCallback::Abort() {
+  Run(PP_ERROR_ABORTED);
+}
 
-void TrackedCallback::PostAbort() { PostRun(PP_ERROR_ABORTED); }
+void TrackedCallback::PostAbort() {
+  PostRun(PP_ERROR_ABORTED);
+}
 
 void TrackedCallback::Run(int32_t result) {
+  // Retain ourselves, since SignalBlockingCallback and MarkAsCompleted might
+  // otherwise cause |this| to be deleted. Do this before acquiring lock_ so
+  // that |this| is definitely valid at the time we release |lock_|.
+  scoped_refptr<TrackedCallback> thiz(this);
+  base::AutoLock acquire(lock_);
   // Only allow the callback to be run once. Note that this also covers the case
   // where the callback was previously Aborted because its associated Resource
   // went away. The callback may live on for a while because of a reference from
   // a Closure. But when the Closure runs, Run() quietly does nothing, and the
   // callback will go away when all referring Closures go away.
-  if (completed())
+  if (completed_)
     return;
   if (result == PP_ERROR_ABORTED)

[bbudge, 2015/02/25 23:51:31]

Would a DCHECK(!aborted_) be useful here?

[dmichael (off chromium), 2015/02/27 22:04:40]

No :(

aborted_ can be set in PostRunWithLock. The idea is that any attempt to Abort()
or PostAbort() the callback should take precedence over any other value sent to
Run() or PostRun(). So imagine that something like this happens:
1) PostRun(PP_OK)
2) Resource is deleted, so we do PostAbort()
3) The "Run(PP_OK)" task runs before the RUN(PP_ERROR_ABORTED)

In step 2, we will have set aborted_, to make sure when we get to step 3 we can
override the result (see comment just below here).

Sorry, it's kind of confusing. There might be a nicer way to have done that. 

[bbudge, 2015/03/05 01:29:34]

OK, makes sense.

     aborted_ = true;
@@ -101,87 +118,43 @@ void TrackedCallback::Run(int32_t result) {
   // PostAbort() being called. If we have been told to Abort, that always
   // trumps a result that was scheduled before, so we should make sure to pass
   // PP_ERROR_ABORTED.
-  if (aborted())
+  if (aborted_)
     result = PP_ERROR_ABORTED;
 
   if (is_blocking()) {
-    // If the condition variable is invalid, there are two possibilities. One,
-    // we're running in-process, in which case the call should have come in on
-    // the main thread and we should have returned PP_ERROR_BLOCKS_MAIN_THREAD
-    // well before this. Otherwise, this callback was not created as a
-    // blocking callback. Either way, there's some internal error.
-    if (!operation_completed_condvar_.get()) {
-      NOTREACHED();
-      return;
-    }
-    result_for_blocked_callback_ = result;
-    // Retain ourselves, since MarkAsCompleted will remove us from the
-    // tracker. Then MarkAsCompleted before waking up the blocked thread,
-    // which could potentially re-enter.
-    scoped_refptr<TrackedCallback> thiz(this);
-    MarkAsCompleted();
-    // Wake up the blocked thread. See BlockUntilComplete for where the thread
-    // Wait()s.
-    operation_completed_condvar_->Signal();
+    // This is a blocking callback; signal the condvar to wake up the thread.
+    SignalBlockingCallback(result);
   } else {
     // If there's a target_loop_, and we're not on the right thread, we need to
     // post to target_loop_.
     if (target_loop_.get() &&
         target_loop_.get() != PpapiGlobals::Get()->GetCurrentMessageLoop()) {
-      PostRun(result);
+      PostRunImpl(result);
       return;
     }
-
-    // Copy callback fields now, since |MarkAsCompleted()| may delete us.
-    PP_CompletionCallback callback = callback_;
-    CompletionTask completion_task = completion_task_;
-    completion_task_.Reset();
     // Do this before running the callback in case of reentrancy from running
-    // the completion task.
-    MarkAsCompleted();
+    // the completion callback.
+    MarkAsCompletedWithLock();
 
-    if (!completion_task.is_null())
-      result = RunCompletionTask(completion_task, result);
+    if (!completion_task_.is_null())
+      result = RunCompletionTask(completion_task_, result);
 
-    // TODO(dmichael): Associate a message loop with the callback; if it's not
-    // the same as the current thread's loop, then post it to the right loop.
-    CallWhileUnlocked(PP_RunCompletionCallback, &callback, result);
+    {
+      base::AutoUnlock release(lock_);
+      // Call the callback without lock_ and without the ProxyLock.
+      CallWhileUnlocked(PP_RunCompletionCallback, &callback_, result);
+    }
   }
 }
 
 void TrackedCallback::PostRun(int32_t result) {
-  if (completed()) {
-    NOTREACHED();
-    return;
-  }
-  if (result == PP_ERROR_ABORTED)
-    aborted_ = true;
-  // We might abort when there's already a scheduled callback, but callers
-  // should never try to PostRun more than once otherwise.
-  DCHECK(result == PP_ERROR_ABORTED || !is_scheduled_);
-
-  if (is_blocking()) {
-    // We might not have a MessageLoop to post to, so we must call Run()
-    // directly.
-    Run(result);
-  } else {
-    base::Closure callback_closure(
-        RunWhileLocked(base::Bind(&TrackedCallback::Run, this, result)));
-    if (target_loop_.get()) {
-      target_loop_->PostClosure(FROM_HERE, callback_closure, 0);
-    } else {
-      // We must be running in-process and on the main thread (the Enter
-      // classes protect against having a null target_loop_ otherwise).
-      DCHECK(IsMainThread());
-      DCHECK(PpapiGlobals::Get()->IsHostGlobals());
-      base::MessageLoop::current()->PostTask(FROM_HERE, callback_closure);
-    }
-  }
-  is_scheduled_ = true;
+  base::AutoLock acquire(lock_);
+  PostRunImpl(result);
 }
 
 void TrackedCallback::set_completion_task(
     const CompletionTask& completion_task) {
+  base::AutoLock acquire(lock_);
   DCHECK(completion_task_.is_null());
   completion_task_ = completion_task;
 }
@@ -191,32 +164,47 @@ bool TrackedCallback::IsPending(
     const scoped_refptr<TrackedCallback>& callback) {
   if (!callback.get())

[bbudge, 2015/02/25 23:51:31]

While you're here, could we eliminate all the .get()'s? They hurt readability a
tiny bit IMO.

[dmichael (off chromium), 2015/02/27 22:04:40]

Done, thanks

     return false;
-  if (callback->aborted())
+  base::AutoLock acquire(callback->lock_);
+  if (callback->aborted_)
     return false;
-  return !callback->completed();
+  return !callback->completed_;
 }
 
 // static
 bool TrackedCallback::IsScheduledToRun(
     const scoped_refptr<TrackedCallback>& callback) {
-  return IsPending(callback) && callback->is_scheduled_;
+  if (!callback.get())
+    return false;
+  base::AutoLock acquire(callback->lock_);
+  if (callback->aborted_)
+    return false;
+  return !callback->completed_ && callback->is_scheduled_;
 }
 
 int32_t TrackedCallback::BlockUntilComplete() {
-  // Note, we are already holding the proxy lock in all these methods, including
-  // this one (see ppapi/thunk/enter.cc for where it gets acquired).
+  // Note, we are already holding the proxy lock in this method and many others
+  // (see ppapi/thunk/enter.cc for where it gets acquired).
+  ProxyLock::AssertAcquired();
+  base::AutoLock acquire(lock_);
 
   // It doesn't make sense to wait on a non-blocking callback. Furthermore,
   // BlockUntilComplete should never be called for in-process plugins, where
   // blocking callbacks are not supported.
-  CHECK(operation_completed_condvar_.get());
-  if (!is_blocking() || !operation_completed_condvar_.get()) {
-    NOTREACHED();
-    return PP_ERROR_FAILED;
-  }
+  CHECK(is_blocking() && operation_completed_condvar_.get());
 
-  while (!completed())
+  while (!completed_) {
+    // Protect us from being deleted to ensure operation_completd_condvar_ is

[bbudge, 2015/02/25 23:51:31]

sp/completd

+    // available to wait on when we drop our lock.
+    scoped_refptr<TrackedCallback> thiz(this);
+    // Unlock our lock temporarily; any thread that tries to signal us will need
+    // the lock.
+    lock_.Release();
     operation_completed_condvar_->Wait();
+    // Note that the condvar has re-acquired the ProxyLock for us, and we will

[bbudge, 2015/02/25 23:51:31]

I had to think about this a bit before understanding: perhaps say "signaling the
condvar has re-acquired the ProxyLock"?

[dmichael (off chromium), 2015/02/27 22:04:40]

I tried rephrasing it; let me know if it's still confusing.

+    // re-acquire lock_ immediately after, preserving lock order.
+    ProxyLock::AssertAcquired();
+    lock_.Acquire();
+  }
 
   if (!completion_task_.is_null()) {
     result_for_blocked_callback_ =
@@ -227,7 +215,13 @@ int32_t TrackedCallback::BlockUntilComplete() {
 }
 
 void TrackedCallback::MarkAsCompleted() {
-  DCHECK(!completed());
+  base::AutoLock acquire(lock_);
+  MarkAsCompletedWithLock();
+}
+
+void TrackedCallback::MarkAsCompletedWithLock() {
+  lock_.AssertAcquired();
+  DCHECK(!completed_);
 
   // We will be removed; maintain a reference to ensure we won't be deleted
   // until we're done.
@@ -237,6 +231,68 @@ void TrackedCallback::MarkAsCompleted() {
   if (resource_id_)
     tracker_->Remove(thiz);
   tracker_ = NULL;
+  target_loop_ = NULL;
+}
+
+void TrackedCallback::PostRunImpl(int32_t result) {
+  lock_.AssertAcquired();
+  if (completed_) {
+    NOTREACHED();
+    return;
+  }
+  if (result == PP_ERROR_ABORTED)
+    aborted_ = true;

[bbudge, 2015/02/25 23:51:31]

DCHECK(!aborted_) ?

[dmichael (off chromium), 2015/02/27 22:04:40]

Some APIs will call PostAbort (e.g. if there's a "Close()" operation). The
callback could also get PostAbort if the resource is subsequently released. So I
don't think we can safely assert that.

+  // We might abort when there's already a scheduled callback, but callers
+  // should never try to PostRun more than once otherwise.
+  DCHECK(result == PP_ERROR_ABORTED || !is_scheduled_);
+
+  if (is_blocking()) {
+    // We might not have a MessageLoop to post to, so we must Signal
+    // directly.
+    SignalBlockingCallback(result);
+  } else {
+    // Note we can't use "RunWhileLocked" from proxy_lock.h, because it requires
+    // that the ProxyLock is held (to protect Resource and Var ref-counting).
+    // Here, we must not acquire the ProxyLock, because we may be on the IO
+    // thread. But we're not passing any parameters that would require
+    // the ProxyLock (such as Var or Resource).
+    base::Closure callback_closure(
+        base::Bind(&ppapi::AcquireProxyLockAndRun,
+            base::Bind(&TrackedCallback::Run, this, result)));
+    if (target_loop_.get()) {
+      target_loop_->PostClosure(FROM_HERE, callback_closure, 0);
+    } else {
+      // We must be running in-process and on the main thread (the Enter
+      // classes protect against having a null target_loop_ otherwise).
+      DCHECK(IsMainThread());
+      DCHECK(PpapiGlobals::Get()->IsHostGlobals());
+      base::MessageLoop::current()->PostTask(FROM_HERE, callback_closure);
+    }
+  }
+  is_scheduled_ = true;
+}
+
+void TrackedCallback::SignalBlockingCallback(int32_t result) {
+  lock_.AssertAcquired();
+  DCHECK(is_blocking());
+  if (!operation_completed_condvar_.get()) {
+    // If the condition variable is invalid, there are two possibilities. One,
+    // we're running in-process, in which case the call should have come in on
+    // the main thread and we should have returned PP_ERROR_BLOCKS_MAIN_THREAD
+    // well before this. Otherwise, this callback was not created as a
+    // blocking callback. Either way, there's some internal error.
+    NOTREACHED();
+    return;
+  }
+  result_for_blocked_callback_ = result;
+  // Retain ourselves, since MarkAsCompleted will remove us from the
+  // tracker. Then MarkAsCompleted before waking up the blocked thread,
+  // which could potentially re-enter.
+  scoped_refptr<TrackedCallback> thiz(this);
+  MarkAsCompletedWithLock();
+  // Wake up the blocked thread. See BlockUntilComplete for where the thread
+  // Wait()s.
+  operation_completed_condvar_->Signal();
 }
 
 }  // namespace ppapi