PPAPI: Make blocking completion callbacks work.

ppapi/thunk/enter.h

Index: ppapi/thunk/enter.h
diff --git a/ppapi/thunk/enter.h b/ppapi/thunk/enter.h
index 4172ce0e60b2863769946ee87e00ab866687742b..313923db7148d92906a1eb1e1c075aeb089805c8 100644
--- a/ppapi/thunk/enter.h
+++ b/ppapi/thunk/enter.h
@@ -5,7 +5,12 @@
 #ifndef PPAPI_THUNK_ENTER_H_
 #define PPAPI_THUNK_ENTER_H_
 
+#include <string>
+
 #include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "ppapi/c/pp_errors.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/shared_impl/api_id.h"
 #include "ppapi/shared_impl/function_group_base.h"
@@ -18,6 +23,8 @@
 #include "ppapi/thunk/resource_creation_api.h"
 
 namespace ppapi {
+class TrackedCallback;
+
 namespace thunk {
 
 // Enter* helper objects: These objects wrap a call from the C PPAPI into
@@ -53,7 +60,23 @@ struct LockOnEntry;
 
 template <>
 struct LockOnEntry<false> {
-// TODO(dmichael) assert the lock is held.
+#if (!NDEBUG)
+  LockOnEntry() {
+    // You must already hold the lock to use Enter*NoLock.
+    AssertLockHeld();
+  }
+  ~LockOnEntry() {
+    // You must not release the lock before leaving the scope of the
+    // Enter*NoLock.
+    AssertLockHeld();
+  }
+ private:
+  static void AssertLockHeld() {
+    base::Lock* proxy_lock = PpapiGlobals::Get()->GetProxyLock();
+    if (proxy_lock)
+      proxy_lock->AssertAcquired();
+  }
+#endif
 };
 
 template <>
@@ -70,10 +93,15 @@ struct LockOnEntry<true> {
 class PPAPI_THUNK_EXPORT EnterBase {
  public:
   EnterBase();
-  explicit EnterBase(const PP_CompletionCallback& callback);
+  explicit EnterBase(PP_Resource resource);
+  EnterBase(PP_Resource resource, const PP_CompletionCallback& callback);
   virtual ~EnterBase();
 
-  // Sets the result.
+  // Sets the result for calls that use a completion callback. It handles making
+  // sure that "Required" callbacks are scheduled to run asynchronously and
+  // "Blocking" callbacks cause the caller to block. (Interface implementations,
+  // therefore, should not do any special casing based on the type of the
+  // callback.)
   //
   // Returns the "retval()". This is to support the typical usage of
   //   return enter.SetResult(...);
@@ -83,6 +111,13 @@ class PPAPI_THUNK_EXPORT EnterBase {
   // Use this value as the return value for the function.
   int32_t retval() const { return retval_; }
 
+  // All failure conditions cause retval_ to be set to an appropriate error
+  // code.
+  bool succeeded() const { return retval_ == PP_OK; }
+  bool failed() const { return !succeeded(); }
+
+  const scoped_refptr<TrackedCallback>& callback() { return callback_; }
+
  protected:
   // Helper function to return a function group from a PP_Instance. Having this
   // code be in the non-templatized base keeps us from having to instantiate
@@ -92,14 +127,22 @@ class PPAPI_THUNK_EXPORT EnterBase {
   // Helper function to return a Resource from a PP_Resource. Having this
   // code be in the non-templatized base keeps us from having to instantiate
   // it in every template.
-  Resource* GetResource(PP_Resource resource) const;
+  static Resource* GetResource(PP_Resource resource);
+
+  // Checks whether the callback is valid (i.e., if it is either non-blocking,
+  // or blocking and we're on a background thread). If the callback is invalid,
+  // this will set retval_ = PP_ERROR_BLOCKS_MAIN_THREAD, and if report_error is
+  // set, it will log a message to the programmer.
+  void SetStateForCallbackError(bool report_error);
+  bool CallbackIsValid() const;
+  void ClearCallback();
 
   // Does error handling associated with entering a resource. The resource_base
   // is the result of looking up the given pp_resource. The object is the
-  // result of converting the base to the desired object (converted back to a
-  // Resource* so this function doesn't have to be templatized). The reason for
-  // passing both resource_base and object is that we can differentiate "bad
-  // resource ID" from "valid resource ID not of the currect type."
+  // result of converting the base to the desired object (converted to a void*
+  // so this function doesn't have to be templatized). The reason for passing
+  // both resource_base and object is that we can differentiate "bad resource
+  // ID" from "valid resource ID not of the correct type."
   //
   // This will set retval_ = PP_ERROR_BADRESOURCE if the object is invalid, and
   // if report_error is set, log a message to the programmer.
@@ -113,11 +156,15 @@ class PPAPI_THUNK_EXPORT EnterBase {
                                 void* object,
                                 bool report_error);
 
+  // For Enter objects that need a resource, we'll store a pointer to the
+  // Resource object so that we don't need to look it up more than once. For
+  // Enter objects with no resource, this will be NULL.
+  Resource* resource_;

[brettw, 2012/04/20 18:24:29]

I'm worried about this. We do a bunch of stuff on destruction of the enter
objects, and the resource might actually be gone by then. I don't see any
obvious problems in this patch, but it seems like something that is easy to
cause bugs in the future.

Using this during initialization is OK. Maybe we should be passing the Resource*
as an argument to the init functions we need (like we did with the PP_Resource)
and not store it?

[dmichael (off chromium), 2012/05/16 22:18:34]

Note that EnterResource already stores it and exposes it. This just moves it up
the hierarchy (though it does make it accessible to SetResult, where most of the
behavior is). I took out the smarts in the destructor, FWIW (the reasoning is
that all thunks that use callbacks need to do SetResult, so we should be able to
make sure the callback is handled properly in SetResult and prior to
destruction, since SetResult should always be called for operations that take a
callback.)

The main motivation for moving it up the hierarchy is because (a) I need the
Resource* for the TrackedCallback, and (b) I don't want to do the PP_Resource
lookup twice. I haven't thought of a nice way to get the Resource* passed to the
EnterBase constructor *and* the Init function. But I can think about it some
more. We can also consider something like a slight optimization for Resource
lookup, where we store the last ID used and a Resource weak ptr. Then I wouldn't
need to care about doing 2 lookups on every resource thunk. Such an optimization
might actually help in typical plugin execution as well...

[brettw, 2012/05/20 17:46:46]

Can you put a big scary warning here not to use it after any Pepper function has
been invoked since that can cause the object to be released. I'd also like a
warning at the top of SetResult that you can't use the resource_ member because
it may have been destroyed.

[dmichael (off chromium), 2012/05/22 18:08:38]

Will do.

+
  private:
-  // Holds the callback. The function will only be non-NULL when the
-  // callback is requried. Optional callbacks don't require any special
-  // handling from us at this layer.
-  PP_CompletionCallback callback_;
+  // Holds the callback. For Enter objects that aren't given a callback, this
+  // will be NULL.
+  scoped_refptr<TrackedCallback> callback_;
 
   int32_t retval_;
 };
@@ -131,21 +178,21 @@ class EnterFunction : public subtle::EnterBase,
                       public subtle::LockOnEntry<lock_on_entry> {
  public:
   EnterFunction(PP_Instance instance, bool report_error)
-      : EnterBase() {
+      : EnterBase(instance) {
     Init(instance, report_error);
   }
   EnterFunction(PP_Instance instance,
                 const PP_CompletionCallback& callback,
                 bool report_error)
-      : EnterBase(callback) {
+      : EnterBase(0 /* resource */, callback) {
+    // TODO(dmichael): This means that the callback_ we get is not associated
+    //                 even with the instance, but we should handle that for
+    //                 MouseLock (maybe others?).
     Init(instance, report_error);
   }
 
   ~EnterFunction() {}
 
-  bool succeeded() const { return !!functions_; }
-  bool failed() const { return !functions_; }
-
   FunctionsT* functions() { return functions_; }
 
  private:
@@ -155,6 +202,10 @@ class EnterFunction : public subtle::EnterBase,
       functions_ = base->GetAs<FunctionsT>();
     else
       functions_ = NULL;
+    // Validate the callback.
+    SetStateForCallbackError(report_error);

[brettw, 2012/04/20 18:24:29]

I think you should just call this from the beginning of the
SetStateForFunction/ResourceError functions. I separated those out to keep the
templateized code minimal, but we needed slightly different implementatnions for
EnterResource vs. EnterFunction. So I think this would go in the common
non-templaized error handling code.

[dmichael (off chromium), 2012/05/16 22:18:34]

Done.

+    // Validate the resource (note, if both are wrong, we will return
+    // PP_ERROR_BADARGUMENT; last in wins).
     SetStateForFunctionError(instance, functions_, report_error);
   }
 
@@ -184,8 +235,7 @@ class EnterFunctionGivenResource : public EnterFunction<FunctionsT> {
 
  private:
   static PP_Instance GetInstanceForResource(PP_Resource resource) {
-    Resource* object =
-        PpapiGlobals::Get()->GetResourceTracker()->GetResource(resource);
+    Resource* object = subtle::EnterBase::GetResource(resource);
     return object ? object->pp_instance() : 0;
   }
 };
@@ -197,34 +247,32 @@ class EnterResource : public subtle::EnterBase,
                       public subtle::LockOnEntry<lock_on_entry> {
  public:
   EnterResource(PP_Resource resource, bool report_error)
-      : EnterBase() {
+      : EnterBase(resource) {
     Init(resource, report_error);
   }
-  EnterResource(PP_Resource resource,
-                const PP_CompletionCallback& callback,
+  EnterResource(PP_Resource resource, const PP_CompletionCallback& callback,
                 bool report_error)
-      : EnterBase(callback) {
+      : EnterBase(resource, callback) {
     Init(resource, report_error);
   }
   ~EnterResource() {}
 
-  bool succeeded() const { return !!object_; }
-  bool failed() const { return !object_; }
-
   ResourceT* object() { return object_; }
   Resource* resource() { return resource_; }
 
  private:
   void Init(PP_Resource resource, bool report_error) {
-    resource_ = GetResource(resource);
     if (resource_)
       object_ = resource_->GetAs<ResourceT>();
     else
       object_ = NULL;
+    // Validate the callback.
+    SetStateForCallbackError(report_error);
+    // Validate the resource (note, if both are wrong, we will return
+    // PP_ERROR_BADRESOURCE; last in wins).
     SetStateForResourceError(resource, resource_, object_, report_error);
   }
 
-  Resource* resource_;
   ResourceT* object_;
 
   DISALLOW_COPY_AND_ASSIGN(EnterResource);
@@ -239,6 +287,10 @@ class EnterResourceNoLock : public EnterResource<ResourceT, false> {
   EnterResourceNoLock(PP_Resource resource, bool report_error)
       : EnterResource<ResourceT, false>(resource, report_error) {
   }
+  EnterResourceNoLock(PP_Resource resource, PP_CompletionCallback callback,
+                      bool report_error)
+      : EnterResource<ResourceT, false>(resource, callback, report_error) {
+  }
 };
 
 // Simpler wrapper to enter the resource creation API. This is used for every
@@ -262,6 +314,16 @@ class PPAPI_THUNK_EXPORT EnterInstance
   ~EnterInstance();
 };
 
+class PPAPI_THUNK_EXPORT EnterInstanceNoLock
+    : public EnterFunctionNoLock<PPB_Instance_FunctionAPI> {
+ public:
+  EnterInstanceNoLock(PP_Instance instance);
+  EnterInstanceNoLock(PP_Instance instance,
+                      const PP_CompletionCallback& callback);
+  ~EnterInstanceNoLock();
+};
+
+
 }  // namespace thunk
 }  // namespace ppapi