Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer. r=dbaron draft
authorAndrew McCreight <continuation@gmail.com>
Fri, 14 Jul 2017 10:20:23 -0700
changeset 609916 cbfe33f44fa03de434bee3674aeb01079e5f887c
parent 609865 e0b0865639cebc1b5afa0268a4b073fcdde0e69c
child 637723 1cefe366afa0e320e12e4021a2eab033bd8e3e22
push id68739
push userbmo:continuation@gmail.com
push dateMon, 17 Jul 2017 19:21:58 +0000
reviewersdbaron
bugs1350729
milestone56.0a1
Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer. r=dbaron 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,66 @@ 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:
+  explicit 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)
+    MOZ_ASSERT(NS_IsMainThread());
+    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 +601,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);