Pepper WebSocket API: Fix memory leak issue

ppapi/cpp/var.cc

 // Copyright (c) 2012 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/cpp/var.h"
 
 #include <stdio.h>
 #include <string.h>
 
 #include <algorithm>
@@ skipping 37 matching lines @@
   } else if (has_interface<PPB_Var_1_0>()) {
     return get_interface<PPB_Var_1_0>()->VarFromUtf8(Module::Get()->pp_module(),
                                                      utf8_str,
                                                      len);
   } else {
     return PP_MakeNull();
   }
 }
 
 }  // namespace
 

[Takashi Toyoshima, 2012/09/07 17:18:27]

ppapi/cpp/websocket.cc uses Var::pp_var() to get internal PP_Var, then pass it
to C interface with const_cast. This is not expected for Var, but it looks
difficult to take another approach without any performance loss. I think it
needs another completion callback.

 Var::Var() {
   memset(&var_, 0, sizeof(var_));
   var_.type = PP_VARTYPE_UNDEFINED;
-  needs_release_ = false;
+  needs_release_ = true;

[Takashi Toyoshima, 2012/09/07 17:18:27]

NeedsRefcounting() tell us if the PP_Var requires refcounting.
So needs_release_ should means that the PP_Var resource should be managed by
this class instance.
It means that constructor Var(DontManage, PP_Var) is the only case which set
needs_release_ as false.

[dmichael (off chromium), 2012/09/07 18:36:58]

Currently, it only tells you if the Var's *type* requires reference counting. So
you're changing its meaning...

I think this is a reasonable way to go, but for me, it reinforces that we need
to get rid of DontManage. Then, we wouldn't need to store any data beyond the
PP_Var, and we would always just use NeedsRefcounting() to decide whether to
release.

[Takashi Toyoshima, 2012/09/10 13:21:27]

I meant that we can change the meaning of |needs_release_|. I can keep the
original meaning of NeedsRefcounting() here.

Current implementation, |needs_release_| means that the instance should manage
the internal PP_Var resource, AND the PP_Var is refcounted object. But the
latter condition looks redundant because we can always know it via
NeedsRefcounting().

So my suggestion is to change the meaning of |needs_release_| to only the former
meaning that the PP_Var is passed via DontManage or not. Maybe it's good to
change its name from needs_release_ to needs_manage_.

So 'needs_manage_ && NeedsRefcounting(var_)' is a right condition to release the
internal PP_Var as shown in the revised destructor.

 }
 
 Var::Var(Null) {
   memset(&var_, 0, sizeof(var_));
   var_.type = PP_VARTYPE_NULL;
-  needs_release_ = false;
+  needs_release_ = true;
 }
 
 Var::Var(bool b) {
   var_.type = PP_VARTYPE_BOOL;
   var_.padding = 0;
   var_.value.as_bool = PP_FromBool(b);
-  needs_release_ = false;
+  needs_release_ = true;
 }
 
 Var::Var(int32_t i) {
   var_.type = PP_VARTYPE_INT32;
   var_.padding = 0;
   var_.value.as_int = i;
-  needs_release_ = false;
+  needs_release_ = true;
 }
 
 Var::Var(double d) {
   var_.type = PP_VARTYPE_DOUBLE;
   var_.padding = 0;
   var_.value.as_double = d;
-  needs_release_ = false;
+  needs_release_ = 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);
 }
 
 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);
 }
 
 Var::Var(const Var& other) {
   var_ = other.var_;
+  needs_release_ = 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 (needs_release_ && NeedsRefcounting(var_) && has_interface<PPB_Var_1_0>())

[Takashi Toyoshima, 2012/09/07 17:18:27]

PP_Var leaked when PP_Var was updated after construction.
WebSocket ReceiveMessage completion callback did so.

     get_interface<PPB_Var_1_0>()->Release(var_);
 }
 
 Var& Var::operator=(const Var& other) {
   // Early return for self-assignment. Note however, that two distinct vars
   // can refer to the same object, so we still need to be careful about the
   // refcounting below.
   if (this == &other)
     return *this;
 
   // 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_;
+  needs_release_ = 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 (old_needs_release && NeedsRefcounting(var_))
     get_interface<PPB_Var_1_0>()->Release(var_);
 
   var_ = other.var_;
   return *this;
 }
 
 bool Var::operator==(const Var& other) const {
   if (var_.type != other.var_.type)
     return false;
   switch (var_.type) {
@@ skipping 82 matching lines @@
     snprintf(buf, sizeof(buf), format, str.c_str());
   } else if (is_array_buffer()) {
     snprintf(buf, sizeof(buf), "Var(ARRAY_BUFFER)");
   } else if (is_object()) {
     snprintf(buf, sizeof(buf), "Var(OBJECT)");
   }
   return buf;
 }
 
 }  // namespace pp