Filed by: rvandermeulen [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=101067270&repo=autoland https://queue.taskcluster.net/v1/task/RqacvgUyRXC7GlUulnK6Pw/runs/0/artifacts/public/logs/live_backing.log https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/RqacvgUyRXC7GlUulnK6Pw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

This looks horrible... Someone is decreasing DynamicAtom's refcnt without invoking its Release method?

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1) > This looks horrible Does this mean someone should eventually be prioritizing investigating it?

Yes, someone should really look into this... Let me try to take a quick look. Just to verify and so I know where to look, this is stylo-only, right?

Yes. Also debug-only. https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1367065&endday=2017-07-28&startday=2017-06-12&tree=trunk

Looking at the code, this crash either indicates that someone decreases DynamicAtom::mRefCnt without invoking DynamicAtom::Release, or someone is holding an nsIAtom pointer without actually owning it, so it can become a dangling pointer. It doesn't seem to me the former is possible in the code, so I guess it is more likely we are doing the latter.

Hmmm... I suspect the violating thing is: https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/servo/components/style/properties/gecko.mako.rs#3074 It isn't clear to me why this Atom::from_addrefed is safe. It doesn't seem to be safe at all?

But even if that is a cause of this, the code doesn't exist before July 4th. But this intermittent started since May...

I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d70e6aa5e688986993740d740d440782886a798 which adds a check that a zero-refcounted atom can only be revived via getting it from atom table, but that assertion doesn't seem to catch anything. Interestingly, this try push actually caught one this assertion, which means we get this assertion isn't because we are reviving a weak atom.

I did another try push in https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc0e0fde59029b9bdd1f0d4e4670f924967093a9 This records all operations on gUnusedAtomCount in the log, so that we can track what happens to it. There is one task which asserts at a different place that we are reviving an atom (refcnt from 0 to 1) when gUnusedAtomCount is zero. From the record, I can see a pattern stating: > DynamicAtom: Reviving `Sans` > DynamicAtom: Reviving `Sans` > DynamicAtom: Killing `Sans` > DynamicAtom: Killing `Sans` which means we are double reviving the atom `Sans`. The position of this pattern is different from the assertion, so it is not clear to me whether they are related, but this kind of pattern doesn't seem to appear in tasks which run normally. I kinda suspect there is some kind of race condition inside AtomTable, and it only appears with Stylo because atom table wasn't under this level of concurrency pressure without parallel styling. (BTW, it is also weird that why we are killing and reviving the `Sans` atom so frequently... It really shouldn't.)

OK, so the assertion I saw above is another intermittent which is bug 1367396. I think these two bugs are caused from the same underlying issue that there is race condition inside AtomTable, although it isn't completely clear to me why this can happen.

Resolving intermittent Stylo test failure bugs that haven't had any recent activity.