1401809 - stylo: Assertion failure: IsStaticAtom(), at /builds/worker/workspace/build/src/xpcom/ds/nsAtomTable.cpp:267

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev a73cc4e08bf5.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

If I understand the ref count handling between nsIAtom and Atom in servo, we need to increment the ref count for nsIAtom if it's dynamic. Right?

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

That's correct. We need to addref for the specific place you fix in the try push. But I don't understand how would that trigger this assertion.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I can reproduce an assertion mRefCnt > 0, but doesn't see the IsStaticAtom() assertion.

Could you provide the stack you see this assertion?

hiro's proposed fix does indeed fix an existing refcnt issue, but that doesn't seem to be related to the assertion mentioned here.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> I can reproduce an assertion mRefCnt > 0, but doesn't see the IsStaticAtom()
> assertion.

Yeah, me too as far as I open the test case on firefox.  But I can somehow reproduce it with the crash test in the try.

#01: nsIAtom::Release() (/home/ikezoe/stylo/xpcom/ds/nsAtomTable.cpp:267 (discriminator 1))
#02: Gecko_ReleaseAtom (/home/ikezoe/stylo/layout/style/ServoBindings.cpp:1218)
#03: style::gecko_string_cache::{{impl}}::drop (/home/ikezoe/stylo/servo/components/style/gecko_string_cache/mod.rs:305)
#04: core::ptr::drop_in_place<style::gecko_string_cache::Atom> (/checkout/src/libcore/ptr.rs:60)
#05: core::ptr::drop_in_place<style::values::CustomIdent> (/checkout/src/libcore/ptr.rs:60)
#06: core::ptr::drop_in_place<[style::values::CustomIdent]> (/checkout/src/libcore/ptr.rs:60)

It seems that the atom has already broken there?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1401809 - Use Atom::from(nsIAtom) to increment reference count in case of dynamic atom for will-change.

If we don't increment the reference count for the Atom in servo side, it's
possible to try to release the Atom in servo side even if we have already
released in gecko side.  When it happens, nsIAtom::mKind is no longer reliable.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

After thinking a bit more, I guess I understand now. So it is a use-after-free, when we are lucky enough that an atom table gc is triggered after the first release but before the second one. In that case, it is just a dangling pointer, and the content can be anything.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Do'h! Sorry I sent a review request for the patch I did confirm what's going on there.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8910605 [details]
Bug 1401809 - Use Atom::from(nsIAtom) to increment reference count in case of dynamic atom for will-change.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8910605 [details]
Bug 1401809 - Use Atom::from(nsIAtom) to increment reference count in case of dynamic atom for will-change.

https://reviewboard.mozilla.org/r/182046/#review187396

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Thank you Xidorn!

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8910605 [details]
Bug 1401809 - Use Atom::from(nsIAtom) to increment reference count in case of dynamic atom for will-change.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9caeafcec998
Use Atom::from(nsIAtom) to increment reference count in case of dynamic atom for will-change. r=xidorn

Comment 13

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

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

Comment 14

[Jason Kratzer [:jkratzer]]

Attachment: log_minidump.txt

Minidump stack

Comment 15

[Bobby Holley (:bholley)]

(In reply to Pulsebot from comment #12)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/9caeafcec998
> Use Atom::from(nsIAtom) to increment reference count in case of dynamic atom
> for will-change. r=xidorn

The actual code change was https://hg.mozilla.org/integration/autoland/rev/77ce63ef8271