Closed Bug 1406562 Opened 2 years ago Closed 2 years ago

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

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: tsmith, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file 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)
Flags: in-testsuite?
INFO: Last good revision: 82f07257d93271021a8e84f6cb4ad30580e1bc15
INFO: First bad revision: ac3ccd00426152daa9f7afbba00cc6abd59a1039
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=82f07257d93271021a8e84f6cb4ad30580e1bc15&tochange=ac3ccd00426152daa9f7afbba00cc6abd59a1039
Blocks: 1404167
Has Regression Range: --- → yes
Flags: needinfo?(emilio)
Priority: -- → P2
Summary: Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?) in [@ mozilla::ServoRestyleState::ChangesHandledFor] → stylo: Assertion failure: mOwner == ExpectedOwnerForChild(aFrame) (Missed some frame in the hierarchy?) in [@ mozilla::ServoRestyleState::ChangesHandledFor]
Flags: needinfo?(bzbarsky)
Bughunter has also seen this on https://store.shopping.yahoo.co.jp/pfudirect/pz-kbkp-kks.html and I've reproduced locally as well.
Looks like ExpectedOwnerForChild should just return first continuation.
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.
Assignee: nobody → xidorn+moz
Status: NEW → ASSIGNED
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.
Attachment #8916458 - Flags: review+
Thanks for looking into this xidorn, hadn't managed to get to this one yet.
Flags: needinfo?(emilio)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fac14728144
Return first continuation for parent of first-letter in ExpectedOwnerForChild. r=emilio
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/3fac14728144
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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 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
Attachment #8916458 - Flags: approval-mozilla-beta?
Hi Selena, could you please help determine if this is a must fix for 57? Thanks!
Flags: needinfo?(sdeckelmann)
(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.
Flags: needinfo?(sdeckelmann)
Comment on attachment 8916458 [details]
Bug 1406562 - Return first continuation for parent of first-letter in ExpectedOwnerForChild.

Low risk, Beta57+
Attachment #8916458 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.