1406562 - stylo: Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?) in [@ mozilla::ServoRestyleState::ChangesHandledFor]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?), at /src/layout/base/ServoRestyleManager.cpp:151

#0 mozilla::ServoRestyleState::ChangesHandledFor(nsIFrame const&) const /src/layout/base/ServoRestyleManager.cpp:150:3
#1 nsIFrame::UpdateStyleOfOwnedChildFrame(nsIFrame*, nsStyleContext*, mozilla::ServoRestyleState&, mozilla::Maybe<nsStyleContext*> const&) /src/layout/generic/nsFrame.cpp:10317:32
#2 nsBlockFrame::UpdateFirstLetterStyle(mozilla::ServoRestyleState&) /src/layout/generic/nsBlockFrame.cpp:5671:3
#3 nsBlockFrame::UpdatePseudoElementStyles(mozilla::ServoRestyleState&) /src/layout/generic/nsBlockFrame.cpp:7587:5
#4 mozilla::UpdateFramePseudoElementStyles(nsIFrame*, mozilla::ServoRestyleState&) /src/layout/base/ServoRestyleManager.cpp:674:41
#5 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:970:7
#6 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:949:32
#7 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:949:32
#8 mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:1138:28
#9 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4145:41
#10 nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1922:18
#11 mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:307:7
#12 mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:328:5
#13 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:770:5
#14 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:683:35
#15 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /src/layout/base/nsRefreshDriver.cpp:529:20
#16 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#17 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:524:10
#18 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#19 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#20 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#21 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#22 nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#23 XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4694:22
#24 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4858:8
#25 XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4953:21
#26 do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22
#27 main /src/browser/app/nsBrowserApp.cpp:304:16
#28 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#29 _start (firefox+0x41eae4)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 82f07257d93271021a8e84f6cb4ad30580e1bc15
INFO: First bad revision: ac3ccd00426152daa9f7afbba00cc6abd59a1039
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=82f07257d93271021a8e84f6cb4ad30580e1bc15&tochange=ac3ccd00426152daa9f7afbba00cc6abd59a1039

Comment 2

[Bob Clary [:bc] (inactive)]

Bughunter has also seen this on https://store.shopping.yahoo.co.jp/pfudirect/pz-kbkp-kks.html and I've reproduced locally as well.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Looks like ExpectedOwnerForChild should just return first continuation.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1406562 - Return first continuation for parent of first-letter in ExpectedOwnerForChild.

Review commit: https://reviewboard.mozilla.org/r/187572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187572/

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

We may at some point find that parent of ::first-line is also not the first continuation... And at that point, we would need to change the return statement in the previous if-block as well, I guess.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916458 [details]
Bug 1406562 - Return first continuation for parent of first-letter in ExpectedOwnerForChild.

https://reviewboard.mozilla.org/r/187572/#review192628

r=me, thanks for looking into it. I didn't thought that a first-letter could be in a non-first continuation, though I guess it makes sense in this case.

::: layout/base/crashtests/1406562.html:3
(Diff revision 1)
> +<style>
> +.class5 { columns: 0px; }
> +li::first-letter {}

Please add a dummy property here (`color: red`? whatever) just in case we start optimizing out empty rules sometime in the future.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Thanks for looking into this xidorn, hadn't managed to get to this one yet.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8916458 [details]
Bug 1406562 - Return first continuation for parent of first-letter in ExpectedOwnerForChild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187572/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3fac14728144
Return first continuation for parent of first-letter in ExpectedOwnerForChild. r=emilio

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3fac14728144

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

I'm not sure whether it's worth uplifting. It is just an assertion fix, so shouldn't affect any real code.

If it affects fuzzer, probably we should uplift?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8916458 [details]
Bug 1406562 - Return first continuation for parent of first-letter in ExpectedOwnerForChild.

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: no impact
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: landed yesterday
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: only changes debug / assertion code
[String changes made/needed]: n/a

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Selena, could you please help determine if this is a must fix for 57? Thanks!

Comment 14

[Selena Deckelmann :selenamarie :selena]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Hi Selena, could you please help determine if this is a must fix for 57?

It's debug only code, and helps with fuzzing our new components, so the risk is very low. Let's take this.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916458 [details]
Bug 1406562 - Return first continuation for parent of first-letter in ExpectedOwnerForChild.

Low risk, Beta57+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/76296f08f283

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: landed yesterday
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Xidorn Quan's assessment on manual testing needs and the fact that this fix has automated coverage.