Enhance the PPAPI enter tracking.

The goal here is to be able to add thread checking to the enter functions, which will require more types of failure than just "bad resource".

I folded completion callback force executing into this, which also simplifies the thunks a bit. If we like this method, I'll convert the rest and delete MayForceCallback.

I converted URLLoader and Graphics2D to use this.

Review URL: https://chromiumcodereview.appspot.com/9235003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@118951 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 184 insertions, 46 deletions

diff --git a/ppapi/thunk/enter.cc b/ppapi/thunk/enter.cc
index 7e9f70a..42f4a0e 100644
--- a/ppapi/thunk/enter.cc
+++ b/ppapi/thunk/enter.cc
@@ -4,13 +4,89 @@
 
 #include "ppapi/thunk/enter.h"
 
-#include "base/lazy_instance.h"
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/message_loop.h"
+#include "ppapi/c/pp_errors.h"
 #include "ppapi/thunk/ppb_instance_api.h"
 #include "ppapi/thunk/resource_creation_api.h"
 
 namespace ppapi {
 namespace thunk {
 
+namespace subtle {
+
+bool CallbackIsRequired(const PP_CompletionCallback& callback) {
+  return callback.func != NULL &&
+         (callback.flags & PP_COMPLETIONCALLBACK_FLAG_OPTIONAL) == 0;
+}
+
+EnterBase::EnterBase()
+    : callback_(PP_BlockUntilComplete()),
+      retval_(PP_OK) {
+  // TODO(brettw) validate threads.
+}
+
+EnterBase::EnterBase(const PP_CompletionCallback& callback)
+    : callback_(CallbackIsRequired(callback) ? callback
+                                             : PP_BlockUntilComplete()),
+      retval_(PP_OK) {
+  // TODO(brettw) validate threads.
+}
+
+EnterBase::~EnterBase() {
+  if (callback_.func) {
+    // All async completions should have cleared the callback in SetResult().
+    DCHECK(retval_ != PP_OK_COMPLETIONPENDING && retval_ != PP_OK);
+    MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
+        callback_.func, callback_.user_data, retval_));
+  }
+}
+
+int32_t EnterBase::SetResult(int32_t result) {
+  if (!callback_.func || result == PP_OK_COMPLETIONPENDING) {
+    // Easy case, we don't need to issue the callback (either none is
+    // required, or the implementation will asynchronously issue it
+    // for us), just store the result.
+    callback_ = PP_BlockUntilComplete();
+    retval_ = result;
+    return retval_;
+  }
+
+  // This is a required callback, asynchronously issue it.
+  // TODO(brettw) make this work on different threads, etc.
+  MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
+      callback_.func, callback_.user_data, result));
+
+  // Now that the callback will be issued in the future, we should return
+  // "pending" to the caller, and not issue the callback again.
+  callback_ = PP_BlockUntilComplete();
+  retval_ = PP_OK_COMPLETIONPENDING;
+  return retval_;
+}
+
+FunctionGroupBase* EnterBase::GetFunctions(PP_Instance instance,
+                                           ApiID id) const {
+  return PpapiGlobals::Get()->GetFunctionAPI(instance, id);
+}
+
+Resource* EnterBase::GetResource(PP_Resource resource) const {
+  return PpapiGlobals::Get()->GetResourceTracker()->GetResource(resource);
+}
+
+void EnterBase::SetStateForResourceError(PP_Resource /* pp_resource */,
+                                         Resource* /* resource_base */,
+                                         void* object,
+                                         bool /* report_error */) {
+  if (object)
+    return;  // Everything worked.
+
+  retval_ = PP_ERROR_BADRESOURCE;
+  // TODO(brettw) log the error.
+}
+
+}  // namespace subtle
+
 EnterResourceCreation::EnterResourceCreation(PP_Instance instance)
     : EnterFunctionNoLock<ResourceCreationAPI>(instance, true) {
 }
diff --git a/ppapi/thunk/enter.h b/ppapi/thunk/enter.h
index e2c6938..86d6022 100644
--- a/ppapi/thunk/enter.h
+++ b/ppapi/thunk/enter.h
@@ -66,16 +66,67 @@ struct LockOnEntry<true> {
   }
 };
 
+// Keep non-templatized since we need non-inline functions here.
+class PPAPI_THUNK_EXPORT EnterBase {
+ public:
+  EnterBase();
+  explicit EnterBase(const PP_CompletionCallback& callback);
+  virtual ~EnterBase();
+
+  // Sets the result.
+  //
+  // 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_; }
+
+ 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;
+
+  // 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."
+  //
+  // 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);
+
+ 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_;
+
+  int32_t retval_;
+};
+
 }  // namespace subtle
 
 
 template<typename FunctionsT, bool lock_on_entry = true>
