Hi, I have implemented the split PluginResourceVar and HostResourceVar as
discussed today. The Plugin one has a scoped PP_Resource.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:45: ResourceVar();
you also need to define a virtual destructor.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:53: class PPAPI_SHARED_EXPORT HostResourceVar :
public ResourceVar {
Should this live in content/renderer/pepper instead?

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:62: // The |creation_message| contains
instructions on how to create the
s/"instruction on how to"/"data needed to"?

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:74: // May be an empty message.
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.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:91: class PPAPI_SHARED_EXPORT PluginResourceVar
: public ResourceVar {
This should probably live in ppapi/proxy

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

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:108: ScopedPPResource pp_resource_;
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.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
File ppapi/shared_impl/unittest_utils.cc (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
ppapi/shared_impl/unittest_utils.cc:190: (expected_message.flags() & 0xffffff00)
|
I think I mentioned this in another CL...  would it be better to just compare
the bits that are expected to be the same, rather than modifying the messages
we're inspecting?

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:51: // Represents a resource Var, usable on the
host side.
Nit: please fill the line to 80chars with comments. (and below)

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:90: // Represents a resource Var, usable on the
plugin side.
nit: Var -> var for consistency

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:103: virtual PP_Resource pp_resource() const
OVERRIDE;
nit: pp_resource() style naming is usually for simple accessors. Maybe name this
GetPPResource()

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
File ppapi/shared_impl/unittest_utils.cc (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
ppapi/shared_impl/unittest_utils.cc:190: (expected_message.flags() & 0xffffff00)
|
On 2013/09/13 17:29:03, dmichael wrote:
> I think I mentioned this in another CL...  would it be better to just compare
> the bits that are expected to be the same, rather than modifying the messages
> we're inspecting?

+1

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
File ppapi/shared_impl/unittest_utils.cc (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
ppapi/shared_impl/unittest_utils.cc:190: (expected_message.flags() & 0xffffff00)
|
See my response there:
https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/unittes...

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
File ppapi/shared_impl/unittest_utils.cc (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/unittest...
ppapi/shared_impl/unittest_utils.cc:190: (expected_message.flags() & 0xffffff00)
|
On 2013/09/13 21:23:11, Matt Giuca wrote:
> See my response there:
>
https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/unittes...
Okay, thanks. Sorry I missed that. Makes sense.

Responded to all notes except for big moves.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:45: ResourceVar();
On 2013/09/13 17:29:03, dmichael wrote:
> you also need to define a virtual destructor.

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:51: // Represents a resource Var, usable on the
host side.
On 2013/09/13 17:41:33, raymes wrote:
> Nit: please fill the line to 80chars with comments. (and below)

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:62: // The |creation_message| contains
instructions on how to create the
On 2013/09/13 17:29:03, dmichael wrote:
> s/"instruction on how to"/"data needed to"?

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:74: // May be an empty message.
On 2013/09/13 17:29:03, dmichael wrote:
> 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.
> 

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:90: // Represents a resource Var, usable on the
plugin side.
On 2013/09/13 17:41:33, raymes wrote:
> nit: Var -> var for consistency

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:103: virtual PP_Resource pp_resource() const
OVERRIDE;
On 2013/09/13 17:41:33, raymes wrote:
> nit: pp_resource() style naming is usually for simple accessors. Maybe name
this
> GetPPResource()

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:108: ScopedPPResource pp_resource_;
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.)

jochen: Added for content/content_renderer.gypi OWNERS.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:53: class PPAPI_SHARED_EXPORT HostResourceVar :
public ResourceVar {
On 2013/09/13 17:29:03, dmichael wrote:
> Should this live in content/renderer/pepper instead?

Done.

https://codereview.chromium.org/23809016/diff/3001/ppapi/shared_impl/resource...
ppapi/shared_impl/resource_var.h:91: class PPAPI_SHARED_EXPORT PluginResourceVar
: public ResourceVar {
On 2013/09/13 17:29:03, dmichael wrote:
> This should probably live in ppapi/proxy
> 
> (Note, I think ArrayBufferVar might have a similar split)

Done.

> 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.
Everywhere else where we return a PP_Resource, we increment the reference count,
passing a reference to the plugin. It seems like we should do that here as well
for consistency.

On 2013/09/17 20:39:00, dmichael wrote:
> > 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.
> Everywhere else where we return a PP_Resource, we increment the reference
count,
> passing a reference to the plugin. It seems like we should do that here as
well
> for consistency.
Oh, and to be explicit...  I think that means we wouldn't ever return a
PP_Resource with refcount 0 here, so I don't think we need to worry about that.
So scoped_refptr<Resource> still seems safer/better to me.

> Everywhere else where we return a PP_Resource, we increment the reference
count,
> passing a reference to the plugin. It seems like we should do that here as
well
> for consistency.

Can you give examples of where else we increment the ref count? I'm not talking
about the public API here, but the internal method ResourceVar::GetPPResource().
It seems wrong to have a getter method that has the side-effect of incrementing
the refcount. Also it doesn't make sense to do that on the host side, and
GetPPResource needs to have consistent semantics on both sides. So I think it
shouldn't increment the ref count. Which means my original argument stands.

Now if we do make it so that the public API returns a PP_Resource with the
refcount incremented (which I agree makes the most sense), then it wouldn't be
the case that the plugin code would ever see a resource with a refcount of 0,
but it still seems wrong to have an internal method GetPPResource that may
return a PP_Resource with a refcount of 0.

I have just added some more code in Patch Set 6. I realised (working on a
subsequent CL) that it will be necessary for VarTracker to have virtual
ResourceVar creation methods, to abstract over whether to create a
HostResourceVar or PluginResourceVar. This follows the same pattern as
ArrayBuffer, which has abstract creation methods in VarTracker.

On 2013/09/18 00:53:39, Matt Giuca wrote:
> > Everywhere else where we return a PP_Resource, we increment the reference
> count,
> > passing a reference to the plugin. It seems like we should do that here as
> well
> > for consistency.
> 
> Can you give examples of where else we increment the ref count? I'm not
talking
> about the public API here, but the internal method
ResourceVar::GetPPResource().
But I'm talking about the public API. That's what step 4 is in your example,
right? You said "we will return to the user in step 4 a resource with a
refocount of 0". I'm saying we should bump the reference count at the time it's
returned to the plugin. I understand it is not strictly necessary in this case,
because the ResourceVar is making sure that the Resource stays alive. But in
other cases, where we return a PP_Resource in an API (either as the return or as
an out-param), we *must* pass a reference count to the plugin, or it would not
be "alive" at the time the plugin gets it. Here's one example, but it's really
any place where we call "Resource::GetReference". E.g., all the "Create" methods
do this.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...



> It seems wrong to have a getter method that has the side-effect of
incrementing
> the refcount. Also it doesn't make sense to do that on the host side, and
> GetPPResource needs to have consistent semantics on both sides. So I think it
> shouldn't increment the ref count. Which means my original argument stands.
What I'm suggesting is we don't have GetPPResource. Instead, it's "GetResource",
and it returns a scoped_refptr<Resource>. This is not unusual at all for us to
be briefly hold a scoped_refptr<Resource> that has 0 plugin ref-count (see all
of the Create methods). At the time we need to return the Resource to the
plugin, we call "GetReference", which returns a PP_Resource and increments the
ref-count on behalf of the plugin.

> 
> Now if we do make it so that the public API returns a PP_Resource with the
> refcount incremented (which I agree makes the most sense), then it wouldn't be
> the case that the plugin code would ever see a resource with a refcount of 0,
> but it still seems wrong to have an internal method GetPPResource that may
> return a PP_Resource with a refcount of 0.
I agree, the *internal* method shouldn't bump the plugin's reference count (we
already have GetReference for that). Sorry for the misunderstanding. Can the
internal method return a scoped_refptr<Resource> instead?

On 2013/09/18 17:26:57, dmichael wrote:
> On 2013/09/18 00:53:39, Matt Giuca wrote:
> > > Everywhere else where we return a PP_Resource, we increment the reference
> > count,
> > > passing a reference to the plugin. It seems like we should do that here as
> > well
> > > for consistency.
> > 
> > Can you give examples of where else we increment the ref count? I'm not
> talking
> > about the public API here, but the internal method
> ResourceVar::GetPPResource().
> But I'm talking about the public API. That's what step 4 is in your example,
> right? You said "we will return to the user in step 4 a resource with a
> refocount of 0".

Sorry, I got a bit mixed up between the public API and the internal methods. I
think the public API should increment the refcount, so that is settled. There is
no problem of having a 0-refcount resource passed to the user's code.

> What I'm suggesting is we don't have GetPPResource. Instead, it's
"GetResource",
> and it returns a scoped_refptr<Resource>. This is not unusual at all for us to
> be briefly hold a scoped_refptr<Resource> that has 0 plugin ref-count (see all
> of the Create methods). At the time we need to return the Resource to the
> plugin, we call "GetReference", which returns a PP_Resource and increments the
> ref-count on behalf of the plugin.

Ah, I see. But the problem is we can't have that API, because we need a
consistent set of methods for both HostResourceVar and PluginResourceVar. On the
host side, it doesn't make sense to have a GetResource that returns a
scoped_refptr<Resource>, because there is no physical Resource object on the
host, only a PP_Resource ID. (Correct me if I'm wrong about that.)

It just overall seems simpler to me for the Var (on both host and plugin) to
remember a resource ID instead of the plugin side keeping a pointer to a
Resource. Is the only reason to keep a scoped_refptr<Resource> just to deal with
the case where the user releases the resource too many times? I don't think it's
necessary to handle that case since it's clearly a user error (just like we
don't handle the case where the user dereferences a dangling pointer).

On Wed, Sep 18, 2013 at 6:09 PM, <mgiuca@chromium.org> wrote:

> On 2013/09/18 17:26:57, dmichael wrote:
>
>> On 2013/09/18 00:53:39, Matt Giuca wrote:
>> > > Everywhere else where we return a PP_Resource, we increment the
>> reference
>> > count,
>> > > passing a reference to the plugin. It seems like we should do that
>> here as
>> > well
>> > > for consistency.
>> >
>> > Can you give examples of where else we increment the ref count? I'm not
>> talking
>> > about the public API here, but the internal method
>> ResourceVar::GetPPResource().
>> But I'm talking about the public API. That's what step 4 is in your
>> example,
>> right? You said "we will return to the user in step 4 a resource with a
>> refocount of 0".
>>
>
> Sorry, I got a bit mixed up between the public API and the internal
> methods. I
> think the public API should increment the refcount, so that is settled.
> There is
> no problem of having a 0-refcount resource passed to the user's code.
>
>
>  What I'm suggesting is we don't have GetPPResource. Instead, it's
>>
> "GetResource",
>
>> and it returns a scoped_refptr<Resource>. This is not unusual at all for
>> us to
>> be briefly hold a scoped_refptr<Resource> that has 0 plugin ref-count
>> (see all
>> of the Create methods). At the time we need to return the Resource to the
>> plugin, we call "GetReference", which returns a PP_Resource and
>> increments the
>> ref-count on behalf of the plugin.
>>
>
> Ah, I see. But the problem is we can't have that API, because we need a
> consistent set of methods for both HostResourceVar and PluginResourceVar.
> On the
> host side, it doesn't make sense to have a GetResource that returns a
> scoped_refptr<Resource>, because there is no physical Resource object on
> the
> host, only a PP_Resource ID. (Correct me if I'm wrong about that.)
>
Oh, good point. If you need GetPPResource on the host side, it has to be
PP_Resource, not Resource.


>
> It just overall seems simpler to me for the Var (on both host and plugin)
> to
> remember a resource ID instead of the plugin side keeping a pointer to a
> Resource. Is the only reason to keep a scoped_refptr<Resource> just to
> deal with
> the case where the user releases the resource too many times? I don't
> think it's
> necessary to handle that case since it's clearly a user error (just like we
> don't handle the case where the user dereferences a dangling pointer).
>
It also is defensive coding. If the plugin can make that object disappear,
you have to deal with that possibility or risk a crash or other security
relevant coding error. If you hold an internal reference, you can be more
sure the object will be alive.


>
>
https://codereview.chromium.**org/23809016/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

On 2013/09/19 00:26:31, dmichael wrote:
> Oh, good point. If you need GetPPResource on the host side, it has to be
> PP_Resource, not Resource.

So does this mean you agree with keeping it the way it is?

> It also is defensive coding. If the plugin can make that object disappear,
> you have to deal with that possibility or risk a crash or other security
> relevant coding error. If you hold an internal reference, you can be more
> sure the object will be alive.

I'm concerned about the security part. Is there an actual security issue you can
see with this? I think that if the resource refcount is 0, it will be removed
from the resource tracker. Then calling ResourceTracker::GetResource will return
NULL. This could cause some weird issues in our code on the plugin side, but I
don't think it's a) a security problem to have NULL pointers on the plugin, or
b) possible to crash the renderer, since this is an issue specific to the
plugin. So it doesn't seem like the user can do any serious damage by
over-releasing the resource, any more than they can already do by running native
code in the sandbox.

If you don't have any explicit security concerns, I'd like to move forward with
this. Are there any other outstanding problems with this CL?

On Wed, Sep 18, 2013 at 7:06 PM, <mgiuca@chromium.org> wrote:

> On 2013/09/19 00:26:31, dmichael wrote:
>
>> Oh, good point. If you need GetPPResource on the host side, it has to be
>> PP_Resource, not Resource.
>>
>
> So does this mean you agree with keeping it the way it is?

Yes, GetPPResource is fine, and it makes sense that it doesn't increment
the reference count.

I still think the plugin-side ResourceVar should hold a
scoped_refptr<Resource> in order to make sure the resource stays in memory
while the Var does, but maybe Raymes and Yuzhu will override me.


>
>  It also is defensive coding. If the plugin can make that object disappear,
>> you have to deal with that possibility or risk a crash or other security
>> relevant coding error. If you hold an internal reference, you can be more
>> sure the object will be alive.
>>
>
> I'm concerned about the security part. Is there an actual security issue
> you can
> see with this? I think that if the resource refcount is 0, it will be
> removed
> from the resource tracker. Then calling ResourceTracker::GetResource will
> return
> NULL. This could cause some weird issues in our code on the plugin side,
> but I
> don't think it's a) a security problem to have NULL pointers on the
> plugin, or
> b) possible to crash the renderer, since this is an issue specific to the
> plugin.

If we use scoped_refptr, we're ensuring that it will never be NULL, so if
we forget to check, we won't get a NULL dereference. That's all I'm saying.
There's not a specific vulnerability, it's just that we are more likely to
be able to mess up and allow a plugin to cause a NULL pointer dereference
in our code in the plugin process. You're right... since we're talking
about the plugin process, and usually NaCl, I guess it's really not a
security problem. But it just means one more possible state that the
VarResource can be in, and so one more case you have to code for.

I don't really see any advantage to only holding a PP_Resource, and I see
advantages to holding scoped_refptr<Resource>. The argument for only using
a plugin reference on a PP_Resource seems to be "it's not worse, or at
least not much"...  but I don't see how it's better? A little less code?
Wouldn't it be better to not allow the plugin to put us in a weird state
where the VarResource's PP_Resource is not in the Tracker, so we don't have
to code for that possibility?


> So it doesn't seem like the user can do any serious damage by
> over-releasing the resource, any more than they can already do by running
> native
> code in the sandbox.
>
> If you don't have any explicit security concerns, I'd like to move forward
> with
> this. Are there any other outstanding problems with this CL?
>
>
https://codereview.chromium.**org/23809016/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

On 2013/09/19 16:25:57, dmichael wrote:
> 
> If we use scoped_refptr, we're ensuring that it will never be NULL, so if
> we forget to check, we won't get a NULL dereference. That's all I'm saying.
> There's not a specific vulnerability, it's just that we are more likely to
> be able to mess up and allow a plugin to cause a NULL pointer dereference
> in our code in the plugin process. You're right... since we're talking
> about the plugin process, and usually NaCl, I guess it's really not a
> security problem. But it just means one more possible state that the
> VarResource can be in, and so one more case you have to code for.
> 
> I don't really see any advantage to only holding a PP_Resource, and I see
> advantages to holding scoped_refptr<Resource>. The argument for only using
> a plugin reference on a PP_Resource seems to be "it's not worse, or at
> least not much"...  but I don't see how it's better? A little less code?
> Wouldn't it be better to not allow the plugin to put us in a weird state
> where the VarResource's PP_Resource is not in the Tracker, so we don't have
> to code for that possibility?

It just seems unintuitive that:
a) You would have a Var with a Resource in it, but you serialize it, you
serialize the PP_Resource ID, not the Resource itself. and,
b) You would serialize the Resource with an ID that has a refcount of 0.

Again, I'm not sure if this would actually cause obscure problems down the road.
My reasoning, though, is that because we are serializing via the resource ID, it
seems awkward to not store that resource ID and hold a reference to it. If we
didn't serialize the Var, we just put resources in it and took them out again,
then I would agree with you that we should store the Resource object itself, and
hold a reference to the resource (not its ID). But I am thinking of the Var as
being an object representing "what to serialize", and what we are serializing is
a resource ID.

So it is not about more code or less code, but just about coding in what seems
to be a more honest way.

So let's say we go with scoped_refptr<Resource> in the Var. On the next CL, in
the ResourceRawVarData, would you want to store a ScopedPPResource in the
ResourceRawVarData, or would you also have the ResourceRawVarData hold a
scoped_refptr<Resource> on the plugin side?

OK I have changed it so it uses scoped_refptr<Resource>. Please have a look.

I decided with Raymes that it wasn't worth having both a PP_Resource and a
Resource reference -- that seemed pretty awkward and it isn't worth doing both
just so that we can achieve a compromise.

I haven't updated the ResourceRawVarData CL just yet.

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/23809016/93001

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/23809016/93001