add some debugging to SkNVRefCnt

include/core/SkRefCnt.h

 
 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkRefCnt_DEFINED
@@ skipping 235 matching lines @@
     SkAutoUnref(SkRefCnt* obj) : SkAutoTUnref<SkRefCnt>(obj) {}
 };
 #define SkAutoUnref(...) SK_REQUIRE_LOCAL_VAR(SkAutoUnref)
 
 // This is a variant of SkRefCnt that's Not Virtual, so weighs 4 bytes instead of 8 or 16.
 // There's only benefit to using this if the deriving class does not otherwise need a vtable.
 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;
     //   - ref() doesn't need any barrier;
     //   - unref() needs a release barrier, and an acquire if it's going to call delete.
 
     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.
 
 private:
     mutable int32_t fRefCnt;
 };
 
 #endif