Testcase found while fuzzing mozilla-central rev a80d568a417e. thread '<unnamed>' panicked at 'assertion failed: !element.has_dirty_descendants()', /builds/worker/workspace/build/src/servo/components/style/traversal.rs:594:8 stack backtrace: 0: 0x7f0f089f62f3 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hfc7985b08e763a82 1: 0x7f0f089f0b34 - std::sys_common::backtrace::_print::h16a1db02a59ead63 2: 0x7f0f08a03a13 - std::panicking::default_hook::{{closure}}::h48ecee46f2eefc30 3: 0x7f0f08a03782 - std::panicking::default_hook::hb4c92ae8d005ca44 4: 0x7f0f08a03f16 - std::panicking::rust_panic_with_hook::h25d461655d60b1a5 5: 0x7f0f081385a5 - std::panicking::begin_panic::h728d8100f72e79de 6: 0x7f0f07f53803 - style::traversal::recalc_style_at::hb71be495a7b1f6e4 7: 0x7f0f07f0a29a - <style::gecko::traversal::RecalcStyleOnly<'recalc> as style::traversal::DomTraversal<style::gecko::wrapper::GeckoElement<'le>>>::process_preorder::h853c1e540427551f 8: 0x7f0f080b92de - style::driver::traverse_dom::hf30006a9d425a49f 9: 0x7f0f07f9d394 - geckoservo::glue::traverse_subtree::h26f5927f66e48f13 10: 0x7f0f07f9d6ed - Servo_TraverseSubtree 11: 0x7f0f06544694 - _ZN7mozilla13ServoStyleSet13StyleDocumentENS_19ServoTraversalFlagsE 12: 0x7f0f06627b20 - _ZN7mozilla19ServoRestyleManager24DoProcessPendingRestylesENS_19ServoTraversalFlagsE 13: 0x7f0f06628d9a - _ZN7mozilla9PresShell27DoFlushPendingNotificationsENS_14ChangesToFlushE 14: 0x7f0f065f87d1 - _ZN15nsRefreshDriver4TickElN7mozilla9TimeStampE 15: 0x7f0f065f98ef - _ZN7mozilla18RefreshDriverTimer18TickRefreshDriversElNS_9TimeStampER8nsTArrayI6RefPtrI15nsRefreshDriverEE 16: 0x7f0f065f99c8 - _ZN7mozilla18RefreshDriverTimer4TickElNS_9TimeStampE 17: 0x7f0f065f9b82 - _ZN7mozilla23VsyncRefreshDriverTimer26RefreshDriverVsyncObserver17TickRefreshDriverENS_9TimeStampE 18: 0x7f0f065f9ed7 - _ZN7mozilla23VsyncRefreshDriverTimer26RefreshDriverVsyncObserver26ParentProcessVsyncNotifier3RunEv 19: 0x7f0f048251b3 - _ZN8nsThread16ProcessNextEventEbPb.part.267 20: 0x7f0f04825f9c - _Z19NS_ProcessNextEventP9nsIThreadb 21: 0x7f0f04be509d - _ZN7mozilla3ipc11MessagePump3RunEPN4base11MessagePump8DelegateE 22: 0x7f0f04bba1a4 - _ZN11MessageLoop11RunInternalEv 23: 0x7f0f04bba1d0 - _ZN11MessageLoop3RunEv 24: 0x7f0f063ef422 - _ZN14nsBaseAppShell3RunEv 25: 0x7f0f07373e30 - _ZN12nsAppStartup3RunEv 26: 0x7f0f073fb008 - _ZN7XREMain11XRE_mainRunEv 27: 0x7f0f073fb805 - _ZN7XREMain8XRE_mainEiPPcRKN7mozilla15BootstrapConfigE 28: 0x7f0f073fbaea - _Z8XRE_mainiPPcRKN7mozilla15BootstrapConfigE 29: 0x4073ac - _ZL7do_mainiPPcS0_ 30: 0x406c59 - main 31: 0x7f0f162c982f - __libc_start_main 32: 0x406eb8 - <unknown> Redirecting call to abort() to mozalloc_abort

