1323693 - stylo: several tests fatally assert with "content->AsElement()->HasServoData()" in nsCSSFrameConstructor::AddFCItemsForAnonymousContent

Status: RESOLVED WORKSFORME

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

Description

[Cameron McCormack (:heycam)]

#0  0x00007fffe6f5bc27 in nsCSSFrameConstructor::AddFCItemsForAnonymousContent (this=0x7fffb0ec0ee0, aState=..., aFrame=0x7fffba03a888, aAnonymousItems=..., aItemsToConstruct=..., aExtraFlags=0)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:10707
#1  0x00007fffe6f56ce5 in nsCSSFrameConstructor::ProcessChildren (this=0x7fffb0ec0ee0, aState=..., aContent=0x7fffbe831a00, aStyleContext=0x7fffba03a6e8, aFrame=0x7fffba03a888, aCanHaveGeneratedContent=true, aFrameItems=..., 
    aAllowBlockStyles=false, aPendingBinding=0x0, aPossiblyLeafFrame=0x7fffba03a888) at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:10794
#2  0x00007fffe6f602d8 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x7fffb0ec0ee0, aItem=..., aState=..., aParentFrame=0x7fffba03a810, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:4032
#3  0x00007fffe6f63fcc in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x7fffb0ec0ee0, aState=..., aIter=..., aParentFrame=0x7fffba03a810, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:6202
#4  0x00007fffe6fb2ed5 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x7fffb0ec0ee0, aState=..., aItems=..., aParentFrame=0x7fffba03a810, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:10651
#5  0x00007fffe6f60249 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x7fffb0ec0ee0, aItem=..., aState=..., aParentFrame=0x7fffba03a510, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:4028
#6  0x00007fffe6f63fcc in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x7fffb0ec0ee0, aState=..., aIter=..., aParentFrame=0x7fffba03a510, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:6202
#7  0x00007fffe6fb2ed5 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x7fffb0ec0ee0, aState=..., aItems=..., aParentFrame=0x7fffba03a510, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:10651
#8  0x00007fffe6f571a2 in nsCSSFrameConstructor::ProcessChildren (this=0x7fffb0ec0ee0, aState=..., aContent=0x7fffb6dcd000, aStyleContext=0x7fffba03a140, aFrame=0x7fffba03a510, aCanHaveGeneratedContent=true, aFrameItems=..., 
    aAllowBlockStyles=false, aPendingBinding=0x0, aPossiblyLeafFrame=0x7fffba03a510) at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:10864
#9  0x00007fffe6f602d8 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x7fffb0ec0ee0, aItem=..., aState=..., aParentFrame=0x7fffba03a268, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:4032
#10 0x00007fffe6f63fcc in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x7fffb0ec0ee0, aState=..., aIter=..., aParentFrame=0x7fffba03a268, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:6202
#11 0x00007fffe6fb2ed5 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x7fffb0ec0ee0, aState=..., aItems=..., aParentFrame=0x7fffba03a268, aFrameItems=...)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:10651
#12 0x00007fffe6f6a741 in nsCSSFrameConstructor::ContentAppended (this=0x7fffb0ec0ee0, aContainer=0x7fffb3715d80, aFirstNewContent=0x7fffb7157820, aAllowLazyConstruction=false)
    at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:7545
