cc: Don't leak a ref to SkColorFilter.

skia/ext/refptr.h

Index: skia/ext/refptr.h
diff --git a/skia/ext/refptr.h b/skia/ext/refptr.h
index a3900f61ba4502c0d59c3b8664cf6bb4dd3c4aa1..7a9ed5a0b0840514fe2e14d4761c0a1d1db864a3 100644
--- a/skia/ext/refptr.h
+++ b/skia/ext/refptr.h
@@ -5,6 +5,7 @@
 #ifndef SKIA_EXT_REFPTR_H_
 #define SKIA_EXT_REFPTR_H_
 
+#include "base/basictypes.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 
 namespace skia {
@@ -44,58 +45,155 @@ namespace skia {
 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);