We have already bugs for this assertion but they all are test failures. I was hit by this assertion when I try to reproduce bug 1381431 (watching a movie on https://www.zdf.de/comedy/neo-magazin-mit-jan-boehmermann/neo-magazin-royale-mit-jan-boehmermann-vom-22-juni-2017-100.html and click full-screen button repreatedly) #7 0x00007f8996d6a1f7 in DynamicAtom::GCAtomTableLocked (aProofOfLock=..., aKind=DynamicAtom::GCKind::RegularOperation) at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:432 #8 0x00007f8996d69e25 in DynamicAtom::GCAtomTable () at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:370 #9 0x00007f8996d6a449 in DynamicAtom::Release (this=0x7f89648efc40) at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:471 #10 0x00007f8996d766bb in nsCOMPtr<nsIAtom>::~nsCOMPtr (this=0x7f896734ebd0, __in_chrg=<optimized out>) at /home/ikezoe/central/obj-firefox/dist/include/nsCOMPtr.h:404 #11 0x00007f8999bec27b in mozilla::JSEventHandler::~JSEventHandler (this=0x7f896734ebb0, __in_chrg=<optimized out>) at /home/ikezoe/central/dom/events/JSEventHandler.cpp:44 #12 0x00007f8999bec2b4 in mozilla::JSEventHandler::~JSEventHandler (this=0x7f896734ebb0, __in_chrg=<optimized out>) at /home/ikezoe/central/dom/events/JSEventHandler.cpp:48 #13 0x00007f8999bec9c6 in mozilla::JSEventHandler::DeleteCycleCollectable (this=0x7f896734ebb0) at /home/ikezoe/central/dom/events/JSEventHandler.cpp:102 #14 0x00007f8999bf5253 in mozilla::JSEventHandler::cycleCollection::DeleteCycleCollectable (this=0x7f89a2e33ab0 <mozilla::JSEventHandler::_cycleCollectorGlobal>, p=0x7f896734ebb0) at /home/ikezoe/central/obj-firefox/dist/include/mozilla/JSEventHandler.h:255 #15 0x00007f8996d337e8 in SnowWhiteKiller::~SnowWhiteKiller (this=0x7ffc42645d70, __in_chrg=<optimized out>) at /home/ikezoe/central/xpcom/base/nsCycleCollector.cpp:2661 #16 0x00007f8996d2282b in nsCycleCollector::FreeSnowWhite (this=0x7f89ac5a7ec0, aUntilNoSWInPurpleBuffer=false) at /home/ikezoe/central/xpcom/base/nsCycleCollector.cpp:2847 #17 0x00007f8996d26806 in nsCycleCollector_doDeferredDeletion () at /home/ikezoe/central/xpcom/base/nsCycleCollector.cpp:4187 #18 0x00007f8997f057f3 in AsyncFreeSnowWhite::Run (this=0x7f89923aedc0) at /home/ikezoe/central/js/xpconnect/src/XPCJSRuntime.cpp:126 #19 0x00007f8996e22dff in IdleRunnableWrapper::Run (this=0x7f896477a3d0) at /home/ikezoe/central/xpcom/threads/nsThreadUtils.cpp:313

Again with different stack; #0 0x00007fffe12eb1f7 in DynamicAtom::GCAtomTableLocked (aProofOfLock=..., aKind=DynamicAtom::GCKind::RegularOperation) at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:432 #1 0x00007fffe12eae25 in DynamicAtom::GCAtomTable () at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:370 #2 0x00007fffe12eb449 in DynamicAtom::Release (this=0x7fffb91c3850) at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:471 #3 0x00007fffe2facaaf in nsAttrValue::Reset (this=0x7fffffff7a20) at /home/ikezoe/central/dom/base/nsAttrValue.cpp:236 #4 0x00007fffe2fc8ef4 in nsAttrValue::ResetIfSet (this=0x7fffffff7a20) at /home/ikezoe/central/dom/base/nsAttrValueInlines.h:216 #5 0x00007fffe2fac894 in nsAttrValue::~nsAttrValue (this=0x7fffffff7a20, __in_chrg=<optimized out>) at /home/ikezoe/central/dom/base/nsAttrValue.cpp:169 #6 0x00007fffe2eefc06 in mozilla::dom::Element::SetAttr (this=0x7fffadab1aa0, aNamespaceID=0, aName=0x7fffd825e5b0, aPrefix=0x0, aValue=..., aNotify=true) at /home/ikezoe/central/dom/base/Element.cpp:2438 #7 0x00007fffe26dc54a in nsIContent::SetAttr (this=0x7fffadab1aa0, aNameSpaceID=0, aName=0x7fffd825e5b0, aValue=..., aNotify=true) at /home/ikezoe/central/dom/base/nsIContent.h:373 #8 0x00007fffe56cf922 in mozilla::ScrollFrameHelper::SetCoordAttribute (this=0x7fffad9ca1f0, aContent=0x7fffadab1aa0, aAtom=0x7fffd825e5b0, aSize=82200) at /home/ikezoe/central/layout/generic/nsGfxScrollFrame.cpp:5738 #9 0x00007fffe56cd82f in mozilla::ScrollFrameHelper::FinishReflowForScrollbar (this=0x7fffad9ca1f0, aContent=0x7fffadab1aa0, aMinXY=0, aMaxXY=82200, aCurPosXY=14400, aPageIncrement=57540, aIncrement=2880) at /home/ikezoe/central/layout/generic/nsGfxScrollFrame.cpp:5299 #10 0x00007fffe56cde01 in mozilla::ScrollFrameHelper::ReflowFinished (this=0x7fffad9ca1f0) at /home/ikezoe/central/layout/generic/nsGfxScrollFrame.cpp:5389 #11 0x00007fffe551036a in mozilla::PresShell::HandlePostedReflowCallbacks (this=0x7fffae5cc000, aInterruptible=true) at /home/ikezoe/central/layout/base/PresShell.cpp:4028 #12 0x00007fffe5522f40 in mozilla::PresShell::DidDoReflow (this=0x7fffae5cc000, aInterruptible=true) at /home/ikezoe/central/layout/base/PresShell.cpp:9168 #13 0x00007fffe5509254 in mozilla::PresShell::ResizeReflowIgnoreOverride (this=0x7fffae5cc000, aWidth=115200, aHeight=60420, aOldWidth=115200, aOldHeight=64800) at /home/ikezoe/central/layout/base/PresShell.cpp:1993 #14 0x00007fffe5508e81 in mozilla::PresShell::ResizeReflow (this=0x7fffae5cc000, aWidth=115200, aHeight=60420, aOldWidth=115200, aOldHeight=64800) at /home/ikezoe/central/layout/base/PresShell.cpp:1921 #15 0x00007fffe50aaa57 in nsViewManager::DoSetWindowDimensions (this=0x7fffae6dbe00, aWidth=115200, aHeight=60420) at /home/ikezoe/central/view/nsViewManager.cpp:192 #16 0x00007fffe50aad9a in nsViewManager::FlushDelayedResize (this=0x7fffae6dbe00, aDoReflow=true) at /home/ikezoe/central/view/nsViewManager.cpp:244 #17 0x00007fffe5510c26 in mozilla::PresShell::DoFlushPendingNotifications (this=0x7fffae5cc000, aFlush=...) at /home/ikezoe/central/layout/base/PresShell.cpp:4229 #18 0x00007fffe54d30e8 in nsIPresShell::FlushPendingNotifications (this=0x7fffae5cc000, aType=...) at /home/ikezoe/central/layout/base/nsIPresShell.h:587 #19 0x00007fffe55104bc in mozilla::PresShell::DoFlushPendingNotifications (this=0x7fffae5cc000, aType=mozilla::FlushType::Layout) at /home/ikezoe/central/layout/base/PresShell.cpp:4069 #20 0x00007fffe2d0747b in nsIPresShell::FlushPendingNotifications (this=0x7fffae5cc000, aType=mozilla::FlushType::Layout) at /home/ikezoe/central/obj-firefox/dist/include/nsIPresShell.h:578 #21 0x00007fffe301dd61 in nsDocument::FlushPendingNotifications (this=0x7fffc1709000, aType=mozilla::FlushType::Layout) at /home/ikezoe/central/dom/base/nsDocument.cpp:8089 #22 0x00007fffe2eef1c0 in mozilla::dom::Element::GetPrimaryFrame (this=0x7fffaccd5820, aType=mozilla::FlushType::Layout) at /home/ikezoe/central/dom/base/Element.cpp:2262 #23 0x00007fffe2eeac48 in mozilla::dom::Element::GetClientRects (this=0x7fffaccd5820) at /home/ikezoe/central/dom/base/Element.cpp:1053 #24 0x00007fffe3c3e38d in mozilla::dom::ElementBinding::getClientRects (cx=0x7fffd822f000, obj=..., self=0x7fffaccd5820, args=...) at /home/ikezoe/central/obj-firefox/dom/bindings/ElementBinding.cpp:2143 #25 0x00007fffe3f0b48d in mozilla::dom::GenericBindingMethod (cx=0x7fffd822f000, argc=0, vp=0x7fffffff8338) at /home/ikezoe/central/dom/bindings/BindingUtils.cpp:3060

I get this on youtube after following the STR in bug 1379617 comment 6, with the same stack as comment 1.

Bobby, I'm wondering whether this assertion is valid. Let's say the atom table has 19 dynamic atoms with a refcount of 0 (one less than kAtomGCThreshold) and another two dynamic atoms with a refcount of 1. gUnusedAtomCount == 19. Then, on the main thread, we release one of the refcount 1 atoms, so we decide to lock the table and GC. Before we start iterating through the table, another thread decides to release the other refcount 1 atom. In DynamicAtom::Release, we might decrement mRefCnt, but before we before we get to increment gUnusedAtomCount, the main thread finishes its GC work, discovering 21 refcount 0 atoms to delete. At the end of GCAtomTableLocked, removedCount == 21 and gUnusedAtomCount == 20, triggering the assertion. If that's right, is there any better assertion we could make here, or should we remove it?

Hm, that's a good point. How about we spin in debug builds until that condition is met? We'd end up with a hang rather than an assertion if the condition was actually violated, but that might be ok.

(It might be more user-friendly to use a 10-second timer instead)

We could also remove the assertion I suppose. If we allow a long window then it increases the chance that some random other worker thread will free enough atoms to push us over the threshold. So it may not be worth it.

I guess I'm more inclined to remove the assertions than make them spin.

I am seeing this assertion come up more and more on test runs, usually WPT. It would help improve test stability if we make a change here.

Comment on attachment 8891589 [details] Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting. https://reviewboard.mozilla.org/r/162698/#review168394 ::: xpcom/ds/nsAtomTable.cpp:466 (Diff revision 1) > 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 > + if (aKind == GCKind::Shutdown) { Please add a comment / preserve the parts of the old comment explaining why we only do this checking on shutdown. ::: xpcom/ds/nsAtomTable.cpp:467 (Diff revision 1) > // Complain if somebody adds new GCKind enums. > - MOZ_ASSERT(aKind == GCKind::Shutdown); > + MOZ_ASSERT(aKind == GCKind::RegularOperation); This doesn't make sense.

Does that updated comment sound right?

SGTM thanks.

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: repository is read only remote: Repo closed per glandium's request; sheriff not around to mark it closed, so deploying the read-only hammer. remote: refusing to add changesets remote: prechangegroup.readonly hook failed abort: push failed on remote

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9ec878bd54b7 Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting. r=bholley