Enable debug builds in the Android framework

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
 #define SkRefCnt_DEFINED
 
@@ skipping 14 matching lines @@
 public:
     /** Default construct, initializing the reference count to 1.
     */
     SkRefCntBase() : fRefCnt(1) {}
 
     /** Destruct, asserting that the reference count is 1.
     */
     virtual ~SkRefCntBase() {
 #ifdef SK_DEBUG
         SkASSERTF(fRefCnt == 1, "fRefCnt was %d", fRefCnt);
         fRefCnt = 0;    // illegal value, to catch us if we reuse after delete

[bungeman-skia, 2015/08/20 22:18:13]

This check may have less weight now that we use valgrind and the various *sans.
However, if we have a change like this, maybe this should be changed to -1?

[bungeman-skia, 2015/08/21 18:22:33]

Changing SkRefCnt to support start at 1, 0 means disposed, -1 means checked
destructed, all goes well until unique(). If unique() can return true for a
reference count of 0 or 1 things go haywire, and for good reason. In the case
where a factory is holding the SkRefCnt object without holding a ref itself (so
that it can be notified when there are no outstanding refs) it cannot legally
ref(), except from 0 to 1. The reason for this is that it has, effectively,
stolen unique(). Even the managed type itself cannot use unique() on itself, as
even if unique() returns true, the cache may still (against the API
specification) ref its bare pointer to the object (without holding a valid
reference). As a result, no one other that the cache can use unique(), including
the SkRefCnt class' implementation.

 #endif
     }
 
 #ifdef SK_DEBUG
     /** Return the reference count. Use only for debugging. */
     int32_t getRefCnt() const { return fRefCnt; }
 #endif
 
     /** May return true if the caller is the only owner.
      *  Ensures that all previous owner's actions are complete.
      */
     bool unique() const {
         if (1 == sk_atomic_load(&fRefCnt, sk_memory_order_acquire)) {
             // The acquire barrier is only really needed if we return true.  It
             // prevents code conditioned on the result of unique() from running
             // until previous owners are all totally done calling unref().
             return true;
         }
         return false;
     }
 
     /** Increment the reference count. Must be balanced by a call to unref().
     */
     void ref() const {
+#ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK
+        // Android employs some special subclasses that enable the fRefCnt to
+        // go to zero, but not below, prior to reusing the object.

[bungeman-skia, 2015/08/21 21:44:35]

So after much back and forth and looking into this, SkRefCnt can't really
support what they're trying to do without something breaking. I'm ok with this
change for now if we add to this comment

// This breaks the use of unique() on such objects.
// This should be removed once the Android code is fixed.

[djsollen, 2015/08/24 12:25:43]

Done.

+        SkASSERT(fRefCnt >= 0);
+#else
         SkASSERT(fRefCnt > 0);
+#endif
         (void)sk_atomic_fetch_add(&fRefCnt, +1, sk_memory_order_relaxed);  // No barrier required.
     }
 
     /** Decrement the reference count. If the reference count is 1 before the
         decrement, then delete the object. Note that if this is the case, then
         the object needs to have been allocated via new, and not on the stack.
     */
     void unref() const {
         SkASSERT(fRefCnt > 0);
         // A release here acts in place of all releases we "should" have been doing in ref().
         if (1 == sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel)) {

[bungeman-skia, 2015/08/20 21:55:13]

Not that I'm advocating this, but as a note to the future, what seems to be
wanted (and what has been desired before) is something like

int prev = sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel);
if (2 == prev) {
    this->become_unique();
} else if (1 == prev) {
    this->internal_dispose();
}

where become_unique() is a protected method. The idea is to allow factory caches
to write wrappers that do something when they are the only owners. This is
essentially what the Android code is doing, which is using the current
internal_dispose() as the proposed 'become_unique()', and using unique_ptr as
the actual 'internal_dispose()'. This is something like allowing weak reference
counting, but with only one weak reference (holdable by the factory).

[bungeman-skia, 2015/08/21 18:22:33]

Note that 'become_unique()' cannot work correctly, as it would need to exclude
competing refs. In other words, internal_dispose() works because the current
thread was the only owning thread, but become_unique() cannot work because it is
running on the thread which just gave up the last shared reference, but the
remaining reference may be in the process of grabbing another reference. The
code in Android gets around this by using a full mutex around this last
reference.

             // Like unique(), the acquire is only needed on success, to make sure
             // code in internal_dispose() doesn't happen before the decrement.
             this->internal_dispose();
         }
     }
 
 #ifdef SK_DEBUG
     void validate() const {
         SkASSERT(fRefCnt > 0);
     }
@@ skipping 156 matching lines @@
             SkDELETE((const Derived*)this);
         }
     }
     void  deref() const { this->unref(); }
 
 private:
     mutable int32_t fRefCnt;
 };
 
 #endif