cc: Don't leak a ref to SkColorFilter.

skia/ext/refptr.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SKIA_EXT_REFPTR_H_
 #define SKIA_EXT_REFPTR_H_
 
+#include "base/basictypes.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 
 namespace skia {
 
 // When creating/receiving a ref-counted pointer from Skia, wrap that pointer in
 // this class to avoid dealing with the ref-counting and prevent leaks/crashes
 // due to ref-counting bugs.
 //
 // Example of creating a new SkShader* and setting it on a SkPaint:
 //   skia::RefPtr<SkShader> shader = skia::AdoptRef(SkGradientShader::Create());
@@ skipping 19 matching lines @@
 //
 //   skia::RefPtr<SkShader> shader = skia::SharePtr(paint.getShader());
 //
 // Never call ref() or unref() on the underlying ref-counted pointer. If you
 // AdoptRef() the raw pointer immediately into a skia::RefPtr and always work
 // with skia::RefPtr instances instead, the ref-counting will be taken care of
 // for you.
 template<typename T>
 class RefPtr {
  public:
-  RefPtr() : ptr_(NULL) {}
+  RefPtr() : ptr_(NULL) {
+#ifndef NDEBUG
+    inside_receive_and_adopt_ref_ = false;
+#endif
+  }
 
   RefPtr(const RefPtr& other)
       : ptr_(other.get()) {
     SkSafeRef(ptr_);
+#ifndef NDEBUG
+    inside_receive_and_adopt_ref_ = false;
+#endif
   }
 
   template<typename U>
   RefPtr(const RefPtr<U>& other)
       : ptr_(other.get()) {
     SkSafeRef(ptr_);
+#ifndef NDEBUG
+    inside_receive_and_adopt_ref_ = false;
+#endif
   }
 
   ~RefPtr() {
+    ShouldNotBeInsideReceiveAndAdoptRef();
     clear();
   }
 
   RefPtr& operator=(const RefPtr& other) {
+    ShouldNotBeInsideReceiveAndAdoptRef();
     SkRefCnt_SafeAssign(ptr_, other.get());
     return *this;
   }
 
   template<typename U>
   RefPtr& operator=(const RefPtr<U>& other) {
+    ShouldNotBeInsideReceiveAndAdoptRef();
     SkRefCnt_SafeAssign(ptr_, other.get());
     return *this;
   }
 
   void clear() {
+    ShouldNotBeInsideReceiveAndAdoptRef();
     T* to_unref = ptr_;
     ptr_ = NULL;
     SkSafeUnref(to_unref);
   }
 
-  T* get() const { return ptr_; }
-  T& operator*() const { return *ptr_; }
-  T* operator->() const { return ptr_; }
+  T* get() const {
+    ShouldNotBeInsideReceiveAndAdoptRef();
+    return ptr_;
+  }
+  T& operator*() const {
+    ShouldNotBeInsideReceiveAndAdoptRef();
+    return *ptr_;
+  }
+  T* operator->() const {
+    ShouldNotBeInsideReceiveAndAdoptRef();
+    return ptr_;
+  }
 
   typedef T* RefPtr::*unspecified_bool_type;
   operator unspecified_bool_type() const {
+    ShouldNotBeInsideReceiveAndAdoptRef();
     return ptr_ ? &RefPtr::ptr_ : NULL;
   }
 
+  class Receiver {
+   public:
+    ~Receiver() {
+      refptr_->ShouldBeEmpty();
+      refptr_->ptr_ = ptr_;
+      refptr_->inside_receive_and_adopt_ref_ = false;
+    }
+
+    operator T**() { return &ptr_; }
+
+   private:
+    friend class RefPtr;
+
+    explicit Receiver(RefPtr* refptr) : refptr_(refptr), ptr_(NULL) {
+      refptr_->ShouldBeEmpty();
+    }
+
+    RefPtr* refptr_;
+    T* ptr_;
+
+    DISALLOW_COPY_AND_ASSIGN(Receiver);
+  };
+
+  // Call this and pass the returned temporary it to a function taking a T**
+  // argument. The returned temporary will set the original RefPtr to an object
+  // with an unowned reference.
+  // NOTE: Do not use this function as part of a subexpression where the RefPtr
+  // is also used somewhere else. Destructors of temporaries are only run at the
+  // end of a full expression so the initialization will not be complete and
+  // the RefPtr will remain empty.

[Stephen White, 2013/07/18 15:50:30]

Maybe I'm old-fashioned, but I'm wondering if this is just too clever. Would it
be so bad to simply continue to pass a raw pointer to the function that expects
one, and then AdoptRef() it into a skia::RefPtr? There wouldn't be any explicit
ref/unref, so I don't think it would violate any of the principles we've set for
ourselves. The only ugliness would be the existing raw pointer, but it's in the
same scope as the RefPtr, so at least its lifetime is clear. And we wouldn't
need all this code and checking. JMHO.

[danakj, 2013/07/18 15:59:36]

My own preference would be to never receive something skia refcounted into a raw
pointer ever, and to raise reviewer alarm bells when a raw pointer appears.

RawPtr* foo = NULL;
asRawPtr(&foo);
skia::RefPtr<RawPtr> foo_ref = skia::AdoptRef(foo);

Compared to:

skia::RefPtr<RawPtr> foo;
asRawPtr(foo.ReceiveAndAdoptRef());

In the end they are the same, but I would prefer to provide primitives that
don't require raw refcounted pointers.

[awong, 2013/07/18 16:00:42]

Yes...I agree but there's precedent (bleck).  See Tommi and Darin's posts on
this thread about scoped_ptr:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RTd7rNxHjqk/1WHHv...

Eventually we left it out of scoped_ptr.  At the time, no one thought about the
lifetime wierdnesses either.

Not sure if this context is helpful or not in coming to a decision. 

+  //
+  // Bad (refptr will be NULL when it is dereffed):
+  //   Foo(refptr.ReceiveAndAdoptRef()) && refptr->Bar();
+  // Good:
+  //   bool ok = Foo(refptr.ReceiveAndAdoptRef());
+  //   ok && refptr->Bar();
+  Receiver ReceiveAndAdoptRef() {
+#ifndef NDEBUG
+    inside_receive_and_adopt_ref_ = true;
+#endif
+    return Receiver(this);
+  }
+
  private:
   T* ptr_;
 
+#ifndef NDEBUG
+  bool inside_receive_and_adopt_ref_;
+#endif
+
+  inline int ShouldNotBeInsideReceiveAndAdoptRef() const {
+#ifndef NDEBUG
+    if (inside_receive_and_adopt_ref_) {
+      // Crash. Can't use DCHECK here because blink includes this file.
+      volatile int* a = NULL;
+      return *a;
+    }
+#endif
+    return 0;
+  }
+
+  inline int ShouldBeEmpty() const {
+#ifndef NDEBUG
+    if (ptr_ != NULL) {
+      // Crash. Can't use DCHECK here because blink includes this file.
+      volatile int* a = NULL;
+      return *a;
+    }
+#endif
+    return 0;
+  }
+
   // This function cannot be public because Skia starts its ref-counted
   // objects at refcnt=1.  This makes it impossible to differentiate
   // between a newly created object (that doesn't need to be ref'd) or an
   // already existing object with one owner (that does need to be ref'd so that
   // this RefPtr can also manage its lifetime).
-  explicit RefPtr(T* ptr) : ptr_(ptr) {}
+  explicit RefPtr(T* ptr) : ptr_(ptr) {
+#ifndef NDEBUG
+    inside_receive_and_adopt_ref_ = false;
+#endif
+  }
 
   template<typename U>
   friend RefPtr<U> AdoptRef(U* ptr);
 
   template<typename U>
   friend RefPtr<U> SharePtr(U* ptr);
 };
 
 // For objects that have an unowned reference (such as newly created objects).
 template<typename T>
 RefPtr<T> AdoptRef(T* ptr) { return RefPtr<T>(ptr); }
 
 // For objects that are already owned. This doesn't take ownership of existing
 // references and adds a new one.
 template<typename T>
 RefPtr<T> SharePtr(T* ptr) { return RefPtr<T>(SkSafeRef(ptr)); }
 
 }  // namespace skia
 
 #endif  // SKIA_EXT_REFPTR_H_