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

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 {

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;
 }
 

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 {

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);