Resource tracking done right.

src/shared/ppapi_proxy/plugin_resource_tracker.cc

Index: src/shared/ppapi_proxy/plugin_resource_tracker.cc
diff --git a/src/shared/ppapi_proxy/plugin_resource_tracker.cc b/src/shared/ppapi_proxy/plugin_resource_tracker.cc
index f5d39839128e970ee8b570f91ec4cd98965d92b4..e380bc9d1087159385b092fd0dd0df084b2181a1 100644
--- a/src/shared/ppapi_proxy/plugin_resource_tracker.cc
+++ b/src/shared/ppapi_proxy/plugin_resource_tracker.cc
@@ -7,36 +7,38 @@
 #include <limits>
 #include <set>
 
+#include "native_client/src/include/nacl_macros.h"
+#include "native_client/src/include/portability.h"
+#include "native_client/src/shared/ppapi_proxy/plugin_globals.h"
 #include "native_client/src/shared/ppapi_proxy/plugin_resource.h"
 #include "ppapi/c/pp_resource.h"
+#include "srpcgen/ppb_rpc.h"
 
 namespace ppapi_proxy {
 
-scoped_refptr<PluginResource>
-PluginResourceTracker::GetResource(PP_Resource res) const {
-  ResourceMap::const_iterator result = live_resources_.find(res);
-  if (result == live_resources_.end()) {
-    return scoped_refptr<PluginResource>();
-  }
-  return result->second.first;
+PluginResourceTracker::ResourceAndRefCounts::ResourceAndRefCounts(
+    PluginResource* r) : resource(r), browser_refcount(1), plugin_refcount(1) {
+}
+
+PluginResourceTracker::ResourceAndRefCounts::~ResourceAndRefCounts() {
 }
 
-PluginResourceTracker::PluginResourceTracker()
-    : last_id_(0) {
+scoped_refptr<PluginResource> PluginResourceTracker::GetExistingResource(
+    PP_Resource res) const {
+  ResourceMap::const_iterator result = live_resources_.find(res);
+  if (result == live_resources_.end())
+    return scoped_refptr<PluginResource>();
+  else
+    return result->second.resource;
 }
 
-PluginResourceTracker::~PluginResourceTracker() {
+PluginResourceTracker::PluginResourceTracker() : last_id_(0) {
 }
 
-PP_Resource PluginResourceTracker::AddResource(PluginResource* resource) {
-  // If the plugin manages to create 4B resources...
-  if (last_id_ == std::numeric_limits<PP_Resource>::max()) {
-    return 0;
-  }
+void PluginResourceTracker::AddResource(PluginResource* resource,
+                                        PP_Resource id) {
   // Add the resource with plugin use-count 1.
-  ++last_id_;
-  live_resources_.insert(std::make_pair(last_id_, std::make_pair(resource, 1)));
-  return last_id_;
+  live_resources_.insert(std::make_pair(id, ResourceAndRefCounts(resource)));
 }
 
 bool PluginResourceTracker::AddRefResource(PP_Resource res) {
@@ -47,7 +49,7 @@ bool PluginResourceTracker::AddRefResource(PP_Resource res) {
     // We don't protect against overflow, since a plugin as malicious as to ref
     // once per every byte in the address space could have just as well unrefed
     // one time too many.
-    ++i->second.second;
+    ++i->second.plugin_refcount;

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

nit: would prefer
  i->second.plugin_refcount += 1;
or
  i->second.plugin_refcount++;

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

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.

     return true;
   }
 }
@@ -55,9 +57,13 @@ bool PluginResourceTracker::AddRefResource(PP_Resource res) {
 bool PluginResourceTracker::UnrefResource(PP_Resource res) {
   ResourceMap::iterator i = live_resources_.find(res);
   if (i != live_resources_.end()) {
-    if (!--i->second.second) {
-      i->second.first->StoppedTracking();
+    if (!--i->second.plugin_refcount) {

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

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) { ...

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

Done.

+      size_t browser_refcount = i->second.browser_refcount;
+      i->second.resource->StoppedTracking();
       live_resources_.erase(i);
+
+      // Release all browser references.
+      ReleaseBrowserResource(res, browser_refcount);
     }
     return true;
   } else {
@@ -65,5 +71,22 @@ bool PluginResourceTracker::UnrefResource(PP_Resource res) {
   }
 }
 
+void PluginResourceTracker::ObtainBrowserResource(PP_Resource res) {
+  if (res) {
+    NaClSrpcChannel* channel = ppapi_proxy::GetMainSrpcChannel();
+    PpbCoreRpcClient::PPB_Core_AddRefResource(channel, res);
+  }
+}
+
+void PluginResourceTracker::ReleaseBrowserResource(PP_Resource res,
+                                                   size_t browser_refcount) {
+  // Release all browser references.
+  if (res) {
+    NaClSrpcChannel* channel = ppapi_proxy::GetMainSrpcChannel();
+    PpbCoreRpcClient::ReleaseResourceMultipleTimes(channel, res,
+                                                   browser_refcount);

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

What will happen if the NaCl module releases more times than the browser knew
about?

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

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

+  }
+}
+
 }  // namespace ppapi_proxy