PPAPI: Make CompletionCallbacks work right on background threads.

ppapi/thunk/enter.cc

Index: ppapi/thunk/enter.cc
diff --git a/ppapi/thunk/enter.cc b/ppapi/thunk/enter.cc
index c89c9695b5a09b4486510d0e9be5430aeea6baaa..296a8b4a7615726475cb708d769f1225e2b1de4b 100644
--- a/ppapi/thunk/enter.cc
+++ b/ppapi/thunk/enter.cc
@@ -87,8 +87,6 @@ int32_t EnterBase::SetResult(int32_t result) {
     // The function completed synchronously.
     if (callback_->is_required()) {
       // This is a required callback, so we must issue it asynchronously.
-      // TODO(dmichael) make this work so that a call from a background thread
-      // goes back to that thread.
       callback_->PostRun(result);
       retval_ = PP_OK_COMPLETIONPENDING;
     } else {
@@ -108,23 +106,52 @@ Resource* EnterBase::GetResource(PP_Resource resource) {
 }
 
 void EnterBase::SetStateForCallbackError(bool report_error) {
-  if (!CallbackIsValid()) {
-    callback_->MarkAsCompleted();
-    callback_ = NULL;
-    retval_ = PP_ERROR_BLOCKS_MAIN_THREAD;
-    if (report_error) {
-      std::string message(
-          "Blocking callbacks are not allowed on the main thread.");
-      PpapiGlobals::Get()->BroadcastLogWithSource(0, PP_LOGLEVEL_ERROR,
-                                                  std::string(), message);
-    }
+  if (PpapiGlobals::Get()->IsHostGlobals()) {
+    // In-process plugins can't make PPAPI calls off the main thread.
+    CHECK(IsMainThread());

[dmichael (off chromium), 2012/11/06 22:49:39]

^^^ I'm mildly afraid this could break internal plugins that are still
in-process (Chromoting... not sure what else). But if this gets hit, they're
clearly doing something wrong. And it makes my logic simpler.

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

I'm happy with this.

   }
-}
+  if (callback_) {
+    if (callback_->is_blocking() && IsMainThread()) {
+      // Blocking callbacks are never allowed on the main thread.
+      callback_->MarkAsCompleted();
+      callback_ = NULL;
+      retval_ = PP_ERROR_BLOCKS_MAIN_THREAD;
+      if (report_error) {
+        std::string message(
+            "Blocking callbacks are not allowed on the main thread.");
+        PpapiGlobals::Get()->BroadcastLogWithSource(0, PP_LOGLEVEL_ERROR,
+                                                    std::string(), message);
+      }
+    } else if (!IsMainThread() &&
+               callback_->has_null_target_loop() &&
+               !callback_->is_blocking()) {
+      // On a non-main thread, there must be a valid target loop for non-
+      // blocking callbacks, or we will have no place to run them.
+
+      // If the callback is required, there's no nice way to tell the plugin.
+      // We can't run their callback asynchronously without a message loop, and
+      // the plugin won't expect any return code other than
+      // PP_OK_COMPLETIONPENDING. So we crash to make the problem more obvious.

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

This contradicts the documentation. I'm also wondering if it's best to crash.
From a plugin author's perspective, if something isn't working, I start looking
at error codes. If a function call is crashing, I'm perplexed and think it's a
bug in the system. So I think I prefer the documented behavior better than
crashing here.

[dmichael (off chromium), 2012/11/06 23:39:26]

This is *only* for required callbacks. They're not expecting anything but
PP_OK_COMPLETIONPENDING, and might not even be looking for it. This is the
resolution we came up with here:
https://groups.google.com/a/chromium.org/d/topic/pepper-dev/dSHfzSFPyuA/discu...
But I'm happy to revisit if you want.

[brettw, 2012/11/07 00:02:21]

Okay, I don't care that much and since this was already discussed, we can keep
the crash. But we should update the message loop documentation which mentions
this case explicitly.

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

I didn't see anything wrong in the message loop documentation. It mentions
required callbacks, but it's in the context of PostWork, where the flags are
ignored. I think that documentation is still right.

The case I'm handling here is where a method taking a callback is invoked on a
background thread, and passed a required callback. The caller always expects
PP_OK_COMPLETIONPENDING in this case, and for us to run the callback later (even
in case of error). But we have no place to run it, and returning an error code
might not help (they might not even bother checking it). Hence why we crash.

I did find that PP_CompletionCallback's documentation was wrong in other ways,
and made no mention of this case. So I tried to clean that up.

[brettw, 2012/11/07 18:52:15]

You're right, I was looking at PostWork.

+      if (callback_->is_required()) {
+        std::string message("Attempted to use a required callback, but there"
+                            "is no attached message loop on which to run the"
+                            "callback.");
+        PpapiGlobals::Get()->BroadcastLogWithSource(0, PP_LOGLEVEL_ERROR,
+                                                    std::string(), message);
+        LOG(FATAL) << message;

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

Does this do the right thing in NaCl? I'm not positive fatal assertions kill the
plugin.

[dmichael (off chromium), 2012/11/06 23:39:26]

It looks like it to me in base/logging and base/debug/debugger_posix.cc (CHECK()
becomes LOG(FATAL) which calls BreakDebugger which calls abort() on NaCl).
Should kill the nexe.

I can slap together a local test to confirm it.

+      }
 
-bool EnterBase::CallbackIsValid() const {
-  // A callback is only considered invalid if it is blocking and we're on the
-  // main thread.
-  return !callback_ || !callback_->is_blocking() || !IsMainThread();
+      callback_->MarkAsCompleted();
+      callback_ = NULL;
+      retval_ = PP_ERROR_NO_MESSAGE_LOOP;
+      if (report_error) {
+        std::string message(
+            "The calling thread must have a message loop attached.");
+        PpapiGlobals::Get()->BroadcastLogWithSource(0, PP_LOGLEVEL_ERROR,
+                                                    std::string(), message);
+      }
+    }
+  }
 }
 
 void EnterBase::ClearCallback() {
@@ -145,8 +172,6 @@ void EnterBase::SetStateForResourceError(PP_Resource pp_resource,
     return;  // Everything worked.
 
   if (callback_ && callback_->is_required()) {
-    // TODO(dmichael) make this work so that a call from a background thread
-    // goes back to that thread.
     callback_->PostRun(static_cast<int32_t>(PP_ERROR_BADRESOURCE));
     callback_ = NULL;
     retval_ = PP_OK_COMPLETIONPENDING;