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

ppapi/shared_impl/var.cc

 // Copyright (c) 2011 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.
 
 #include "ppapi/shared_impl/var.h"
 
 #include <limits>
 
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
@@ skipping 81 matching lines @@
 
 void Var::AssignVarID(int32 id) {
   DCHECK(!var_id_);  // Must not have already been generated.
   var_id_ = id;
 }
 
 // StringVar -------------------------------------------------------------------
 
 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() {
   return this;
 }
 
 PP_Var StringVar::GetPPVar() {
-  int32 id = GetOrCreateVarID();
+  int32 id = GetExistingVarID();
   if (!id)
     return PP_MakeNull();
 
   PP_Var result;
   result.type = PP_VARTYPE_STRING;
   result.padding = 0;
   result.value.as_id = id;
   return result;
 }
 
@@ skipping 19 matching lines @@
   if (var.type != PP_VARTYPE_STRING)
     return NULL;
   scoped_refptr<Var> var_object(
       PpapiGlobals::Get()->GetVarTracker()->GetVar(var));
   if (!var_object)
     return NULL;
   return var_object->AsStringVar();
 }
 
 }  // namespace ppapi
-