Appending an element into a hidden <frame> element... sounds like something we may not restyle into...

Force a layout to make the crash more reliable.

Given this testcase, I suspect that we are probably skipping the traversal when the new element is inserted.

So... I totally have no idea what's the whole idea of the piece of code in recalc_style_at is: https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/servo/components/style/traversal.rs#555-596 At the beginning of the piece of code, we say, if the element "has_dirty_descendants", then we need to "traverse_children". Then we say that, if the element "is_display_none", we don't need to "traverse_children". Finally, we check that, if the element "is_display_none", "has_dirty_descendants" should have been reset. But apparently it would never happen, because we clearly stopped traversing when we find the element is display none. Maybe descendant of display:none element should be restyled elsewhere so that any element should never have both "is_display_none" and "has_dirty_descendants"? In that case, this should probably be asserted in some earlier place, I guess.

That assertion was added in bug 1383332... Probably emilio knows something about it.

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #7) > That assertion was added in bug 1383332... Probably emilio knows something > about it. The check in http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/dom/base/Element.cpp#4507 is what is supposed to prevent this assert from firing...

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #6) > So... I totally have no idea what's the whole idea of the piece of code in > recalc_style_at is: > https://searchfox.org/mozilla-central/rev/ > 1285ae3e810e2dbf2652c49ee539e3199fcfe820/servo/components/style/traversal. > rs#555-596 And what makes this code make a bit more sense is that we clear the data in display: none subtrees, but as of right now only when we restyle the element (not the case here): https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/servo/components/style/traversal.rs#512

So the issue is that that element has display: none, but got somehow an nsSubDocumentFrame, so the GetPrimaryFrame check we do in NoteDirtyElement makes us think that it doesn't have display: none

And the reason for this is that nsHTMLFramesetFrame::Init does its own YOLO frame construction...

Comment on attachment 8921792 [details] Bug 1411478: Deindent some code in nsHTMLFrameSetFrame::Init. https://reviewboard.mozilla.org/r/192822/#review198040

Comment on attachment 8921793 [details] Bug 1411478: Use GetNextSibling instead of GetChildAt in nsHTMLFrameSetFrame::Init. https://reviewboard.mozilla.org/r/192824/#review198042

Comment on attachment 8921796 [details] Bug 1411478: Crashtest. https://reviewboard.mozilla.org/r/192828/#review198044

Comment on attachment 8921794 [details] Bug 1411478: remove primary frame check which is incorrect in <frame> child of a <frameset>. https://reviewboard.mozilla.org/r/192826/#review198046 I don't think I'm qualified to review this patch, maybe bz should have a look. What I'm thinking is that we probably shouldn't remove the primary frame check here, but instead, we should add an additional check like that for `<area>`, because I don't think we are going to display any children of `<frame>` even if it isn't `display: none` anway. If unconditionally skipping a `<frame>` child is more problematic, we can probably check `parent->IsHTMLElement(nsGkAtoms::frame) && Servo_Element_IsDisplayNone(parent)`, which shouldn't affect the FFI call a lot. Finally, if we really want to remove the primary frame check, we should be able to remove the special case for `<area>` below at the same time.

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #19) > What I'm thinking is that we probably shouldn't remove the primary frame > check here, but instead, we should add an additional check like that for > `<area>`, because I don't think we are going to display any children of > `<frame>` even if it isn't `display: none` anway. We can't just do that, because <frame> elements can render fallback content when not in a <frameset>. So we'd need to do that plus checking that the frameset has a primary frame. > If unconditionally skipping a `<frame>` child is more problematic, we can > probably check `parent->IsHTMLElement(nsGkAtoms::frame) && > Servo_Element_IsDisplayNone(parent)`, which shouldn't affect the FFI call a > lot. Honestly, I'm not that sure avoiding the FFI call here is hot enough to matter. > Finally, if we really want to remove the primary frame check, we should be > able to remove the special case for `<area>` below at the same time. I think that's not right, because <area>'s computed display value can be non-none, we just don't honor it.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #20) > > If unconditionally skipping a `<frame>` child is more problematic, we can > > probably check `parent->IsHTMLElement(nsGkAtoms::frame) && > > Servo_Element_IsDisplayNone(parent)`, which shouldn't affect the FFI call a > > lot. > > Honestly, I'm not that sure avoiding the FFI call here is hot enough to > matter. Hmmm... maybe not a big deal? I'm not sure how hot is this path. > > Finally, if we really want to remove the primary frame check, we should be > > able to remove the special case for `<area>` below at the same time. > > I think that's not right, because <area>'s computed display value can be > non-none, we just don't honor it. It cannot, unless something completely messes up. https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/layout/style/res/html.css#725

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #21) > It cannot, unless something completely messes up. > https://searchfox.org/mozilla-central/rev/ > 1285ae3e810e2dbf2652c49ee539e3199fcfe820/layout/style/res/html.css#725 huh, I had grepped for "none !important" and "none!important", heh. Fair enough, I'll remove that check.

