add some debugging to SkNVRefCnt

include/core/SkRefCnt.h

Index: include/core/SkRefCnt.h
diff --git a/include/core/SkRefCnt.h b/include/core/SkRefCnt.h
index 1e31d918cc973727fcdf6b99b5d27a823bba478c..ab716ddf58d83fa3e4f23195f15b818b6e93fa1e 100644
--- a/include/core/SkRefCnt.h
+++ b/include/core/SkRefCnt.h
@@ -253,6 +253,11 @@ template <typename Derived>
 class SkNVRefCnt : SkNoncopyable {
 public:
     SkNVRefCnt() : fRefCnt(1) {}
+#ifdef SK_DEBUG

[mtklein, 2014/11/22 00:08:14]

Double debug guards seem like overkill?  Should be identical without the
#ifdef...#endif.

+    ~SkNVRefCnt() {
+        SkASSERTF(1 == fRefCnt, "NVRefCnt was %d", fRefCnt);
+    }
+#endif
 
     // Implementation is pretty much the same as SkRefCntBase. All required barriers are the same:
     //   - unique() needs acquire when it returns true, and no barrier if it returns false;
@@ -261,7 +266,14 @@ public:
 
     bool unique() const { return 1 == sk_acquire_load(&fRefCnt); }
     void    ref() const { sk_atomic_inc(&fRefCnt); }
-    void  unref() const { if (1 == sk_atomic_dec(&fRefCnt)) { SkDELETE((const Derived*)this); } }
+    void  unref() const {
+        int32_t prevValue = sk_atomic_dec(&fRefCnt);
+        SkASSERT(prevValue >= 1);
+        if (1 == prevValue) {
+            SkDEBUGCODE(fRefCnt = 1;)   // for the assert in our destructor

[bsalomon, 2014/11/21 22:48:50]

Why 1? For stack objects? Do we want to allow that?

[mtklein, 2014/11/22 00:08:14]

I'm game to try disallowing the stack and delete in the spirit of trying new
things.

Would that be best as,
  protected:
    ~SkNVRefCnt() { SkASSERT(0 == fRefCnt); } 
?

For reference, I had been toying with writing unref() like this:
  if (this->unique() || 1 == sk_atomic_dec(&fRefCnt)) {
    SkDELETE((const Derived*)this);
  }
to avoid the last write when we see we're unique (ought to be pretty common). 
This has the nice effect that if we never actually share the object, we never
issue a single atomic write.

If we were to do something like that, the asserts do complicate it, as it's
legal to end up with 0 or 1 in the dtor.  We'd want something like this:
  ~SkNVRefCnt() { SkASSERT(this->unique()); }
  void unref() const {
    if (this->unique()) {
       SkDELETE((const Derived*)this);
    } else if (1 == sk_atomic_dec(&fRefCnt)) {
       SkDEBUGCODE(fRefCnt = 1);
       SkDELETE((const Derived*)this);
    }
  }
That'd also allow direct deleters.

+            SkDELETE((const Derived*)this);
+        }
+    }
     void  deref() const { this->unref(); }  // Chrome prefers to call deref().
     int32_t getRefCnt() const { return fRefCnt; } // Used by Chrome unit tests.