-class EnterFunction : subtle::LockOnEntry<lock_on_entry> {
+class EnterFunction : public subtle::EnterBase,
+                      public subtle::LockOnEntry<lock_on_entry> {
  public:
   EnterFunction(PP_Instance instance, bool report_error)
       : functions_(NULL) {
-    FunctionGroupBase* base = PpapiGlobals::Get()->GetFunctionAPI(
-        instance, FunctionsT::kApiID);
+    FunctionGroupBase* base = GetFunctions(instance, FunctionsT::kApiID);
     if (base)
       functions_ = base->GetAs<FunctionsT>();
     // TODO(brettw) check error and if report_error is set, do something.
@@ -124,15 +175,18 @@ class EnterFunctionGivenResource : public EnterFunction<FunctionsT> {
 // EnterResource ---------------------------------------------------------------
 
 template<typename ResourceT, bool lock_on_entry = true>
-class EnterResource : subtle::LockOnEntry<lock_on_entry> {
+class EnterResource : public subtle::EnterBase,
+                      public subtle::LockOnEntry<lock_on_entry> {
  public:
   EnterResource(PP_Resource resource, bool report_error)
-      : object_(NULL) {
-    resource_ =
-        PpapiGlobals::Get()->GetResourceTracker()->GetResource(resource);
-    if (resource_)
-      object_ = resource_->GetAs<ResourceT>();
-    // TODO(brettw) check error and if report_error is set, do something.
+      : EnterBase() {
+    Init(resource, report_error);
+  }
+  EnterResource(PP_Resource resource,
+                const PP_CompletionCallback& callback,
+                bool report_error)
+      : EnterBase(callback) {
+    Init(resource, report_error);
   }
   ~EnterResource() {}
 
@@ -143,6 +197,15 @@ class EnterResource : subtle::LockOnEntry<lock_on_entry> {
   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;
+    SetStateForResourceError(resource, resource_, object_, report_error);
+  }
+
   Resource* resource_;
   ResourceT* object_;
 
diff --git a/ppapi/thunk/ppb_graphics_2d_thunk.cc b/ppapi/thunk/ppb_graphics_2d_thunk.cc
index e42d002..edb7ea9 100644
--- a/ppapi/thunk/ppb_graphics_2d_thunk.cc
+++ b/ppapi/thunk/ppb_graphics_2d_thunk.cc
@@ -16,24 +16,26 @@ namespace thunk {
 
 namespace {
 
+typedef EnterResource<PPB_Graphics2D_API> EnterGraphics2D;
+
 PP_Resource Create(PP_Instance instance,
                    const PP_Size* size,
                    PP_Bool is_always_opaque) {
-  EnterFunction<ResourceCreationAPI> enter(instance, true);
+  EnterResourceCreation enter(instance);
   if (enter.failed())
     return 0;
   return enter.functions()->CreateGraphics2D(instance, *size, is_always_opaque);
 }
 
 PP_Bool IsGraphics2D(PP_Resource resource) {
-  EnterResource<PPB_Graphics2D_API> enter(resource, false);
+  EnterGraphics2D enter(resource, false);
   return enter.succeeded() ? PP_TRUE : PP_FALSE;
 }
 
 PP_Bool Describe(PP_Resource graphics_2d,
                  PP_Size* size,
                  PP_Bool* is_always_opaque) {
-  EnterResource<PPB_Graphics2D_API> enter(graphics_2d, true);
+  EnterGraphics2D enter(graphics_2d, true);
   if (enter.failed()) {
     size->width = 0;
     size->height = 0;
@@ -47,7 +49,7 @@ void PaintImageData(PP_Resource graphics_2d,
                     PP_Resource image_data,
                     const PP_Point* top_left,
                     const PP_Rect* src_rect) {
-  EnterResource<PPB_Graphics2D_API> enter(graphics_2d, true);
+  EnterGraphics2D enter(graphics_2d, true);
   if (enter.failed())
     return;
   enter.object()->PaintImageData(image_data, top_left, src_rect);
@@ -56,14 +58,14 @@ void PaintImageData(PP_Resource graphics_2d,
 void Scroll(PP_Resource graphics_2d,
             const PP_Rect* clip_rect,
             const PP_Point* amount) {
-  EnterResource<PPB_Graphics2D_API> enter(graphics_2d, true);
+  EnterGraphics2D enter(graphics_2d, true);
   if (enter.failed())
     return;
   enter.object()->Scroll(clip_rect, amount);
 }
 
 void ReplaceContents(PP_Resource graphics_2d, PP_Resource image_data) {
-  EnterResource<PPB_Graphics2D_API> enter(graphics_2d, true);
+  EnterGraphics2D enter(graphics_2d, true);
   if (enter.failed())
     return;
   enter.object()->ReplaceContents(image_data);
@@ -71,11 +73,10 @@ void ReplaceContents(PP_Resource graphics_2d, PP_Resource image_data) {
 
 int32_t Flush(PP_Resource graphics_2d,
               PP_CompletionCallback callback) {
-  EnterResource<PPB_Graphics2D_API> enter(graphics_2d, true);
+  EnterGraphics2D enter(graphics_2d, callback, true);
   if (enter.failed())
-    return MayForceCallback(callback, PP_ERROR_BADRESOURCE);
-  int32_t result = enter.object()->Flush(callback);
-  return MayForceCallback(callback, result);
+    return enter.retval();
+  return enter.SetResult(enter.object()->Flush(callback));
 }
 
 const PPB_Graphics2D g_ppb_graphics_2d_thunk = {
diff --git a/ppapi/thunk/ppb_url_loader_thunk.cc b/ppapi/thunk/ppb_url_loader_thunk.cc
index 719d443..a4a2b3d 100644
--- a/ppapi/thunk/ppb_url_loader_thunk.cc
+++ b/ppapi/thunk/ppb_url_loader_thunk.cc
@@ -15,41 +15,41 @@ namespace thunk {
 
 namespace {
 
+typedef EnterResource<PPB_URLLoader_API> EnterURLLoader;
+
 PP_Resource Create(PP_Instance instance) {
-  EnterFunction<ResourceCreationAPI> enter(instance, true);
+  EnterResourceCreation enter(instance);
   if (enter.failed())
     return 0;
   return enter.functions()->CreateURLLoader(instance);
 }
 
 PP_Bool IsURLLoader(PP_Resource resource) {
-  EnterResource<PPB_URLLoader_API> enter(resource, false);
+  EnterURLLoader enter(resource, false);
   return PP_FromBool(enter.succeeded());
 }
 
 int32_t Open(PP_Resource loader,
              PP_Resource request_id,
              PP_CompletionCallback callback) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, callback, true);
   if (enter.failed())
-    return MayForceCallback(callback, PP_ERROR_BADRESOURCE);
-  int32_t result = enter.object()->Open(request_id, callback);
-  return MayForceCallback(callback, result);
+    return enter.retval();
+  return enter.SetResult(enter.object()->Open(request_id, callback));
 }
 
 int32_t FollowRedirect(PP_Resource loader,
                        PP_CompletionCallback callback) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, callback, true);
   if (enter.failed())
-    return MayForceCallback(callback, PP_ERROR_BADRESOURCE);
-  int32_t result = enter.object()->FollowRedirect(callback);
-  return MayForceCallback(callback, result);
+    return enter.retval();
+  return enter.SetResult(enter.object()->FollowRedirect(callback));
 }
 
 PP_Bool GetUploadProgress(PP_Resource loader,
                           int64_t* bytes_sent,
                           int64_t* total_bytes_to_be_sent) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, true);
   if (enter.failed()) {
     *bytes_sent = 0;
     *total_bytes_to_be_sent = 0;
@@ -62,7 +62,7 @@ PP_Bool GetUploadProgress(PP_Resource loader,
 PP_Bool GetDownloadProgress(PP_Resource loader,
                             int64_t* bytes_received,
                             int64_t* total_bytes_to_be_received) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, true);
   if (enter.failed()) {
     *bytes_received = 0;
     *total_bytes_to_be_received = 0;
@@ -73,7 +73,7 @@ PP_Bool GetDownloadProgress(PP_Resource loader,
 }
 
 PP_Resource GetResponseInfo(PP_Resource loader) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, true);
   if (enter.failed())
     return 0;
   return enter.object()->GetResponseInfo();
@@ -83,38 +83,36 @@ int32_t ReadResponseBody(PP_Resource loader,
                          void* buffer,
                          int32_t bytes_to_read,
                          PP_CompletionCallback callback) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, callback, true);
   if (enter.failed())
-    return MayForceCallback(callback, PP_ERROR_BADRESOURCE);
-  int32_t result = enter.object()->ReadResponseBody(buffer, bytes_to_read,
-                                                    callback);
-  return MayForceCallback(callback, result);
+    return enter.retval();
+  return enter.SetResult(enter.object()->ReadResponseBody(buffer, bytes_to_read,
+                                                         callback));
 }
 
 int32_t FinishStreamingToFile(PP_Resource loader,
                               PP_CompletionCallback callback) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, callback, true);
   if (enter.failed())
-    return MayForceCallback(callback, PP_ERROR_BADRESOURCE);
-  int32_t result = enter.object()->FinishStreamingToFile(callback);
-  return MayForceCallback(callback, result);
+    return enter.retval();
+  return enter.SetResult(enter.object()->FinishStreamingToFile(callback));
 }
 
 void Close(PP_Resource loader) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, true);
   if (enter.succeeded())
     enter.object()->Close();
 }
 
 void GrantUniversalAccess(PP_Resource loader) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, true);
   if (enter.succeeded())
     enter.object()->GrantUniversalAccess();
 }
 
 void SetStatusCallback(PP_Resource loader,
                        PP_URLLoaderTrusted_StatusCallback cb) {
-  EnterResource<PPB_URLLoader_API> enter(loader, true);
+  EnterURLLoader enter(loader, true);
   if (enter.succeeded())
     enter.object()->SetStatusCallback(cb);
 }