Pepper: Var keeps invalid var_id if VarTracker release it and there is another reference

ppapi/shared_impl/var.cc

Index: ppapi/shared_impl/var.cc
diff --git a/ppapi/shared_impl/var.cc b/ppapi/shared_impl/var.cc
index 3bec5ce3ade7468bef7c7ae7a9b4ec62e3c5d95a..ecfb71484678dadcfff8558891fc5beeff3ed096 100644
--- a/ppapi/shared_impl/var.cc
+++ b/ppapi/shared_impl/var.cc
@@ -99,13 +99,17 @@ void Var::AssignVarID(int32 id) {
 
 StringVar::StringVar(const std::string& str)
     : value_(str) {
+  GetOrCreateVarID();
 }
 
 StringVar::StringVar(const char* str, uint32 len)
     : value_(str, len) {
+  GetOrCreateVarID();

[dmichael (off chromium), 2011/12/12 18:28:50]

I think we shouldn't have this ref-count here. If the ref-count can't go to zero
while the object is alive, we get a chicken-and-egg problem and most strings
will leak, since:
1) The object can't get deleted until its reference count goes to 0.
2) The reference count can't go to zero until the object is deleted.

I think I understand the problem now. It's not exactly a bug, but it's
definitely tricky to use the current interface correctly. You're doing 'new
StringVar' in to a scoped_refptr member, which does bump the reference count in
base::RefCounted to 1 correctly. But as you note, the var id is not created, and
it's not registered with the var tracker. I'm trying to think of an elegant way
to handle this. You can certainly make it work; you just need to manage call 
GetPPVar immediately after doing "new StringVar" in PPB_WebSocket_Impl. E.g.:
empty_string_ = new StringVar("", 0);
var_.GetPPVar();  // Keep a reference while the WebSocket is alive

Then, you need to explicitly release any vars you created in the destructor:
PPB_WebSocket_Impl::~PPB_WebSocket_Impl() {
  VarTracker* var_tracker = PpapiGlobals::Get()->GetVarTracker();
  if (empty_string_.get())
    var_tracker->ReleaseVar(empty_string_.GetExistingVarID());
  if (url_.get())
    var_tracker->ReleaseVar(url_.GetExistingVarID());
  if (close_reason_.get())
    var_tracker->ReleaseVar(close_reason_.GetExistingVarID());
}

There probably needs to be a nicer & less confusing way to do this. If you were
using PP_Vars as members as you had originally (before I convinced you to use
StringVar :-p) this wouldn't have happened. I'll put some thought in to how Var
& VarTracker can get refactored...  it's debatable whether there's a bug in the
Var/VarTracker code or not, but it's at best way too easy to use incorrectly.
Now that I poked around a little, I think there's a bug in PPB_FileRef_Shared
that's similar to what you were seeing.

 }
 
 StringVar::~StringVar() {
+  VarTracker* tracker = PpapiGlobals::Get()->GetVarTracker();
+  tracker->ReleaseVar(GetExistingVarID());
 }
 
 StringVar* StringVar::AsStringVar() {
@@ -113,7 +117,7 @@ StringVar* StringVar::AsStringVar() {
 }
 
 PP_Var StringVar::GetPPVar() {
-  int32 id = GetOrCreateVarID();
+  int32 id = GetExistingVarID();
   if (!id)
     return PP_MakeNull();
 
@@ -153,4 +157,3 @@ StringVar* StringVar::FromPPVar(PP_Var var) {
 }
 
 }  // namespace ppapi
-