1. STYLO_FORCE_ENABLED=1 2. http://www.kongregate.com/games/graebor/sort-the-court in debug build on Linux 3. Crash Crash reason: EXCEPTION_ILLEGAL_INSTRUCTION Crash address: 0x1046a8f3 Assertion: Unknown assertion type 0x00000000 Process uptime: not available Thread 0 (crashed) 0 xul.dll!panic_abort::__rust_start_panic [lib.rs:0ade339411587887bf01bcfa2e9ae4414c8900d4 : 55 + 0x3] eip = 0x1046a8f3 esp = 0x0102ed00 ebp = 0x0102ed00 ebx = 0x0102ee70 esi = 0x14ab8bc8 edi = 0x1024dc30 eax = 0x00000011 ecx = 0x15cf1dc0 edx = 0x14ac0610 efl = 0x00010202 thread '<unnamed>' panicked at '<div id=subwrap> (0xb264dc0) has still dirty bit true or animation-only dirty bit false', Z:/build/build/src/servo/ports/geckolib/glue.rs:3311 This is with a fresh build from https://hg.mozilla.org/mozilla-central/rev/e336d84fc1d2d1fde7387dd5f86fe06fa59abe10 log contains sections ==== Marionette Log ==== marionette stuff crash report ==== Gecko Log ==== gecko log stuff

This might be the same as bug 1394586. Bob, can you reproduce with that patch applied?

I lost the ability to reproduce with the original url with or without the patch. Fortunately, I had a number of other urls to try. With the patch, https://www.neowin.net/ reproduces thread '<unnamed>' panicked at '<div id=global_below_community_activity class="ad"> (0x7f9f718fcdc0) has still dirty bit true or animation-only dirty bit false', /mozilla/builds/nightly/mozilla/servo/ports/geckolib/glue.rs:3311 I still have 18 more urls in case that one stops working. ;-)

Could you post them all in this bug? Tomorrow I'll investigate these, and it'd be helpful to see if the others repro too, to be able to work on them sooner if they happen to be different bugs. Thanks!

