Bug 1276669 - Part 11: Strengthen assertions for atom table shutdown GC. r=erahm, a=ritu
authorNathan Froyd <froydnj@mozilla.com>
Thu, 26 Jan 2017 16:43:38 -0400
changeset 375828 b4ed5ea97d2553ad845bf31e746d767b4bffa50a
parent 375827 8ee370bffd15ed59c401285dac62e7f24d4f15ef
child 375829 f27e061cee9d2e6fc8f11bb2df254f365caca1ef
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm, ritu
bugs1276669
milestone53.0a2
Bug 1276669 - Part 11: Strengthen assertions for atom table shutdown GC. r=erahm, a=ritu This could have been done more simply, but the small amount of refactoring that takes place in this comment enables better error messages in the case where something does go wrong.
xpcom/ds/nsAtomTable.cpp
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -19,16 +19,17 @@
 #include "nsCRT.h"
 #include "PLDHashTable.h"
 #include "prenv.h"
 #include "nsThreadUtils.h"
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
 #include "nsAutoPtr.h"
 #include "nsUnicharUtils.h"
+#include "nsPrintfCString.h"
 
 // There are two kinds of atoms handled by this module.
 //
 // - DynamicAtom: the atom itself is heap allocated, as is the nsStringBuffer it
 //   points to. |gAtomTable| holds weak references to them DynamicAtoms. When
 //   the refcount of a DynamicAtom drops to zero, we increment a static counter.
 //   When that counter reaches a certain threshold, we iterate over the atom
 //   table, removing and deleting DynamicAtoms with refcount zero. This allows
@@ -74,16 +75,24 @@ public:
   static already_AddRefed<DynamicAtom> Create(const nsAString& aString, uint32_t aHash)
   {
     // The refcount is appropriately initialized in the constructor.
     return dont_AddRef(new DynamicAtom(aString, aHash));
   }
 
   static void GCAtomTable();
 
+  enum class GCKind {
+    RegularOperation,
+    Shutdown,
+  };
+
+  static void GCAtomTableLocked(const MutexAutoLock& aProofOfLock,
+                                GCKind aKind);
+
 private:
   DynamicAtom(const nsAString& aString, uint32_t aHash)
     : mRefCnt(1)
   {
     mLength = aString.Length();
     mIsStatic = false;
     RefPtr<nsStringBuffer> buf = nsStringBuffer::FromString(aString);
     if (buf) {
@@ -354,39 +363,63 @@ static const PLDHashTableOps AtomTableOp
 };
 
 //----------------------------------------------------------------------
 
 void
 DynamicAtom::GCAtomTable()
 {
   MutexAutoLock lock(*gAtomTableLock);
+  GCAtomTableLocked(lock, GCKind::RegularOperation);
+}
+
+void
+DynamicAtom::GCAtomTableLocked(const MutexAutoLock& aProofOfLock,
+                               GCKind aKind)
+{
   uint32_t removedCount = 0; // Use a non-atomic temporary for cheaper increments.
   for (auto i = gAtomTable->Iter(); !i.Done(); i.Next()) {
     auto entry = static_cast<AtomTableEntry*>(i.Get());
-    if (!entry->mAtom->IsStaticAtom()) {
-      auto atom = static_cast<DynamicAtom*>(entry->mAtom);
-      if (atom->mRefCnt == 0) {
-        i.Remove();
-        delete atom;
-        ++removedCount;
-      }
+    if (entry->mAtom->IsStaticAtom()) {
+      continue;
+    }
+
+    auto atom = static_cast<DynamicAtom*>(entry->mAtom);
+    if (atom->mRefCnt == 0) {
+      i.Remove();
+      delete atom;
+      ++removedCount;
+    } else if (aKind == GCKind::Shutdown) {
+      // We only perform these kind of GCs in leak-checking builds.  If
+      // something is anomalous, then we'll report an error here, and crash
+      // later on in this function.
+      nsAutoCString name;
+      atom->ToUTF8String(name);
+      nsPrintfCString msg("dynamic atom with non-zero refcount %s!", name.get());
+      NS_ASSERTION(false, msg.get());
     }
   }
 
   // During the course of this function, the atom table is locked. This means
   // that, barring refcounting bugs in consumers, an atom can never go from
   // refcount == 0 to refcount != 0 during a GC. However, an atom _can_ go from
   // refcount != 0 to refcount == 0 if a Release() occurs in parallel with GC.
   // This means that we cannot assert that gUnusedAtomCount == removedCount, and
   // thus that there are no unused atoms at the end of a GC. We can and do,
   // however, assert this after the last GC at shutdown.
-  MOZ_ASSERT(removedCount <= gUnusedAtomCount);
+  if (aKind == GCKind::RegularOperation) {
+    MOZ_ASSERT(removedCount <= gUnusedAtomCount);
+  } else {
+    // Complain if somebody adds new GCKind enums.
+    MOZ_ASSERT(aKind == GCKind::Shutdown);
+    // Our unused atom count should be accurate.
+    MOZ_ASSERT(removedCount == gUnusedAtomCount);
+  }
+
   gUnusedAtomCount -= removedCount;
-
 }
 
 NS_IMPL_QUERY_INTERFACE(DynamicAtom, nsIAtom)
 
 NS_IMETHODIMP_(MozExternalRefCountType)
 DynamicAtom::AddRef(void)
 {
   nsrefcnt count = ++mRefCnt;
@@ -496,23 +529,22 @@ void
 NS_ShutdownAtomTable()
 {
   delete gStaticAtomTable;
   gStaticAtomTable = nullptr;
 
 #ifdef NS_FREE_PERMANENT_DATA
   // Do a final GC to satisfy leak checking. We skip this step in release
   // builds.
-  DynamicAtom::GCAtomTable();
-  MOZ_ASSERT(gUnusedAtomCount == 0);
+  {
+    MutexAutoLock lock(*gAtomTableLock);
+    DynamicAtom::GCAtomTableLocked(lock, DynamicAtom::GCKind::Shutdown);
+  }
 #endif
 
-  // XXXbholley: it would be good to assert gAtomTable->EntryCount() == 0
-  // here, but that currently fails. Probably just a few things that need
-  // to be fixed up.
   delete gAtomTable;
   gAtomTable = nullptr;
   delete gAtomTableLock;
   gAtomTableLock = nullptr;
 }
 
 void
 NS_SizeOfAtomTablesIncludingThis(MallocSizeOf aMallocSizeOf,