Comment on attachment 8921794 [details] Bug 1411478: remove primary frame check which is incorrect in <frame> child of a <frameset>. https://reviewboard.mozilla.org/r/192826/#review198168 r=me with some more comments and the <area> bit dropped. ::: dom/base/Element.cpp:4508 (Diff revision 1) > > // If the parent is styled but is display:none, we're done. > // > - // We check for a frame to reduce the cases where we need the FFI call. > - if (!parent->GetPrimaryFrame() && Servo_Element_IsDisplayNone(parent)) { > + // We can't check for a frame here, since <frame> elements inside <frameset> > + // still need to generate a frame, even if they're display: none. :( > + if (Servo_Element_IsDisplayNone(parent)) { Please document why we can't have an IsLeaf() check on the frame here, and which general invariants this is relying on and trying to preserve.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s d9eb6be031b0 -d 24906afd73a2: rebasing 429631:d9eb6be031b0 "Bug 1411478: Deindent some code in nsHTMLFrameSetFrame::Init. r=xidorn" rebasing 429632:259f45091da3 "Bug 1411478: Use GetNextSibling instead of GetChildAt in nsHTMLFrameSetFrame::Init. r=xidorn" rebasing 429633:8c261a67bc15 "Bug 1411478: remove primary frame check which is incorrect in <frame> child of a <frameset>. r=bz" rebasing 429634:fd418ebb6785 "Bug 1411478: Crashtest. r=xidorn" (tip) merging layout/style/crashtests/crashtests.list warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/91d58af147aa Deindent some code in nsHTMLFrameSetFrame::Init. r=xidorn https://hg.mozilla.org/integration/autoland/rev/ec41d5ba5cb1 Use GetNextSibling instead of GetChildAt in nsHTMLFrameSetFrame::Init. r=xidorn https://hg.mozilla.org/integration/autoland/rev/a3812e567015 remove primary frame check which is incorrect in <frame> child of a <frameset>. r=bz https://hg.mozilla.org/integration/autoland/rev/6c917c7b2c03 Crashtest. r=xidorn

Emilio, should we uplift this fix to Beta 57?

It's a debug assertion, so probably not...

Yeah, the likeliness that this matters is minimal, though it may be a correctness issue. I think the effort of uplifting (even though it's just one minimal patch, https://hg.mozilla.org/integration/autoland/rev/a3812e567015), aren't even worth it.

https://hg.mozilla.org/mozilla-central/rev/91d58af147aa https://hg.mozilla.org/mozilla-central/rev/ec41d5ba5cb1 https://hg.mozilla.org/mozilla-central/rev/a3812e567015 https://hg.mozilla.org/mozilla-central/rev/6c917c7b2c03

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #35) > It's a debug assertion, so probably not... status-firefox57=wontfix