Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting. r?bholley draft
authorCameron McCormack <cam@mcc.id.au>
Sat, 29 Jul 2017 11:57:32 +0800
changeset 618939 07153dd25bf1d36ec75b45b0e754bbc10bf70422
parent 618757 44121dbcac6a9d3ff18ed087a09b3205e5a04db1
child 640242 97b3bacc1deeb5a709308906e9435b7434f861e5
push id71516
push userbmo:cam@mcc.id.au
push dateTue, 01 Aug 2017 09:33:12 +0000
reviewersbholley
bugs1381731
milestone56.0a1
Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting. r?bholley MozReview-Commit-ID: ByM6mzL1IgF
xpcom/ds/nsAtomTable.cpp
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -458,43 +458,42 @@ DynamicAtom::GCAtomTableLocked(const Mut
 
   }
   if (nonZeroRefcountAtomsCount) {
     nsPrintfCString msg("%d dynamic atom(s) with non-zero refcount: %s",
                         nonZeroRefcountAtomsCount, nonZeroRefcountAtoms.get());
     NS_ASSERTION(nonZeroRefcountAtomsCount == 0, 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.
-  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);
-  }
+  // We would like to assert that gUnusedAtomCount matches the number of atoms
+  // we found in the table which we removed. During the course of this function,
+  // the atom table is locked, but this lock is not acquired for AddRef() and
+  // Release() calls. This means we might see a gUnusedAtomCount value in
+  // between, say, AddRef() incrementing mRefCnt and it decrementing
+  // gUnusedAtomCount. So, we don't bother asserting that there are no unused
+  // atoms at the end of a regular GC. But we can (and do) assert thist just
+  // after the last GC at shutdown.
+  //
+  // Note that, barring refcounting bugs, an atom can only go from a zero
+  // refcount to a non-zero refcount while the atom table lock is held, so
+  // so we won't try to resurrect a zero refcount atom while trying to delete
+  // it.
+
+  MOZ_ASSERT_IF(aKind == GCKind::Shutdown, removedCount == gUnusedAtomCount);
 
   gUnusedAtomCount -= removedCount;
 }
 
 NS_IMPL_QUERY_INTERFACE(DynamicAtom, nsIAtom)
 
 NS_IMETHODIMP_(MozExternalRefCountType)
 DynamicAtom::AddRef(void)
 {
   nsrefcnt count = ++mRefCnt;
   if (count == 1) {
-    MOZ_ASSERT(gUnusedAtomCount > 0);
     gUnusedAtomCount--;
   }
   return count;
 }
 
 #ifdef DEBUG
 // We set a lower GC threshold for atoms in debug builds so that we exercise
 // the GC machinery more often.