Fix some egregious bugs in Var.

Self-assignment was broken and would lose the reference. I uncovered this when
running a test. It outputted a warning to the console, but we never looked at
it. I made a more explicit test.

This also fixes output exceptions. The OutException helper class detected
whether the existing object had an exception or not incorrectly. This was
exposed when var assignment was fixed.

TEST=included
BUG=none
Review URL: http://codereview.chromium.org/7511026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@95690 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 27 insertions, 13 deletions

diff --git a/ppapi/cpp/private/var_private.h b/ppapi/cpp/private/var_private.h
index b2f4204..860f643 100644
--- a/ppapi/cpp/private/var_private.h
+++ b/ppapi/cpp/private/var_private.h
@@ -86,7 +86,7 @@ class VarPrivate : public Var {
    public:
     OutException(Var* v)
         : output_(v),
-          originally_had_exception_(v && v->is_null()) {
+          originally_had_exception_(v && !v->is_undefined()) {
       if (output_) {
         temp_ = output_->pp_var();
       } else {
diff --git a/ppapi/cpp/var.cc b/ppapi/cpp/var.cc
index 8df9b67..683a344 100644
--- a/ppapi/cpp/var.cc
+++ b/ppapi/cpp/var.cc
@@ -117,20 +117,27 @@ Var::~Var() {
 }
 
 Var& Var::operator=(const Var& other) {
-  if (needs_release_ && has_interface<PPB_Var>())
-    get_interface<PPB_Var>()->Release(var_);
-  var_ = other.var_;
-  if (NeedsRefcounting(var_)) {
-    if (has_interface<PPB_Var>()) {
-      needs_release_ = true;
-      get_interface<PPB_Var>()->AddRef(var_);
-    } else {
-      var_.type = PP_VARTYPE_NULL;
-      needs_release_ = false;
-    }
+  // 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_;
+  if (NeedsRefcounting(other.var_)) {
+    // Assume we already has_interface<PPB_Var> for refcounted vars or else we
+    // couldn't have created them in the first place.
+    needs_release_ = true;
+    get_interface<PPB_Var>()->AddRef(other.var_);
   } else {
     needs_release_ = false;
   }
+  if (old_needs_release)
+    get_interface<PPB_Var>()->Release(var_);
+
+  var_ = other.var_;
   return *this;
 }
 
diff --git a/ppapi/cpp/var.h b/ppapi/cpp/var.h
index 8ad831a0..1ed2984 100644
--- a/ppapi/cpp/var.h
+++ b/ppapi/cpp/var.h
@@ -257,7 +257,7 @@ class Var {
     /// A constructor.
     OutException(Var* v)
         : output_(v),
-          originally_had_exception_(v && v->is_null()) {
+          originally_had_exception_(v && !v->is_undefined()) {
       if (output_) {
         temp_ = output_->var_;
       } else {
diff --git a/ppapi/tests/test_var.cc b/ppapi/tests/test_var.cc
index 3e15795..9e91dc4 100644
--- a/ppapi/tests/test_var.cc
+++ b/ppapi/tests/test_var.cc
@@ -64,6 +64,13 @@ std::string TestVar::TestBasicString() {
     ASSERT_EQ(NULL, result);
   }
 
+  // Make sure we can assign a C++ object to itself and it stays alive.
+  {
+    pp::Var a("test");
+    a = a;
+    ASSERT_TRUE(a.AsString() == "test");
+  }
+
   // Make sure nothing leaked.
   ASSERT_TRUE(testing_interface_->GetLiveObjectsForInstance(
       instance_->pp_instance()) == before_object);