Pepper WebSocket API: Fix memory leak issue

ppapi/cpp/var.cc

Index: ppapi/cpp/var.cc
diff --git a/ppapi/cpp/var.cc b/ppapi/cpp/var.cc
index 3e76593bd2763ff5fec11c110bef977ea0a8dc1a..6c288e904850eee3a7c56f2f5d11e66e20765c0c 100644
--- a/ppapi/cpp/var.cc
+++ b/ppapi/cpp/var.cc
@@ -59,65 +59,63 @@ PP_Var VarFromUtf8Helper(const char* utf8_str, uint32_t len) {
 Var::Var() {
   memset(&var_, 0, sizeof(var_));
   var_.type = PP_VARTYPE_UNDEFINED;
-  needs_release_ = false;
+  needs_management_ = true;
 }
 
 Var::Var(Null) {
   memset(&var_, 0, sizeof(var_));
   var_.type = PP_VARTYPE_NULL;
-  needs_release_ = false;
+  needs_management_ = true;
 }
 
 Var::Var(bool b) {
   var_.type = PP_VARTYPE_BOOL;
   var_.padding = 0;
   var_.value.as_bool = PP_FromBool(b);
-  needs_release_ = false;
+  needs_management_ = true;
 }
 
 Var::Var(int32_t i) {
   var_.type = PP_VARTYPE_INT32;
   var_.padding = 0;
   var_.value.as_int = i;
-  needs_release_ = false;
+  needs_management_ = true;
 }
 
 Var::Var(double d) {
   var_.type = PP_VARTYPE_DOUBLE;
   var_.padding = 0;
   var_.value.as_double = d;
-  needs_release_ = false;
+  needs_management_ = true;
 }
 
 Var::Var(const char* utf8_str) {
   uint32_t len = utf8_str ? static_cast<uint32_t>(strlen(utf8_str)) : 0;
   var_ = VarFromUtf8Helper(utf8_str, len);
-  needs_release_ = (var_.type == PP_VARTYPE_STRING);
+  needs_management_ = true;
 }
 
 Var::Var(const std::string& utf8_str) {
   var_ = VarFromUtf8Helper(utf8_str.c_str(),
-                            static_cast<uint32_t>(utf8_str.size()));
-  needs_release_ = (var_.type == PP_VARTYPE_STRING);
+                           static_cast<uint32_t>(utf8_str.size()));
+  needs_management_ = true;
 }
 
 Var::Var(const Var& other) {
   var_ = other.var_;
+  needs_management_ = other.needs_management_;

[dmichael (off chromium), 2012/09/10 16:21:14]

Shouldn't this always just be true, with the new meaning of needs_management_?
At present, "needs_release_" is not copied, so if you copy an Unmanaged Var, the
new copy still takes a reference and later needs to release it.

(You're getting leaks right now in the few tests that use DontManage vars, and I
think this is why.)

[Takashi Toyoshima, 2012/09/11 08:38:15]

Oops, this was wrong.
I revert this line to be always true.

   if (NeedsRefcounting(var_)) {
-    if (has_interface<PPB_Var_1_0>()) {
-      needs_release_ = true;
+    if (has_interface<PPB_Var_1_0>())
       get_interface<PPB_Var_1_0>()->AddRef(var_);
-    } else {
+    else
       var_.type = PP_VARTYPE_NULL;
-      needs_release_ = false;
-    }
-  } else {
-    needs_release_ = false;
   }
 }
 
 Var::~Var() {
-  if (needs_release_ && has_interface<PPB_Var_1_0>())
+  if (NeedsRefcounting(var_) &&
+      needs_management_ &&
+      has_interface<PPB_Var_1_0>())
     get_interface<PPB_Var_1_0>()->Release(var_);
 }
 
@@ -130,16 +128,14 @@ Var& Var::operator=(const Var& other) {
 
   // Be careful to keep the ref alive for cases where we're assigning an
   // object to itself by addrefing the new one before releasing the old one.
-  bool old_needs_release = needs_release_;
+  bool old_needs_management_ = needs_management_;

[dmichael (off chromium), 2012/09/10 16:21:14]

no trailing underscore for local variables

[Takashi Toyoshima, 2012/09/11 08:38:15]

Done.

+  needs_management_ = true;
   if (NeedsRefcounting(other.var_)) {
     // Assume we already has_interface<PPB_Var_1_0> for refcounted vars or else
     // we couldn't have created them in the first place.
-    needs_release_ = true;
     get_interface<PPB_Var_1_0>()->AddRef(other.var_);
-  } else {
-    needs_release_ = false;
   }
-  if (old_needs_release)
+  if (NeedsRefcounting(var_) && old_needs_management_)
     get_interface<PPB_Var_1_0>()->Release(var_);
 
   var_ = other.var_;