#13 0x00007fffe6f67cfc in nsCSSFrameConstructor::CreateNeededFrames (this=0x7fffb0ec0ee0, aContent=0x7fffb3715d80) at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:7171
#14 0x00007fffe6f67d5b in nsCSSFrameConstructor::CreateNeededFrames (this=0x7fffb0ec0ee0, aContent=0x7fffb6236940) at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:7178
#15 0x00007fffe6f6ab30 in nsCSSFrameConstructor::CreateNeededFrames (this=0x7fffb0ec0ee0) at /z/stylo/hg-incubator/layout/base/nsCSSFrameConstructor.cpp:7193
#16 0x00007fffe6f26ecf in mozilla::ServoRestyleManager::ProcessPendingRestyles (this=0x7fffbd79e5c0) at /z/stylo/hg-incubator/layout/base/ServoRestyleManager.cpp:309
#17 0x00007fffe6f2ed3d in mozilla::RestyleManagerHandle::Ptr::ProcessPendingRestyles (this=0x7fffffffb230) at /z/stylo/hg-incubator/obj/dist/include/mozilla/RestyleManagerHandleInlines.h:75
#18 0x00007fffe6ef92aa in mozilla::PresShell::FlushPendingNotifications (this=0x7fffb6514800, aFlush=...) at /z/stylo/hg-incubator/layout/base/PresShell.cpp:4115
#19 0x00007fffe6ec2e64 in nsRefreshDriver::Tick (this=0x7fffb3729400, aNowEpoch=1481797076896097, aNowTime=...) at /z/stylo/hg-incubator/layout/base/nsRefreshDriver.cpp:1842
#20 0x00007fffe6eca278 in mozilla::RefreshDriverTimer::TickDriver (driver=0x7fffb3729400, jsnow=1481797076896097, now=...) at /z/stylo/hg-incubator/layout/base/nsRefreshDriver.cpp:326
#21 0x00007fffe6eca073 in mozilla::RefreshDriverTimer::TickRefreshDrivers (this=0x7fffb9fe3220, aJsNow=1481797076896097, aNow=..., aDrivers=...) at /z/stylo/hg-incubator/layout/base/nsRefreshDriver.cpp:295
#22 0x00007fffe6ec9efa in mozilla::RefreshDriverTimer::Tick (this=0x7fffb9fe3220, jsnow=1481797076896097, now=...) at /z/stylo/hg-incubator/layout/base/nsRefreshDriver.cpp:316
#23 0x00007fffe6ecc3fd in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers (this=0x7fffb9fe3220, aTimeStamp=...) at /z/stylo/hg-incubator/layout/base/nsRefreshDriver.cpp:669
#24 0x00007fffe6ecb8cc in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver (this=0x7fffb9ee2900, aVsyncTimestamp=...) at /z/stylo/hg-incubator/layout/base/nsRefreshDriver.cpp:589
#25 0x00007fffe6ecc07e in mozilla::detail::RunnableMethodArguments<mozilla::TimeStamp>::applyImpl<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), StoreCopyPassByConstLRef<mozilla::TimeStamp>, 0ul> (o=0x7fffb9ee2900, m=
    (void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver * const, mozilla::TimeStamp)) 0x7fffe6ecb760 <mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp)>, args=...) at /z/stylo/hg-incubator/obj/dist/include/nsThreadUtils.h:791
#26 0x00007fffe6ecbfb9 in mozilla::detail::RunnableMethodArguments<mozilla::TimeStamp>::apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)> (this=0x7fffdc6224b0, o=0x7fffb9ee2900, m=
    (void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver * const, mozilla::TimeStamp)) 0x7fffe6ecb760 <mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp)>) at /z/stylo/hg-incubator/obj/dist/include/nsThreadUtils.h:797
#27 0x00007fffe6ecbe02 in mozilla::detail::RunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, false, mozilla::TimeStamp>::Run (this=0x7fffdc622470)
    at /z/stylo/hg-incubator/obj/dist/include/nsThreadUtils.h:826
#28 0x00007fffe2ad0ca8 in nsThread::ProcessNextEvent (this=0x7fffdfe3d300, aMayWait=false, aResult=0x7fffffffbd4e) at /z/stylo/hg-incubator/xpcom/threads/nsThread.cpp:1213
#29 0x00007fffe2b505fc in NS_ProcessNextEvent (aThread=0x7fffdfe3d300, aMayWait=false) at /z/stylo/hg-incubator/xpcom/glue/nsThreadUtils.cpp:381
#30 0x00007fffe33cb0e9 in mozilla::ipc::MessagePump::Run (this=0x7fffdfe98a80, aDelegate=0x7ffff6bb2410) at /z/stylo/hg-incubator/ipc/glue/MessagePump.cpp:96
#31 0x00007fffe33296c5 in MessageLoop::RunInternal (this=0x7ffff6bb2410) at /z/stylo/hg-incubator/ipc/chromium/src/base/message_loop.cc:232
#32 0x00007fffe3329645 in MessageLoop::RunHandler (this=0x7ffff6bb2410) at /z/stylo/hg-incubator/ipc/chromium/src/base/message_loop.cc:225
#33 0x00007fffe332961d in MessageLoop::Run (this=0x7ffff6bb2410) at /z/stylo/hg-incubator/ipc/chromium/src/base/message_loop.cc:205
#34 0x00007fffe6ad4003 in nsBaseAppShell::Run (this=0x7fffd35e7970) at /z/stylo/hg-incubator/widget/nsBaseAppShell.cpp:156
#35 0x00007fffe7cab192 in nsAppStartup::Run (this=0x7fffd35ef880) at /z/stylo/hg-incubator/toolkit/components/startup/nsAppStartup.cpp:283
#36 0x00007fffe7da48ac in XREMain::XRE_mainRun (this=0x7fffffffc728) at /z/stylo/hg-incubator/toolkit/xre/nsAppRunner.cpp:4485
#37 0x00007fffe7da5396 in XREMain::XRE_main (this=0x7fffffffc728, argc=4, argv=0x7fffffffdc08, aAppData=0x7fffffffc9e8) at /z/stylo/hg-incubator/toolkit/xre/nsAppRunner.cpp:4618
#38 0x00007fffe7da5b6f in XRE_main (argc=4, argv=0x7fffffffdc08, aAppData=0x7fffffffc9e8, aFlags=0) at /z/stylo/hg-incubator/toolkit/xre/nsAppRunner.cpp:4709
#39 0x000000000040633f in do_main (argc=4, argv=0x7fffffffdc08, envp=0x7fffffffdc30, xreDirectory=0x7ffff6b5eb40) at /z/stylo/hg-incubator/browser/app/nsBrowserApp.cpp:328
#40 0x0000000000405a62 in main (argc=4, argv=0x7fffffffdc08, envp=0x7fffffffdc30) at /z/stylo/hg-incubator/browser/app/nsBrowserApp.cpp:461

