[PPAPI] ResourceVar now reference counts its PP_Resource in the plugin.

ppapi/shared_impl/resource_var.h

 // Copyright 2013 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_SHARED_IMPL_RESOURCE_VAR_H_
 #define PPAPI_SHARED_IMPL_RESOURCE_VAR_H_
 
 #include "ipc/ipc_message.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/c/pp_var.h"
 #include "ppapi/shared_impl/ppapi_shared_export.h"
+#include "ppapi/shared_impl/scoped_pp_resource.h"
 #include "ppapi/shared_impl/var.h"
 
 namespace ppapi {
 
+class HostResourceVar;
+
 // Represents a resource Var.
 class PPAPI_SHARED_EXPORT ResourceVar : public Var {
  public:
-  // Makes a null resource var.
-  ResourceVar();
-
-  // Makes a resource var with an existing plugin-side resource.
-  explicit ResourceVar(PP_Resource pp_resource);
-
-  // Makes a resource var with a pending resource host.
-  // The |creation_message| contains instructions on how to create the
-  // plugin-side resource. Its type depends on the type of resource.
-  explicit ResourceVar(const IPC::Message& creation_message);
-
-  virtual ~ResourceVar();
-
   // Gets the resource ID associated with this var.
-  // This is 0 if a resource is still pending.
-  PP_Resource pp_resource() const { return pp_resource_; }
-
-  // Gets the message for creating a plugin-side resource.
-  // May be an empty message.
-  const IPC::Message& creation_message() const { return creation_message_; }
+  // This is 0 if a resource is still pending (only possible on the host side).
+  virtual PP_Resource pp_resource() const = 0;
 
   // Determines whether this is a pending resource.
-  // This is true if the pp_resource is 0 and there is a non-empty
-  // creation_message.
-  bool IsPending() const;
+  // This is true if, on the host side, the pp_resource is 0 and there is a
+  // non-empty creation_message.
+  virtual bool IsPending() const = 0;
+
+  // Safely down-cast to HostResourceVar, for access to creation_message.
+  // If this is not a HostResourceVar, returns NULL.
+  // Always succeeds if IsPending returns true.
+  virtual HostResourceVar* AsHostResourceVar();
 
   // Var override.
   virtual ResourceVar* AsResourceVar() OVERRIDE;
   virtual PP_VarType GetType() const OVERRIDE;
 
   // Helper function that converts a PP_Var to a ResourceVar. This will
   // return NULL if the PP_Var is not of Resource type.
   static ResourceVar* FromPPVar(PP_Var var);
 
+ protected:
+  ResourceVar();

[dmichael (off chromium), 2013/09/13 17:29:03]

you also need to define a virtual destructor.

[Matt Giuca, 2013/09/17 00:24:52]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ResourceVar);
+};
+
+// Represents a resource Var, usable on the host side.

[raymes, 2013/09/13 17:41:33]

Nit: please fill the line to 80chars with comments. (and below)

[Matt Giuca, 2013/09/17 00:24:52]

Done.

+// Can either have a plugin-side resource or a pending resource host.
+class PPAPI_SHARED_EXPORT HostResourceVar : public ResourceVar {

[dmichael (off chromium), 2013/09/13 17:29:03]

Should this live in content/renderer/pepper instead?

[Matt Giuca, 2013/09/17 01:26:28]

Done.

+ public:
+  // Makes a null resource var.
+  HostResourceVar();
+
+  // Makes a resource var with an existing plugin-side resource.
+  explicit HostResourceVar(PP_Resource pp_resource);
+
+  // Makes a resource var with a pending resource host.
+  // The |creation_message| contains instructions on how to create the

[dmichael (off chromium), 2013/09/13 17:29:03]

s/"instruction on how to"/"data needed to"?

[Matt Giuca, 2013/09/17 00:24:52]

Done.

+  // plugin-side resource. Its type depends on the type of resource.
+  explicit HostResourceVar(const IPC::Message& creation_message);
+
+  virtual ~HostResourceVar();
+
+  // ResourceVar override.
+  virtual PP_Resource pp_resource() const OVERRIDE;
+  virtual bool IsPending() const OVERRIDE;
+  virtual HostResourceVar* AsHostResourceVar() OVERRIDE;
+
+  // Gets the message for creating a plugin-side resource.
+  // May be an empty message.

[dmichael (off chromium), 2013/09/13 17:29:03]

Would it be better to own a scoped_ptr instead, and return NULL if there isn't
one? You could maybe also make it virtual then and get rid of AsHostResourceVar.

[Matt Giuca, 2013/09/17 00:24:52]

Done.

+  const IPC::Message& creation_message() const { return creation_message_; }
+
  private:
   // Real resource ID in the plugin. 0 if one has not yet been created
   // (indicating that there is a pending host resource).
   PP_Resource pp_resource_;
 
   // If the plugin-side resource has not yet been created, carries a message to
   // create a resource of the specific type on the plugin side.
   // Otherwise, carries an empty message.
   IPC::Message creation_message_;
 
-  DISALLOW_COPY_AND_ASSIGN(ResourceVar);
+  DISALLOW_COPY_AND_ASSIGN(HostResourceVar);
+};
+
+// Represents a resource Var, usable on the plugin side.

