PPAPI: Make blocking completion callbacks work.

ppapi/thunk/enter.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "ppapi/shared_impl/ppapi_globals.h"
 #include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/thunk/ppapi_thunk_export.h"
 #include "ppapi/thunk/ppb_instance_api.h"
 #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
 // the internal implementation. They make sure the lock is acquired and will
 // automatically set up some stuff for you.
 //
 // You should always check whether the enter succeeded before using the object.
 // If this fails, then the instance or resource ID supplied was invalid.
 //
 // The |report_error| arguments to the constructor should indicate if errors
@@ skipping 15 matching lines @@
 // This helps us define our RAII Enter classes easily. To make an RAII class
 // which locks the proxy lock on construction and unlocks on destruction,
 // inherit from |LockOnEntry<true>|. For cases where you don't want to lock,
 // inherit from |LockOnEntry<false>|. This allows us to share more code between
 // Enter* and Enter*NoLock classes.
 template <bool lock_on_entry>
 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 <>
 struct LockOnEntry<true> {
   LockOnEntry() {
     ppapi::ProxyLock::Acquire();
   }
   ~LockOnEntry() {
     ppapi::ProxyLock::Release();
   }
 };
 
 // Keep non-templatized since we need non-inline functions here.
 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(...);
   // without having to write a separate "return enter.retval();" line.
   int32_t SetResult(int32_t result);
 
   // 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
   // it in every template.
   FunctionGroupBase* GetFunctions(PP_Instance instance, ApiID id) const;
 
   // 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.
   void SetStateForResourceError(PP_Resource pp_resource,
                                 Resource* resource_base,
                                 void* object,
                                 bool report_error);
 
   // Same as SetStateForResourceError except for function API.
   void SetStateForFunctionError(PP_Instance pp_instance,
                                 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_;
 };
 
 }  // namespace subtle
 
 // EnterFunction --------------------------------------------------------------
 
 template<typename FunctionsT, bool lock_on_entry = true>
 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:
   void Init(PP_Instance instance, bool report_error) {
     FunctionGroupBase* base = GetFunctions(instance, FunctionsT::kApiID);
     if (base)
       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);
   }
 
   FunctionsT* functions_;
 
   DISALLOW_COPY_AND_ASSIGN(EnterFunction);
 };
 
 // Like EnterFunction but assumes the lock is already held.
 template<typename FunctionsT>
 class EnterFunctionNoLock : public EnterFunction<FunctionsT, false> {
  public:
   EnterFunctionNoLock(PP_Instance instance, bool report_error)
       : EnterFunction<FunctionsT, false>(instance, report_error) {
   }
 };
 
 // Used when a caller has a resource, and wants to do EnterFunction for the
 // instance corresponding to that resource.
 template<typename FunctionsT>
 class EnterFunctionGivenResource : public EnterFunction<FunctionsT> {
  public:
   EnterFunctionGivenResource(PP_Resource resource, bool report_error)
       : EnterFunction<FunctionsT>(GetInstanceForResource(resource),
                                   report_error) {
   }
 
  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;
   }
 };
 
 // EnterResource ---------------------------------------------------------------
 
 template<typename ResourceT, bool lock_on_entry = true>
 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);
 };
 
 // ----------------------------------------------------------------------------
 
 // Like EnterResource but assumes the lock is already held.
 template<typename ResourceT>
 class EnterResourceNoLock : public EnterResource<ResourceT, false> {
  public:
   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
 // class so we have this helper function to save template instantiations and
 // typing.
 class PPAPI_THUNK_EXPORT EnterResourceCreation
     : public EnterFunction<ResourceCreationAPI> {
  public:
   EnterResourceCreation(PP_Instance instance);
   ~EnterResourceCreation();
 };
 
 // Simpler wrapper to enter the instance API from proxy code. This is used for
 // many interfaces so we have this helper function to save template
 // instantiations and typing.
 class PPAPI_THUNK_EXPORT EnterInstance
     : public EnterFunction<PPB_Instance_FunctionAPI> {
  public:
   EnterInstance(PP_Instance instance);
   EnterInstance(PP_Instance instance, const PP_CompletionCallback& callback);
   ~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
 
 #endif  // PPAPI_THUNK_ENTER_H_