Jason Kratzer [:jkratzer] Reporter
	Description • 7 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 3 • 7 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 4 • 7 years ago

Force a layout to make the crash more reliable.

	Xidorn Quan [:xidorn] UTC+11
	Comment 5 • 7 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 6 • 7 years ago

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 7 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 8 • 7 years ago

(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...

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 9 • 7 years ago

(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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 10 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 11 • 7 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8921792 [details]
Bug 1411478: Deindent some code in nsHTMLFrameSetFrame::Init.

https://reviewboard.mozilla.org/r/192822/#review198040

	Xidorn Quan [:xidorn] UTC+11
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8921793 [details]
Bug 1411478: Use GetNextSibling instead of GetChildAt in nsHTMLFrameSetFrame::Init.

https://reviewboard.mozilla.org/r/192824/#review198042

	Xidorn Quan [:xidorn] UTC+11
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8921796 [details]
Bug 1411478: Crashtest.

https://reviewboard.mozilla.org/r/192828/#review198044

	Xidorn Quan [:xidorn] UTC+11
	Comment 19 • 7 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 20 • 7 years ago

(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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 21 • 7 years ago

(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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 22 • 7 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 7 years ago
mozreview-review

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.

	Mozilla Autoland
	Comment 28 • 7 years ago

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)

	Pulsebot
	Comment 33 • 7 years ago

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

	Chris Peterson [:cpeterson]
	Comment 34 • 7 years ago

Emilio, should we uplift this fix to Beta 57?

	Xidorn Quan [:xidorn] UTC+11
	Comment 35 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 36 • 7 years ago

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.

	Attila Craciun [:arny]
	Comment 37 • 7 years ago
bugherder

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

	Chris Peterson [:cpeterson]
	Comment 38 • 7 years ago

(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