[raymes, 2013/09/13 17:41:33]

nit: Var -> var for consistency

[Matt Giuca, 2013/09/17 00:24:52]

Done.

+class PPAPI_SHARED_EXPORT PluginResourceVar : public ResourceVar {

[dmichael (off chromium), 2013/09/13 17:29:03]

This should probably live in ppapi/proxy

(Note, I think ArrayBufferVar might have a similar split)

[Matt Giuca, 2013/09/17 01:26:28]

Done.

+ public:
+  // Makes a null resource var.
+  PluginResourceVar();
+
+  // Makes a resource var with an existing resource.
+  // Takes one reference to the given resource.
+  explicit PluginResourceVar(PP_Resource pp_resource);
+
+  virtual ~PluginResourceVar();
+
+  // ResourceVar override.
+  virtual PP_Resource pp_resource() const OVERRIDE;

[raymes, 2013/09/13 17:41:33]

nit: pp_resource() style naming is usually for simple accessors. Maybe name this
GetPPResource()

[Matt Giuca, 2013/09/17 00:24:52]

Done.

+  virtual bool IsPending() const OVERRIDE;
+
+ private:
+  // Resource ID.
+  ScopedPPResource pp_resource_;

[dmichael (off chromium), 2013/09/13 17:29:03]

If possible, I think scoped_refptr<Resource> would be better. That would use our
internal ref-counting, and give the plugin author (and us) fewer ways to mess
up. E.g., if the plugin decrements the ref-count one too many times, the
resource will disappear. So with ScopedPPResource, we can't assume that resource
will stay alive.

[Matt Giuca, 2013/09/17 00:24:52]

I had a long chat with Yuzhu about this. First, we decided that it didn't really
matter either way, since it only matters if there is a refcounting bug in the
user's code.

I can see the point of your suggestion -- it seems cleaner to be using the
internal refcount than the one designed for our users. But I think the problem
is that since we need to be able to get the PP_Resource back out of the var, it
doesn't make sense for the var to not hold a reference on the PP_Resource (as
opposed to the underlying Resource).

So I'm wondering about what happens in the (I would imagine, common) case where
the user's code:
1. Creates a PP_Resource.
2. Puts it in a PP_Var.
3. Releases the PP_Resource.
4. Gets the PP_Resource back from the PP_Var.

With the current proposal, the refcounts will look like this:
1. PP_Resource refcount 1, Resource refcount 1.
2. Var refcount 1, PP_Resource refcount 2, Resource refcount 1.
3. Var refcount 1, PP_Resource refcount 1, Resource refcount 1.

I'm thinking that with your proposal, the refcounts will look like this after
each step:
1. PP_Resource refcount 1, Resource refcount 1.
2. Var refcount 1, PP_Resource refcount 1, Resource refcount 2.
3. Var refcount 1, PP_Resource refcount 0, Resource refcount 1.

So we will return to the user in step 4 a resource with a refcount of 0. This is
weird. Apparently, this will be okay -- a resource with a refcount of 0 is still
usable. But it doesn't seem right and could have unintended consequences. So I
would prefer to stick with the current implementation. What do you think?

(This is a decision which we should be able to change our minds on later, since
it doesn't affect the user.)

+
+  DISALLOW_COPY_AND_ASSIGN(PluginResourceVar);
 };
 
 }  // namespace ppapi
 
 #endif  // PPAPI_SHARED_IMPL_RESOURCE_VAR_H_