(ni? me so I don't forget about this bug)

I ran these quickly through with some reloading to see if that would help crash. I marked the ones that reproduced a related dirty panic. One has an unrelated assertion I have yet to file. Also note, some of these originally reproduced only on Windows.

Thank you. I've been distracted today fixing bustage, but I debugged this and found a few sources of fishyness in the current restyle root patch. This crash is a regression from that. I have fixes in progress.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b5aae17a20cf297339093d09862a8c1693cdc85&selectedJob=127112211 That looks green, modulo some extra assertions I added that also fail on trunk, and I'll address separately.

Comment on attachment 8902917 [details] Bug 1394935: Add a special case for marking something as dirty from invalidation code. https://reviewboard.mozilla.org/r/174636/#review179706

Comment on attachment 8902918 [details] Bug 1394935: Add a (commented out for now) assertion about clobbering dirty bits. https://reviewboard.mozilla.org/r/174638/#review179708 File a bug on this just so we don't lose track of it? r=me with that.

Comment on attachment 8902919 [details] Bug 1394935: Assert that the content we're marking dirty is under the restyle root. https://reviewboard.mozilla.org/r/174640/#review179712 ::: commit-message-b1339:3 (Diff revision 1) > +Bug 1394935: Assert that the content we're marking dirty is under the restyle root. r?bholley > + > +This would also have catched the bug earlier. Nit: s/catched/caught/

Comment on attachment 8902920 [details] Bug 1394935: Make the restyle root thing more obviously correct and less error prone. https://reviewboard.mozilla.org/r/174642/#review179764 We discussed this at length on IRC and came up with a compromise approach.

(The discussion, for reference, starts at http://logs.glob.uno/?c=mozilla%23servo&s=30+Aug+2017&e=30+Aug+2017#c740370)

Comment on attachment 8902917 [details] Bug 1394935: Add a special case for marking something as dirty from invalidation code. Mozreview is drunk

Comment on attachment 8902917 [details] Bug 1394935: Add a special case for marking something as dirty from invalidation code. Mozreview is still drunk, sorry for the bugspam

Comment on attachment 8902917 [details] Bug 1394935: Add a special case for marking something as dirty from invalidation code. https://reviewboard.mozilla.org/r/174636/#review179878 ::: dom/base/Element.h:478 (Diff revision 4) > ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO | > NODE_DESCENDANTS_NEED_FRAMES; > > + /** > + * Notes that something in the given subtree of `aSubtreeRoot` needs dirtying, > + * and that all the relevant dirty bits have already been setup. I'd say "...have already been propagated up to the given element." ::: dom/base/Element.cpp:4426 (Diff revision 4) > - MOZ_ASSERT(doc->GetServoRestyleRootDirtyBits() & aBit); > + MOZ_ASSERT(doc->GetServoRestyleRootDirtyBits() & aBits); > +} > + > +/* static */ > +void > +Element::NoteDirtySubtreeForServo(Element& aSubtreeRoot) Let's make this non-static, and have it operate on |this| instead of an arg. The comment will need to change as well. Also, though it doesn't matter given the above, Element* in dom. ::: dom/base/Element.cpp:4428 (Diff revision 4) > + MOZ_ASSERT(aSubtreeRoot.IsInComposedDoc()); > + MOZ_ASSERT(aSubtreeRoot.HasServoData()); > + I think we can drop these assertions, since we'll get equivalent checking when we call into NoteDirtyElement, and it'll keep this method smaller. ::: dom/base/Element.cpp:4433 (Diff revision 4) > + MOZ_ASSERT(aSubtreeRoot.IsInComposedDoc()); > + MOZ_ASSERT(aSubtreeRoot.HasServoData()); > + > + nsIDocument* doc = aSubtreeRoot.GetComposedDoc(); > + nsINode* existingRoot = doc->GetServoRestyleRoot(); > + uint32_t bits = existingRoot ? doc->GetServoRestyleRootDirtyBits() : 0; Maybe |existingBits|? ::: dom/base/Element.cpp:4437 (Diff revision 4) > + // NOTE(emilio): This could return true unnecessarily if the root is a > + // scrollbar or children of it, and aSubtreeRoot is the document element, but > + // it'll do the right thing anyway, given PropagateBits will return null. Per IRC, I think it would be easier to reason about to just add a |ContentIsFlattenedTreeDescendantOfForStyle| which would use the style flattened tree, and then there's no ambiguity here and we can drop the comment and the scrollbar handling. ::: layout/style/ServoBindings.cpp:373 (Diff revision 4) > void > Gecko_NoteDirtyElement(RawGeckoElementBorrowed aElement) > { > MOZ_ASSERT(NS_IsMainThread()); > - const_cast<Element*>(aElement)->NoteDirtyForServo(); > + Element::NoteDirtySubtreeForServo(*const_cast<Element*>(aElement)); Hm, I guesss this lets you avoid doing any servo-side changes? I'd really prefer if we only called into this function from stylesheet invalidation, and not from the other callsites. I'd be ok with leaving it like this for now if you bundle a fix into your next coordiland.

Comment on attachment 8902981 [details] Bug 1394935: Assert that the new root is always higher up in the tree than the old root. https://reviewboard.mozilla.org/r/174744/#review180060 ::: dom/base/nsIDocumentInlines.h:70 (Diff revision 2) > MOZ_ASSERT(aDirtyBits); > MOZ_ASSERT((aDirtyBits & ~Element::kAllServoDescendantBits) == 0); > > + MOZ_ASSERT(!mServoRestyleRoot || > + mServoRestyleRoot == aRoot || > + nsContentUtils::ContentIsFlattenedTreeDescendantOf(mServoRestyleRoot, aRoot)); Let's switch this to the ForStyle variant.

Comment on attachment 8902982 [details] Bug 1394935: Add a (commented out for now) assertion about clobbering dirty bits. https://reviewboard.mozilla.org/r/174746/#review180062

Comment on attachment 8902983 [details] Bug 1394935: Assert that the content we're marking dirty is under the restyle root. https://reviewboard.mozilla.org/r/174748/#review180064 ::: dom/base/Element.cpp:4390 (Diff revision 2) > doc->SetServoRestyleRoot(doc, existingBits | aBit); > } > } > > MOZ_ASSERT(aElement == doc->GetServoRestyleRoot() || > + nsContentUtils::ContentIsFlattenedTreeDescendantOf( ForStyle.

Comment on attachment 8902984 [details] Bug 1394935: Assert that we don't call into NoteDirtyElement with extra bits on the restyle root's parent chain. https://reviewboard.mozilla.org/r/174750/#review180066

Comment on attachment 8902985 [details] Bug 1394935: Fix a little typo in NoteDirtyElement. https://reviewboard.mozilla.org/r/174752/#review180068 (I'm fine with bundling comment/typo fixes into other changes if that makes life easier for you)

Comment on attachment 8902986 [details] Bug 1394935: Introduce nsContentUtils::CommonFlattenedTreeAncestorForStyle. https://reviewboard.mozilla.org/r/174754/#review180070

Comment on attachment 8902987 [details] Bug 1394935: Assert that if we find a common ancestor using the dirty bits, it is the actual common flattened tree ancestor. https://reviewboard.mozilla.org/r/174756/#review180072 r=me with that fixed. ::: dom/base/Element.cpp:4393 (Diff revision 2) > + MOZ_ASSERT(commonAncestor == aElement || > + nsContentUtils::GetCommonFlattenedTreeAncestorForStyle(aElement, rootParent)); I think the right-hand side of the || isn't checking what you intend. Shouldn't it be: MOZ_ASSERT(commonAncestor == aElement || commonAncestor == nsContentUtils::...)?

Comment on attachment 8902917 [details] Bug 1394935: Add a special case for marking something as dirty from invalidation code. https://reviewboard.mozilla.org/r/174636/#review180108

Comment on attachment 8903256 [details] Bug 1394935: Return the document as the flattened tree parent of doc-level NAC. https://reviewboard.mozilla.org/r/175032/#review180110

Comment on attachment 8903257 [details] Bug 1394935: Introduce ContentIsFlattenedTreeDescendantOfForStyle. https://reviewboard.mozilla.org/r/175044/#review180112

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/23e8ef26bb40 Return the document as the flattened tree parent of doc-level NAC. r=bholley https://hg.mozilla.org/integration/autoland/rev/d4830b06540f Introduce ContentIsFlattenedTreeDescendantOfForStyle. r=bholley https://hg.mozilla.org/integration/autoland/rev/f8c9e983f36c Introduce nsContentUtils::CommonFlattenedTreeAncestorForStyle. r=bholley https://hg.mozilla.org/integration/autoland/rev/88227ffe3c6e Assert that the new root is always higher up in the tree than the old root. r=bholley https://hg.mozilla.org/integration/autoland/rev/5ededad9baca Add a (commented out for now) assertion about clobbering dirty bits. r=bholley https://hg.mozilla.org/integration/autoland/rev/9fad64b68cbc Assert that the content we're marking dirty is under the restyle root. r=bholley https://hg.mozilla.org/integration/autoland/rev/089cbcabf67a Assert that we don't call into NoteDirtyElement with extra bits on the restyle root's parent chain. r=bholley https://hg.mozilla.org/integration/autoland/rev/3b1074717dac Fix a little typo in NoteDirtyElement. r=bholley https://hg.mozilla.org/integration/autoland/rev/c3f9a3e2ce96 Assert that if we find a common ancestor using the dirty bits, it is the actual common flattened tree ancestor. r=bholley https://hg.mozilla.org/integration/autoland/rev/8daf204f77fb Add a special case for marking something as dirty from invalidation code. r=bholley

Jesse posted a test case for this panic in bug 1389645. I did confirm the test case does not crash on autoland locally. I think we can use the test case for an automation test case for this bug. I did push a try with the test case based on autoland now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f74446fac18129907690ca5204aaca69975baa77

The try looks good.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fe4bf0764f4d A crash test. r=bholley

https://hg.mozilla.org/mozilla-central/rev/23e8ef26bb40 https://hg.mozilla.org/mozilla-central/rev/d4830b06540f https://hg.mozilla.org/mozilla-central/rev/f8c9e983f36c https://hg.mozilla.org/mozilla-central/rev/88227ffe3c6e https://hg.mozilla.org/mozilla-central/rev/5ededad9baca https://hg.mozilla.org/mozilla-central/rev/9fad64b68cbc https://hg.mozilla.org/mozilla-central/rev/089cbcabf67a https://hg.mozilla.org/mozilla-central/rev/3b1074717dac https://hg.mozilla.org/mozilla-central/rev/c3f9a3e2ce96 https://hg.mozilla.org/mozilla-central/rev/8daf204f77fb https://hg.mozilla.org/mozilla-central/rev/fe4bf0764f4d