Closed Bug 1402442 Opened 3 years ago Closed 2 years ago

stylo: panicked at 'We're removing a pseudo, so we should reframe!'

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: truber, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file testcase.html
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
Flags: in-testsuite?
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Priority: -- → P2
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.
See Also: → 1251799, 1359656
Attached file testcase
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.
Attachment #8912190 - Flags: review?(mats) → review+
Can this land?
Flags: needinfo?(emilio)
yes, let me fix the comments and do a try run just in case, this just fell off my radar.
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/faa69ac1c14b
Properly remove display: contents pseudo-frames. r=mats
https://hg.mozilla.org/mozilla-central/rev/faa69ac1c14b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please request Beta approval on this when you get a chance. And nice touch landing the tests as wpt :)
Flags: needinfo?(emilio)
Flags: in-testsuite?
Flags: in-testsuite+
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.
Flags: needinfo?(emilio)
Duplicate of this bug: 1402036
Duplicate of this bug: 1400763
Given the other examples also fixed by the patch here (see dup'd bugs), I wonder if we should reconsider uplifting this?
Flags: needinfo?(ryanvm)
Flags: needinfo?(emilio)
Flags: needinfo?(ryanvm) → needinfo?(rkothari)
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
Flags: needinfo?(emilio)
Attachment #8912190 - Flags: approval-mozilla-beta?
Comment on attachment 8912190 [details]
Bug 1402442: Properly remove display: contents pseudo-frames.

Must fix based on user impact, Beta57+
Flags: needinfo?(rkothari)
Attachment #8912190 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1400763
You need to log in before you can comment on or make changes to this bug.