Bug 1313351 - Fix js::RefCounted to not do leak checking. r=jandem
authorLuke Wagner <luke@mozilla.com>
Fri, 23 Dec 2016 21:13:26 +0100
changeset 453594 47d317cfa2672e3e843da7a369072590460e96c6
parent 453593 1f3cc2963f093a5a3bdff31382f5c5fe6d24ee9f
child 453595 806029a2c31dc314b01911b22c75702597a160e9
push id39711
push userdmitchell@mozilla.com
push dateFri, 23 Dec 2016 21:59:47 +0000
reviewersjandem
bugs1313351
milestone53.0a1
Bug 1313351 - Fix js::RefCounted to not do leak checking. r=jandem
js/public/RefCounted.h
js/src/jsapi.h
mfbt/RefCounted.h
--- a/js/public/RefCounted.h
+++ b/js/public/RefCounted.h
@@ -2,26 +2,90 @@
  * vim: set ts=8 sts=4 et sw=4 tw=99:
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef js_RefCounted_h
 #define js_RefCounted_h
 
-#include "mozilla/RefCounted.h"
+#include "mozilla/Atomics.h"
+#include "mozilla/RefCountType.h"
 
 #include "js/Utility.h"
 
+// These types implement the same interface as mozilla::(Atomic)RefCounted and
+// must be used instead of mozilla::(Atomic)RefCounted for everything in
+// SpiderMonkey. There are two reasons:
+//  - Release() needs to call js_delete, not delete
+//  - SpiderMonkey does not have MOZILLA_INTERNAL_API defined which can lead
+//    to ODR violations that show up as spurious leak reports when ref-counted
+//    types are allocated by SpiderMonkey and released by Gecko (or vice versa).
+
 namespace js {
 
-// Replacement for mozilla::RefCounted and mozilla::external::AtomicRefCounted
-// that default to JS::DeletePolicy.
+template <typename T>
+class RefCounted
+{
+    static const MozRefCountType DEAD = 0xffffdead;
+
+  protected:
+    RefCounted() : mRefCnt(0) {}
+    ~RefCounted() { MOZ_ASSERT(mRefCnt == DEAD); }
+
+  public:
+    void AddRef() const
+    {
+        MOZ_ASSERT(int32_t(mRefCnt) >= 0);
+        ++mRefCnt;
+    }
+
+    void Release() const
+    {
+      MOZ_ASSERT(int32_t(mRefCnt) > 0);
+      MozRefCountType cnt = --mRefCnt;
+      if (0 == cnt) {
+#ifdef DEBUG
+          mRefCnt = DEAD;
+#endif
+          js_delete(const_cast<T*>(static_cast<const T*>(this)));
+      }
+    }
+
+  private:
+    mutable MozRefCountType mRefCnt;
+};
 
-template <typename T, typename D = JS::DeletePolicy<T>>
-using RefCounted = mozilla::RefCounted<T, D>;
+template <typename T>
+class AtomicRefCounted
+{
+    static const MozRefCountType DEAD = 0xffffdead;
+
+  protected:
+    AtomicRefCounted() : mRefCnt(0) {}
+    ~AtomicRefCounted() { MOZ_ASSERT(mRefCnt == DEAD); }
+
+  public:
+    void AddRef() const
+    {
+        MOZ_ASSERT(int32_t(mRefCnt) >= 0);
+        ++mRefCnt;
+    }
 
-template <typename T, typename D = JS::DeletePolicy<T>>
-using AtomicRefCounted = mozilla::external::AtomicRefCounted<T, D>;
+    void Release() const
+    {
+        MOZ_ASSERT(int32_t(mRefCnt) > 0);
+        MozRefCountType cnt = --mRefCnt;
+        if (0 == cnt) {
+#ifdef DEBUG
+            mRefCnt = DEAD;
+#endif
+            js_delete(const_cast<T*>(static_cast<const T*>(this)));
+        }
+    }
+
+  private:
+    mutable mozilla::Atomic<MozRefCountType> mRefCnt;
+};
 
 } // namespace js
 
 #endif /* js_RefCounted_h */
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -6103,17 +6103,16 @@ SetBuildIdOp(JSContext* cx, BuildIdOp bu
  * bytecode file descriptor and, if valid, the compiled-code file descriptor.
  * The JS::WasmObject is then transported to the JSRuntime thread (which
  * originated the request) and the wrapping WebAssembly.Module object is created
  * by calling createObject().
  */
 
 struct WasmModule : js::AtomicRefCounted<WasmModule>
 {
-    MOZ_DECLARE_REFCOUNTED_TYPENAME(WasmModule)
     virtual ~WasmModule() {}
 
     virtual void serializedSize(size_t* maybeBytecodeSize, size_t* maybeCompiledSize) const = 0;
     virtual void serialize(uint8_t* maybeBytecodeBegin, size_t maybeBytecodeSize,
                            uint8_t* maybeCompiledBegin, size_t maybeCompiledSize) const = 0;
 
     virtual JSObject* createObject(JSContext* cx) = 0;
 };
--- a/mfbt/RefCounted.h
+++ b/mfbt/RefCounted.h
@@ -11,17 +11,16 @@
 
 #include "mozilla/AlreadyAddRefed.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Atomics.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/Move.h"
 #include "mozilla/RefCountType.h"
 #include "mozilla/TypeTraits.h"
-#include "mozilla/UniquePtr.h"
 
 #if defined(MOZILLA_INTERNAL_API)
 #include "nsXPCOM.h"
 #endif
 
 #if defined(MOZILLA_INTERNAL_API) && \
     (defined(DEBUG) || defined(FORCE_BUILD_REFCNT_LOGGING))
 #define MOZ_REFCOUNTED_LEAK_CHECKING
@@ -48,16 +47,19 @@ namespace mozilla {
  * to 0, the RefCounted<T> "dies" and is destroyed.  The "destroyed"
  * state is represented in DEBUG builds by refcount==0xffffdead.  This
  * state distinguishes use-before-ref (refcount==0) from
  * use-after-destroy (refcount==0xffffdead).
  *
  * Note that when deriving from RefCounted or AtomicRefCounted, you
  * should add MOZ_DECLARE_REFCOUNTED_TYPENAME(ClassName) to the public
  * section of your class, where ClassName is the name of your class.
+ *
+ * Note: SpiderMonkey should use js::RefCounted instead since that type
+ * will use appropriate js_delete and also not break ref-count logging.
  */
 namespace detail {
 const MozRefCountType DEAD = 0xffffdead;
 
 // When building code that gets compiled into Gecko, try to use the
 // trace-refcount leak logging facilities.
 #ifdef MOZ_REFCOUNTED_LEAK_CHECKING
 class RefCountLogger
@@ -82,17 +84,17 @@ public:
 
 // This is used WeakPtr.h as well as this file.
 enum RefCountAtomicity
 {
   AtomicRefCount,
   NonAtomicRefCount
 };
 
-template<typename T, RefCountAtomicity Atomicity, typename D>
+template<typename T, RefCountAtomicity Atomicity>
 class RefCounted
 {
 protected:
   RefCounted() : mRefCnt(0) {}
   ~RefCounted() { MOZ_ASSERT(mRefCnt == detail::DEAD); }
 
 public:
   // Compatibility with nsRefPtr.
@@ -128,17 +130,17 @@ public:
     if (0 == cnt) {
       // Because we have atomically decremented the refcount above, only
       // one thread can get a 0 count here, so as long as we can assume that
       // everything else in the system is accessing this object through
       // RefPtrs, it's safe to access |this| here.
 #ifdef DEBUG
       mRefCnt = detail::DEAD;
 #endif
-      D()(const_cast<T*>(static_cast<const T*>(this)));
+      delete static_cast<const T*>(this);
     }
   }
 
   // Compatibility with wtf::RefPtr.
   void ref() { AddRef(); }
   void deref() { Release(); }
   MozRefCountType refCount() const { return mRefCnt; }
   bool hasOneRef() const
@@ -167,18 +169,18 @@ private:
 // two small inline functions which will hopefully get eliminated by the linker
 // in non-leak-checking builds.
 #define MOZ_DECLARE_REFCOUNTED_TYPENAME(T) \
   const char* typeName() const { return #T; } \
   size_t typeSize() const { return sizeof(*this); }
 
 } // namespace detail
 
-template<typename T, class D = DefaultDelete<T>>
-class RefCounted : public detail::RefCounted<T, detail::NonAtomicRefCount, D>
+template<typename T>
+class RefCounted : public detail::RefCounted<T, detail::NonAtomicRefCount>
 {
 public:
   ~RefCounted()
   {
     static_assert(IsBaseOf<RefCounted, T>::value,
                   "T must derive from RefCounted<T>");
   }
 };
@@ -187,19 +189,19 @@ namespace external {
 
 /**
  * AtomicRefCounted<T> is like RefCounted<T>, with an atomically updated
  * reference counter.
  *
  * NOTE: Please do not use this class, use NS_INLINE_DECL_THREADSAFE_REFCOUNTING
  * instead.
  */
-template<typename T, typename D = DefaultDelete<T>>
+template<typename T>
 class AtomicRefCounted :
-  public mozilla::detail::RefCounted<T, mozilla::detail::AtomicRefCount, D>
+  public mozilla::detail::RefCounted<T, mozilla::detail::AtomicRefCount>
 {
 public:
   ~AtomicRefCounted()
   {
     static_assert(IsBaseOf<AtomicRefCounted, T>::value,
                   "T must derive from AtomicRefCounted<T>");
   }
 };