Fix some egregious bugs in Var.

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95690

[brettw, 2011-08-04 23:10:50]

http://codereview.chromium.org/7511026/diff/1/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

http://codereview.chromium.org/7511026/diff/1/ppapi/cpp/var.h#newcode260
ppapi/cpp/var.h:260: originally_had_exception_(v && !v->is_undefined()) {
It sucks this is in two places, I filed
http://code.google.com/p/chromium/issues/detail?id=91751 for this

[darin (slow to review), 2011-08-04 23:20:30]

On Thu, Aug 4, 2011 at 4:10 PM, <brettw@chromium.org> wrote:

> Reviewers: darin,
>
>
>
http://codereview.chromium.**org/7511026/diff/1/ppapi/cpp/**var.h<http://code...
> File ppapi/cpp/var.h (right):
>
> http://codereview.chromium.**org/7511026/diff/1/ppapi/cpp/**
>
var.h#newcode260<http://codereview.chromium.org/7511026/diff/1/ppapi/cpp/var.h#newcode260>
> ppapi/cpp/var.h:260: originally_had_exception_(v && !v->is_undefined())
> {
> It sucks this is in two places, I filed
>
http://code.google.com/p/**chromium/issues/detail?id=**91751<http://code.goog...
this
>
> Description:
> 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
>
> Please review this at
http://codereview.chromium.**org/7511026/<http://codereview.chromium.org/7511...
>
> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>
> Affected files:
>  M     ppapi/cpp/private/var_private.**h
>  M     ppapi/cpp/var.h
>  M     ppapi/cpp/var.cc
>  M     ppapi/tests/test_var.cc
>
>
> Index: ppapi/cpp/private/var_private.**h
> ==============================**==============================**=======
> --- ppapi/cpp/private/var_private.**h     (revision 95308)
> +++ ppapi/cpp/private/var_private.**h     (working copy)
> @@ -86,7 +86,7 @@
>    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 {
> Index: ppapi/cpp/var.cc
> ==============================**==============================**=======
> --- ppapi/cpp/var.cc    (revision 95308)
> +++ ppapi/cpp/var.cc    (working copy)
> @@ -117,20 +117,21 @@
>  }
>
>  Var& Var::operator=(const Var& other) {
>

should we have an early return here?

 if (this == &other)
   return *this;



> -  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;
> -    }
> +  // 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;
>  }
>
> Index: ppapi/cpp/var.h
> ==============================**==============================**=======
> --- ppapi/cpp/var.h     (revision 95308)
> +++ ppapi/cpp/var.h     (working copy)
> @@ -257,7 +257,7 @@
>     /// 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 {
> Index: ppapi/tests/test_var.cc
> ==============================**==============================**=======
> --- ppapi/tests/test_var.cc     (revision 95308)
> +++ ppapi/tests/test_var.cc     (working copy)
> @@ -64,6 +64,13 @@
>     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);
>
>
>

LGTM