Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer. r=dbaron
authorAndrew McCreight <continuation@gmail.com>
Fri, 14 Jul 2017 10:20:23 -0700
changeset 369254 3bc3266f999ad7b1f9d504abe4b017912ba38210
parent 369253 1d1f8fefbb425e1a0498e873cd84d00385f823bc
child 369255 5c928291e2d5051284d8858486d054b4c524bd18
push id32194
push userryanvm@gmail.com
push dateTue, 18 Jul 2017 14:46:42 +0000
treeherdermozilla-central@8ff4f17b266d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron
bugs1350729
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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);