1381731 - stylo: Assertion failure: removedCount <= gUnusedAtomCount, at /home/ikezoe/central/xpcom/ds/nsAtomTable.cpp:432

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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

Comment 2

[Cameron McCormack (:heycam)]

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

Comment 3

[Cameron McCormack (:heycam)]

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?

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Intermittent Failures Robot]

3 failures in 822 pushes (0.004 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 3

Platform breakdown:
* linux64-stylo: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1381731&startday=2017-07-17&endday=2017-07-23&tree=all

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 10

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed9f64d124329747f9176c376d81568c56053e64

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting.

Review commit: https://reviewboard.mozilla.org/r/162698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162698/

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8891589 [details]
Bug 1381731 - Remove gUnusedAtomCount assertions since they're hard to get right given OMT atom refcounting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162698/diff/1-2/

Comment 15

[Cameron McCormack (:heycam)]

Does that updated comment sound right?

Comment 16

[Bobby Holley (:bholley)]

SGTM thanks.

Comment 17

[Mozilla Autoland]

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

Comment 18

[Pulsebot]

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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9ec878bd54b7

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf61f9884b5b