Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer. draft
authorAndrew McCreight <continuation@gmail.com>
Fri, 14 Jul 2017 10:20:23 -0700
changeset 609151 e99219c30ad0be67f5630fd08ae15b5b038620a3
parent 609150 63f58c15949460f41886ae006172c17158e32003
child 637519 53acaf01778b341f6e59ef7a45a07a1434f6bc42
push id68522
push userbmo:continuation@gmail.com
push dateFri, 14 Jul 2017 21:55:09 +0000
bugs1350729
milestone56.0a1
Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer. Running with XPCOM_MEM_LOG_CLASSES=nsStringBuffer triggers an assertion in refcount logging for nsFakeStringBuffers. These are given an initial refcount of 1, without calling NS_LOG_ADDREF. Then, AddRef() is called on these objects in StaticAtom::StaticAtom(), and we tell the refcount logging system about the fake buffer, and that it has a refcount of 0, triggering the assertion. The first part of the fix is to call NS_LOG_ADDREF for this initial refcount, in StaticAtom(). This first fix causes refcount logging to start reporting that the fake string buffers leak, when XPCOM_MEM_LOG_CLASSES is not set. This is because refcount logging is now getting told about these objects being AddRefed at 1, which it takes to mean that an object is created. To work around this issue, I add an array gFakeBuffers that contains every fake string buffer we create, and tell the refcount logging system that these objects are all being destroyed, when the atom table is being shut down. This could result in some bogosity if the fake buffers are "leaked" but hopefully this is still an improvement over the current state. MozReview-Commit-ID: 5AxoBYAlYRU
xpcom/ds/nsAtomTable.cpp
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -127,24 +127,65 @@ private:
   // DynamicAtom* pointer (in GCAtomTable()), not an nsIAtom* pointer.
   ~DynamicAtom();
 
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIATOM
 };
 
+#if defined(NS_BUILD_REFCNT_LOGGING)
+// nsFakeStringBuffers don't really use the refcounting system, but we
+// have to give a coherent series of addrefs and releases to the
+// refcount logging system, or we'll hit assertions when running with
+// XPCOM_MEM_LOG_CLASSES=nsStringBuffer.
+class FakeBufferRefcountHelper
+{
+public:
+  FakeBufferRefcountHelper(nsStringBuffer* aBuffer)
+    : mBuffer(aBuffer)
+  {
+    // Account for the initial static refcount of 1, so that we don't
+    // hit a refcount logging assertion when this object first appears
+    // with a refcount of 2.
+    NS_LOG_ADDREF(aBuffer, 1, "nsStringBuffer", sizeof(nsStringBuffer));
+  }
+
+  ~FakeBufferRefcountHelper()
+  {
+    // We told the refcount logging system in the ctor that this
+    // object was created, so now we have to tell it that it was
+    // destroyed, to avoid leak reports. This may cause odd the
+    // refcount isn't actually 0.
+    NS_LOG_RELEASE(mBuffer, 0, "nsStringBuffer");
+  }
+
+private:
+  nsStringBuffer* mBuffer;
+};
+
+UniquePtr<nsTArray<FakeBufferRefcountHelper>> gFakeBuffers;
+#endif
+
 class StaticAtom final : public nsIAtom
 {
 public:
   StaticAtom(nsStringBuffer* aStringBuffer, uint32_t aLength, uint32_t aHash)
   {
     mLength = aLength;
     mIsStatic = true;
     mString = static_cast<char16_t*>(aStringBuffer->Data());
+
+#if defined(NS_BUILD_REFCNT_LOGGING)
+    if (!gFakeBuffers) {
+      gFakeBuffers = MakeUnique<nsTArray<FakeBufferRefcountHelper>>();
+    }
+    gFakeBuffers->AppendElement(aStringBuffer);
+#endif
+
     // Technically we could currently avoid doing this addref by instead making
     // the static atom buffers have an initial refcount of 2.
     aStringBuffer->AddRef();
 
     mHash = aHash;
     MOZ_ASSERT(mHash == HashString(mString, mLength));
 
     MOZ_ASSERT(mString[mLength] == char16_t(0), "null terminated");
@@ -559,16 +600,20 @@ NS_InitAtomTable()
     NS_STATIC_ATOM(empty, &empty_atom)
   };
   NS_RegisterStaticAtoms(default_atoms);
 }
 
 void
 NS_ShutdownAtomTable()
 {
+#if defined(NS_BUILD_REFCNT_LOGGING)
+  gFakeBuffers = nullptr;
+#endif
+
   delete gStaticAtomTable;
   gStaticAtomTable = nullptr;
 
 #ifdef NS_FREE_PERMANENT_DATA
   // Do a final GC to satisfy leak checking. We skip this step in release
   // builds.
   {
     MutexAutoLock lock(*gAtomTableLock);