Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting. r=bholley, a=NPOTB
authorCameron McCormack <cam@mcc.id.au>
Sat, 29 Jul 2017 11:57:32 +0800
changeset 421015 bf61f9884b5b3b22b5c7d9606cffb5ebffd70be4
parent 421012 706a35d8549e9abc807580e031fc15d1f28c3967
child 421016 c342cab423f6de346ac5c2daeeceebc715b5f84c
push id7578
push userryanvm@gmail.com
push dateSat, 05 Aug 2017 15:50:35 +0000
treeherdermozilla-beta@08ab8a747932 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley, NPOTB
bugs1381731
milestone56.0
Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting. r=bholley, a=NPOTB 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.