The attached testcase causes a panic in m-c rev 20170922-14db7c0bcf9a thread '<unnamed>' panicked at 'We're removing a pseudo, so we should reframe!', /builds/worker/workspace/build/src/servo/components/style/traversal.rs:257 #0: mozalloc_abort, at memory/mozalloc/mozalloc_abort.cpp:33 #1: abort, at memory/mozalloc/mozalloc_abort.cpp:80 #2: panic_abort::__rust_start_panic, at src/libpanic_abort/lib.rs:61 #3: std::panicking::rust_panic, at src/libstd/panicking.rs:580 #4: std::panicking::rust_panic_with_hook, at src/libstd/panicking.rs:565 #5: std::panicking::begin_panic<&str>, at src/libstd/panicking.rs:511 #6: style::traversal::DomTraversal::element_needs_traversal<style::gecko::traversal::RecalcStyleOnly,style::gecko::wrapper::GeckoElement>, at obj-firefox/toolkit/library/gtest/rust/<panic macros>:3 #7: style::traversal::note_children<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure>, at servo/components/style/traversal.rs:876 #8: style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure>, at servo/components/style/traversal.rs:562 #9: style::gecko::traversal::{{impl}}::process_preorder<closure>, at servo/components/style/gecko/traversal.rs:37 #10: style::driver::traverse_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly>, at servo/components/style/driver.rs:71 #11: geckoservo::glue::traverse_subtree, at servo/ports/geckolib/glue.rs:262 #12: geckoservo::glue::Servo_TraverseSubtree, at servo/ports/geckolib/glue.rs:300 #13: mozilla::ServoStyleSet::StyleDocument, at layout/style/ServoStyleSet.cpp:977 #14: mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1106 #15: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4170 #16: nsRefreshDriver::Tick, at layout/base/nsRefreshDriver.cpp:1921 #17: mozilla::RefreshDriverTimer::TickRefreshDrivers, at layout/base/nsRefreshDriver.cpp:307 #18: mozilla::RefreshDriverTimer::Tick, at layout/base/nsRefreshDriver.cpp:329 #19: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver, at layout/base/nsRefreshDriver.cpp:770 #20: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync, at layout/base/nsRefreshDriver.cpp:584 #21: mozilla::layout::VsyncChild::RecvNotify, at layout/ipc/VsyncChild.cpp:67 #22: mozilla::layout::PVsyncChild::OnMessageReceived, at 9a6479c28b0f57ad4e6d1aec1c5258678abc48225caeaa2a25f8a6893b83595687422ba915c00ef9e645c78dda14478aa8e2f891ae54d4764ccdba97f65bc47d/ipc/ipdl/PVsyncChild.cpp:155 #23: mozilla::ipc::MessageChannel::DispatchAsyncMessage, at ipc/glue/MessageChannel.cpp:2115 #24: mozilla::ipc::MessageChannel::DispatchMessage, at ipc/glue/MessageChannel.cpp:2045 #25: mozilla::ipc::MessageChannel::RunMessage, at ipc/glue/MessageChannel.cpp:1891 #26: mozilla::ipc::MessageChannel::MessageTask::Run, at ipc/glue/MessageChannel.cpp:1924 #27: nsThread::ProcessNextEvent, at xpcom/threads/nsThread.cpp:1039 #28: NS_ProcessNextEvent, at xpcom/threads/nsThreadUtils.cpp:521 #29: mozilla::ipc::MessagePump::Run, at ipc/glue/MessagePump.cpp:125 #30: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326 #31: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319 #32: nsBaseAppShell::Run, at widget/nsBaseAppShell.cpp:158 #33: XRE_RunAppShell, at toolkit/xre/nsEmbedFunctions.cpp:880 #34: mozilla::ipc::MessagePumpForChildProcess::Run, at ipc/glue/MessagePump.cpp:269 #35: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326 #36: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319 #37: XRE_InitChildProcess, at toolkit/xre/nsEmbedFunctions.cpp:705 #38: content_process_main, at ipc/contentproc/plugin-container.cpp:63 #39: main, at browser/app/nsBrowserApp.cpp:285 #40: libc-2.26.so+0x20f6a #41: MOZ_ReportAssertionFailure, at mfbt/Assertions.h:165

This is a funny test-case, and has literally everything: fieldset, display: contents, pseudo-elements... Stylo arrives to the wrong answer regarding pseudo-elements, and that assertion catches it. However the underlying reason is how we store pseudo-elements of display: contents elements (we store them on the insertion point, and <fieldset> has two insertion points). Gecko also does the same thing, fwiw. See also bug 1359656, and bug 1251799, by extension. I don't have a fix yet, but I doubt this is really a P2 per the priority of the other bugs.

So the assert fires because we're failing to remove a pseudo-element. I have a patch for this running through try, but here's a clearer test-case of how this fails. The problematic line is: http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/layout/base/nsCSSFrameConstructor.cpp#8691 Working on a fix for that and bug 1359656 / bug 1251799.

Comment on attachment 8912190 [details] Bug 1402442: Properly remove display: contents pseudo-frames. https://reviewboard.mozilla.org/r/183562/#review190178 ::: layout/base/nsCSSFrameConstructor.cpp:1700 (Diff revision 2) > > nsFrameManager::NotifyDestroyingFrame(aFrame); > } > > +static bool > +HasGeneratedContent(const nsIContent& aChild) nit: using a reference type rather than a pointer seems like an odd choice in this context. Layout code uses nsIContent*, nsIFrame* etc in general, also when a non-null argument is expected. Whether that's a good choice or not can be debated, but I'd prefer we stick with one style until there is a policy decision to move to something else, like using reference types, or NotNull<T>. Also, the use of "GetBeforeFrame(&aChild)" later makes me think "oh, we're getting a result in an out-param" for a brief moment before realizing it's just converting it back to a pointer, so I think using a mix of T& and T* is probably not a good idea.

Can this land?

yes, let me fix the comments and do a try run just in case, this just fell off my radar.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/faa69ac1c14b Properly remove display: contents pseudo-frames. r=mats

Please request Beta approval on this when you get a chance. And nice touch landing the tests as wpt :)

I think that given that this is a pre-existing Gecko bug we've shipped with since 37 IIRC, unless there's a bigger reason for it, this can just ride the trains.

Given the other examples also fixed by the patch here (see dup'd bugs), I wonder if we should reconsider uplifting this?

FWIW, given the dupes, yes, I think we should reconsider. Also, I've confirmed that the patch grafts cleanly to Beta as-landed.

Comment on attachment 8912190 [details] Bug 1402442: Properly remove display: contents pseudo-frames. Approval Request Comment [Feature/Bug causing the regression]: bug 907396, I guess [User impact if declined]: Incorrect rendering of pages using display: contents, and security impact (see dupes). [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: not risky [Why is the change risky/not risky?]: has been baked in nightly for a while now, and patch is simple. [String changes made/needed]: none

Comment on attachment 8912190 [details] Bug 1402442: Properly remove display: contents pseudo-frames. Must fix based on user impact, Beta57+