Resource tracking done right.

Resource tracking done right.

BUG=none
TEST=graphics2d (in another CL) uses it and works.

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=3980

[neb, 2010-12-08 23:41:36]

This is the first part of the 2D patch, the resource rework. I plan on adding a
bunch of comments to it first thing tomorrow morning, it's not obvious how it
works and what its invariants are.

[neb, 2010-12-09 23:52:49]

Adding Nicholas, as this applies to Audio as well.

[sehr (please use chromium), 2010-12-10 01:09:18]

A few comments and questions.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.cc:87: browser_refcount);
What will happen if the NaCl module releases more times than the browser knew
about?

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.h (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.h:67: void
ReleaseBrowserResource(PP_Resource res, size_t refcount = 1);
I don't think default parameters are allowed by the style guide.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.h:75: size_t plugin_refcount;
You've used size_t here all over.  Will it ever matter that size_t in untrusted
code may have a different size than in trusted code?

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/ppb_...
File src/shared/ppapi_proxy/ppb_core.srpc (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/ppb_...
src/shared/ppapi_proxy/ppb_core.srpc:23: # resource. This does not exist in
PPB_Core, we implement it as an
Why would we ever need more than one reference to a resource from untrusted
code?  The previous version only sent RPCs for 0 --> 1 and 1 --> 0 transitions.

[neb, 2010-12-10 20:43:31]

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.cc:87: browser_refcount);
On 2010/12/10 01:09:18, sehr wrote:
> What will happen if the NaCl module releases more times than the browser knew
> about?

NaCl module doesn't call this function directly, but UnrefResource(), which will
release only as many resources as the plugin owns.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.h (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.h:67: void
ReleaseBrowserResource(PP_Resource res, size_t refcount = 1);
On 2010/12/10 01:09:18, sehr wrote:
> I don't think default parameters are allowed by the style guide.

Done.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.h:75: size_t plugin_refcount;
On 2010/12/10 01:09:18, sehr wrote:
> You've used size_t here all over.  Will it ever matter that size_t in
untrusted
> code may have a different size than in trusted code?

Nope, could have been an int for all I care. Just tracks the number of
references, and it makes no sense to guard against overflow there.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/ppb_...
File src/shared/ppapi_proxy/ppb_core.srpc (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/ppb_...
src/shared/ppapi_proxy/ppb_core.srpc:23: # resource. This does not exist in
PPB_Core, we implement it as an
On 2010/12/10 01:09:18, sehr wrote:
> Why would we ever need more than one reference to a resource from untrusted
> code?  The previous version only sent RPCs for 0 --> 1 and 1 --> 0
transitions.

When the PPAPI layer returns Resources from function calls, it always gives the
caller one reference to it. If, for instance, we did:

Resource r1 = audio.config();
Resource r2 = audio.config();

we would consume two browser-side references to the same AudioConfig Resource
object. We could issue a RPC to decrease the refcount the very same moment we
see that we get a refcount to an object that we already track, I chose to block
less and just release them all at the end.

[nfullagar, 2010-12-14 00:39:28]

a couple nits

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource.h (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource.h:22: F(PluginImageData)
Perhaps not for this CL, but it is starting to nag me how many places scattered
across PPAPI code where "if you add a new PPAPI device/resource, add yourself to
this list/function/table/etc..."

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.cc:52:
++i->second.plugin_refcount;
nit: would prefer
  i->second.plugin_refcount += 1;
or
  i->second.plugin_refcount++;

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.cc:60: if
(!--i->second.plugin_refcount) {
I'm not a huge fan of !--i->second.plugin_refcount, which itself is a bit
awkward to parse, let alone sitting inside an if (statement).  It is also a
chore to single step in a debugger.  I'd prefer something along the lines of:
  i->second.plugin_refcount -= 1;  // or i->second.plugin_refcount--;
  if (0 == i->second.plugin_refcount) { ...

[sehr (please use chromium), 2010-12-14 00:52:20]

On 2010/12/14 00:39:28, nfullagar wrote:
> a couple nits
> 
>
http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
> File src/shared/ppapi_proxy/plugin_resource.h (right):
> 
>
http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
> src/shared/ppapi_proxy/plugin_resource.h:22: F(PluginImageData)
> Perhaps not for this CL, but it is starting to nag me how many places
scattered
> across PPAPI code where "if you add a new PPAPI device/resource, add yourself
to
> this list/function/table/etc..."
> 
>
http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
> File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):
> 
>
http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
> src/shared/ppapi_proxy/plugin_resource_tracker.cc:52:
> ++i->second.plugin_refcount;
> nit: would prefer
>   i->second.plugin_refcount += 1;
> or
>   i->second.plugin_refcount++;
> 
>
http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
> src/shared/ppapi_proxy/plugin_resource_tracker.cc:60: if
> (!--i->second.plugin_refcount) {
> I'm not a huge fan of !--i->second.plugin_refcount, which itself is a bit
> awkward to parse, let alone sitting inside an if (statement).  It is also a
> chore to single step in a debugger.  I'd prefer something along the lines of:
>   i->second.plugin_refcount -= 1;  // or i->second.plugin_refcount--;
>   if (0 == i->second.plugin_refcount) { ...

Modulo nfullagar's comments, LGTM.

[neb, 2010-12-14 23:03:46]

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource.h (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource.h:22: F(PluginImageData)
On 2010/12/14 00:39:28, nfullagar wrote:
> Perhaps not for this CL, but it is starting to nag me how many places
scattered
> across PPAPI code where "if you add a new PPAPI device/resource, add yourself
to
> this list/function/table/etc..."

I know of only two - this file and pepper_resource.h that this file is a copy
of. There's also GetInterface(), but that one's obvious. The solutions are
limited by the mandate to not have static initializers, so we can't really make
these lists autogenerate themselves.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.cc:52:
++i->second.plugin_refcount;
On 2010/12/14 00:39:28, nfullagar wrote:
> nit: would prefer
>   i->second.plugin_refcount += 1;
> or
>   i->second.plugin_refcount++;

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

The rationale is that compilers will not optimize it correctly if the class has
custom operator++. I use it everywhere for consistency.

http://codereview.chromium.org/5581011/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.cc:60: if
(!--i->second.plugin_refcount) {
On 2010/12/14 00:39:28, nfullagar wrote:
> I'm not a huge fan of !--i->second.plugin_refcount, which itself is a bit
> awkward to parse, let alone sitting inside an if (statement).  It is also a
> chore to single step in a debugger.  I'd prefer something along the lines of:
>   i->second.plugin_refcount -= 1;  // or i->second.plugin_refcount--;
>   if (0 == i->second.plugin_refcount) { ...

Done.

[neb, 2010-12-15 20:26:09]

Changed prefix->postfix

[nfullagar, 2010-12-15 20:55:58]

if (0x0000FF00 == trybot.color()) lgtm(); else ptal();