Comment 1

[Cameron McCormack (:heycam)]

layout/forms/crashtests/949891.xhtml
layout/forms/crashtests/1140216.html

Comment 2

[Cameron McCormack (:heycam)]

layout/style/crashtests/1233135-1.html
layout/style/crashtests/1233135-2.html

Comment 3

[Bobby Holley (:bholley)]

Incremental restyle.

Comment 4

[Bobby Holley (:bholley)]

So this one is fun. I looked into the first crash, guessing the second is similar.

The basic issue is that we have an <input type="number">, which creates a NAC subtree of the scheme described at [1]. These are all "pseudo-element-implementing-nac", and so the style contexts are created explicitly by CreateAnonymousContent, and returned in the nsTArray<ContentInfo>. The frame constructor sees these explicit style contexts, determines that subtree to be "manually managed", and doesn't kick over to servo [2]. StyleChildrenIterator plays along to prevent us from visiting that subtree during subsequent restyles [3].

Unfortunately, part of the NAC subtree described in [1] is a text <input> field, which creates its _own_ NAC, which is non-pseudo-element-implementing. So we expect Servo to manage this one as part of the flattened tree, except we're already in a manually-managed pseudo-element-implementing subtree that servo doesn't know about, so we don't have any parent style on the servo side. Chaos ensues.

My gut feeling is that we're probably going to need to teach the servo traversal about pseudo-elementing NAC, and delegate more work to that side of the fence. I haven't worked out the details yet, but will ponder this one for a bit.

[1] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/layout/forms/nsNumberControlFrame.cpp#367
[2] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/layout/base/nsCSSFrameConstructor.cpp#4269
[3] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/dom/base/ChildIterator.cpp#593

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> Unfortunately, part of the NAC subtree described in [1] is a text <input>
> field, which creates its _own_ NAC, which is
> non-pseudo-element-implementing. So we expect Servo to manage this one as
> part of the flattened tree, except we're already in a manually-managed
> pseudo-element-implementing subtree that servo doesn't know about, so we
> don't have any parent style on the servo side. Chaos ensues.
> 
> My gut feeling is that we're probably going to need to teach the servo
> traversal about pseudo-elementing NAC, and delegate more work to that side
> of the fence. I haven't worked out the details yet, but will ponder this one
> for a bit.

Seems like we can just move the check into Servo to avoid calling recalc_style_at in these nodes, and iterate the children of the NAC node regardless of whether it is pseudo-element-implementing?

Ideally it won't make the common case slower. How does dynamic restyling work for these elements? do we restyle them at all? Because right now in Servo we're skipping them completely as far as I know.

Also, if we don't need to care about dynamic restyling of these in the common path, it might be worth to split the frame constructor traversal from the normal RestyleManager path to avoid these checks (but that's probably an optimization that should be measured given the impact in code size may not be trivial).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> How does dynamic restyling work for these elements? 

Which "these elements"?  Do you mean the pseudo-element-implementing ones, or their descendants?

The pseudo-element-implementing ones obviously can get restyled per spec and do get restyled in Gecko.

Note that ::placeholder on number inputs targets a child of the anonymous <input type="text">, but maybe that one is also treated as pseudo-element-implementing in practice (unlike the main <div> inside there)?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6)
> > How does dynamic restyling work for these elements? 
> 
> Which "these elements"?  Do you mean the pseudo-element-implementing ones,
> or their descendants?
> 
> The pseudo-element-implementing ones obviously can get restyled per spec and
> do get restyled in Gecko.

I meant the ones we call ResolvePseudoElementStyle directly in [1]. Of course I looked through the callers of StyleSetHandle::ResolvePseudoElementStyle and found nothing in the restyle manager (which is what confused me), but that was just because RestyleManager calls the nsStyleSet method directly[2].

As far as I know we were not restyling them at all for dynamic changes right now, right Bobby? So yeah, I think we need to teach Servo about these.

[1]: http://searchfox.org/mozilla-central/source/layout/forms/nsNumberControlFrame.cpp#343
[2]: http://searchfox.org/mozilla-central/source/layout/base/RestyleManager.cpp#2860

Comment 8

[Bobby Holley (:bholley)]

I filed bug 1331047 for the architectural work here, and will write up my plan in that bug.

Comment 9

[Bobby Holley (:bholley)]

Re-enabled the crashtests associated with this bug: 

https://hg.mozilla.org/integration/autoland/rev/91727c7a6f5bb08ea5297e6b82690ece6ca2cd38