PPAPI: Make CompletionCallbacks work right on background threads.

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 65d94be8240b747cab103a155b68a438dcbaf903..05bc87ceac55d0cc87d6c2e94266c2beb69ea5df 100644
--- a/ppapi/shared_impl/tracked_callback.cc
+++ b/ppapi/shared_impl/tracked_callback.cc
@@ -14,11 +14,21 @@
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/shared_impl/callback_tracker.h"
 #include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/ppb_message_loop_shared.h"
 #include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource.h"
 
 namespace ppapi {
 
+namespace {
+
+bool IsMainThread() {
+  return
+      PpapiGlobals::Get()->GetMainThreadMessageLoop()->BelongsToCurrentThread();
+}
+
+}  // namespace
+
 // TrackedCallback -------------------------------------------------------------
 
 // Note: don't keep a Resource* since it may go out of scope before us.
@@ -30,7 +40,12 @@ TrackedCallback::TrackedCallback(
       completed_(false),
       aborted_(false),
       callback_(callback),
+      target_loop_(PpapiGlobals::Get()->GetCurrentMessageLoop()),
       result_for_blocked_callback_(PP_OK) {
+  // Note that target_loop_ may be NULL at this point, if the plugin has not
+  // attached a loop to this thread, or if this is an in-process plugin.
+  // The Enter class should handle checking this for us.
+
   // TODO(dmichael): Add tracking at the instance level, for callbacks that only
   // have an instance (e.g. for MouseLock).
   if (resource) {
@@ -40,19 +55,27 @@ TrackedCallback::TrackedCallback(
   }
 
   base::Lock* proxy_lock = PpapiGlobals::Get()->GetProxyLock();
-  // We only need a ConditionVariable if the lock is valid (i.e., we're out-of-
-  // process) and the callback is blocking.
-  if (proxy_lock && is_blocking())
-    operation_completed_condvar_.reset(new base::ConditionVariable(proxy_lock));
+  if (proxy_lock) {
+    // If the proxy_lock is valid, we're running out-of-process, and locking
+    // is enabled.
+    if (is_blocking()) {
+      // This is a blocking completion callback, so we will need a condition
+      // variable for blocking & signalling the calling thread.
+      operation_completed_condvar_.reset(
+          new base::ConditionVariable(proxy_lock));
+    } else {
+      // It's a non-blocking callback, so we should have a MessageLoopResource
+      // to dispatch to. Note that we don't error check here, though. Later,
+      // EnterResource::SetResult will check to make sure the callback is valid
+      // and take appropriate action.
+    }
+  }
 }
 
 TrackedCallback::~TrackedCallback() {
 }
 
 void TrackedCallback::Abort() {
-  // It doesn't make sense to abort a callback that's not associated with a
-  // resource.
-  DCHECK(resource_id_);
   Run(PP_ERROR_ABORTED);
 }
 
@@ -71,13 +94,11 @@ void TrackedCallback::Run(int32_t result) {
   if (result == PP_ERROR_ABORTED)
     aborted_ = true;
 
-  // Copy |callback_| and look at |aborted()| now, since |MarkAsCompleted()|
-  // may delete us.
-  PP_CompletionCallback callback = callback_;
   // Note that this call of Run() may have been scheduled prior to Abort() or
   // PostAbort() being called. If we have been told to Abort, that always
-  // trumps a result that was scheduled before.
-  if (aborted())
+  // trumps a result that was scheduled before, so we should make sure to pass
+  // PP_ERROR_ABORTED.
+  if (aborted() && (result != PP_ERROR_ABORTED))

[brettw, 2012/11/06 23:16:28]

I would have probably kept the previous code here (omitted the != check since
it's redundant).

[dmichael (off chromium), 2012/11/07 18:44:34]

Done. I was trying to make it look less weird with the stuff on 94 and 95, but I
guess it didn't help. I think the code is right, it just looks wrongish at first
glance.

     result = PP_ERROR_ABORTED;
 
   if (is_blocking()) {
@@ -100,6 +121,16 @@ void TrackedCallback::Run(int32_t result) {
     // Wait()s.
     operation_completed_condvar_->Signal();
   } 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_ &&
+        target_loop_ != PpapiGlobals::Get()->GetCurrentMessageLoop()) {
+      PostRun(result);
+      return;
+    }
+    // Copy |callback_| and look at |aborted()| now, since |MarkAsCompleted()|

[brettw, 2012/11/06 23:16:28]

You mention aborted() here but I don't see anything in the code corresponding to
this.

[dmichael (off chromium), 2012/11/07 18:44:34]

Copy paste error; fixed.

+    // may delete us.
+    PP_CompletionCallback callback = callback_;
     // Do this before running the callback in case of reentrancy (which
     // shouldn't happen, but avoid strange failures).
     MarkAsCompleted();
@@ -122,7 +153,15 @@ void TrackedCallback::PostRun(int32_t result) {
 
   base::Closure callback_closure(
       RunWhileLocked(base::Bind(&TrackedCallback::Run, this, result)));
-  MessageLoop::current()->PostTask(FROM_HERE, callback_closure);
+  if (!target_loop_) {
+    // 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());
+    MessageLoop::current()->PostTask(FROM_HERE, callback_closure);
+  } else {
+    target_loop_->PostClosure(FROM_HERE, callback_closure, 0);
+  }
   is_scheduled_ = true;
 }