Closed Bug 1401809 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: jkratzer, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev a73cc4e08bf5.
Flags: in-testsuite?
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
Component: XPCOM → CSS Parsing and Computation
Summary: Assertion failure: IsStaticAtom(), at /builds/worker/workspace/build/src/xpcom/ds/nsAtomTable.cpp:267 → stylo: Assertion failure: IsStaticAtom(), at /builds/worker/workspace/build/src/xpcom/ds/nsAtomTable.cpp:267
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.
Priority: -- → P2
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.
Flags: needinfo?(jkratzer)
(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?
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.
Do'h! Sorry I sent a review request for the patch I did confirm what's going on there.
Flags: needinfo?(jkratzer)
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
Attachment #8910605 - Flags: review?(xidorn+moz) → review+
Attached file Servo PR
Thank you Xidorn!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/9caeafcec998
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Attached file log_minidump.txt
Minidump stack
(In reply to Pulsebot from comment #12)
> Pushed by hikezoe@mozilla.com:
> 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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.