1431781 - Assertion failure: blockFrame (Why did we have an IB split?), at /src/layout/generic/nsInlineFrame.cpp:956

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

[25954, Main Thread] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file /src/layout/base/nsLayoutUtils.cpp, line 7980
[25954, Main Thread] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /src/layout/base/nsLayoutUtils.cpp, line 7966
[25954, Main Thread] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /src/layout/base/nsLayoutUtils.cpp, line 7966
[25954, Main Thread] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /src/layout/base/nsLayoutUtils.cpp, line 7966
[25954, Main Thread] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /src/layout/base/nsLayoutUtils.cpp, line 7966
[25954, Main Thread] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file /src/layout/base/nsLayoutUtils.cpp, line 7980
Assertion failure: blockFrame (Why did we have an IB split?), at /src/layout/generic/nsInlineFrame.cpp:956

#0 nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit(mozilla::ServoRestyleState&) /src/layout/generic/nsInlineFrame.cpp:952:3
#1 nsIFrame::DoUpdateStyleOfOwnedAnonBoxes(mozilla::ServoRestyleState&) /src/layout/generic/nsFrame.cpp:11108:42
#2 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:978:19
#3 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:954:32
#4 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:954:32
#5 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:954:32
#6 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:954:32
#7 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:954:32
#8 mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:954:32
#9 mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:1147:28
#10 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4215:41
#11 nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1891:18
#12 mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:306:7
#13 mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:327:5
#14 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:769:5
#15 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:682:35
#16 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /src/layout/base/nsRefreshDriver.cpp:528:20
#17 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1040:14
#18 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:517:10
#19 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#20 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#21 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#22 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:157:27
#23 nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#24 XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4709:22
#25 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4871:8
#26 XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4963:21
#27 do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22
#28 main /src/browser/app/nsBrowserApp.cpp:304:16
#29 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#30 _start (/firefox+0x41f3d4)

Comment 1

[Daniel Holbert [:dholbert]]

The first assertion ("frame tree not empty, but caller reported complete status", which IIUC is where the problem really starts) is the same one we hit over in bug 1402766 comment 5 (though without using "page-break-inside" to trigger the problem, in this case).

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame tree

The reason we report a COMPLETE status is that we have a single
OverflowContainer child (yellow) and then we hit this code:
https://searchfox.org/mozilla-central/rev/9f2721b19b10a9a441fa5ca7116720d2fcebcc4a/layout/generic/nsContainerFrame.cpp#1160-1164
where the condition is true so we skip reflowing it.
The parent has no other children so it reports COMPLETE and thus
the whole next-in-flow chain after it is destroyed.

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix

This fixes it for me locally.  It's kinda wallpaper-ish, but this code
block is a wallpaper to begin with IMO (from bug 399412).

(not tested on Try yet though, since it's currently closed)

Comment 4

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8cc4a6fff0fdee00f3467ed0054dfaa5157540a

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8219653e495985f33eb08d6a3134e84246f15de4

Comment 6

[Mats Palmgren (inactive)]

Attachment: crashtest

This crashtest doesn't actually crash when loaded as a crashtest locally
for some reason.  (It only crashes when loading into a tabbed browser,
not when loaded into "firefox -layoutdebug".)  If anyone knows how to
fix that I'll be happy to update it.  I'll land it anyway for now...

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

I came up with a variant that triggers the *nonfatal* "frame tree not empty" assertion, at least (in -layoutdebug as well as in the crashtest harness).  And the assertion goes away if I apply your fix.

Assuming you see the same results, maybe include this as a second crashtest, and then we'll have some coverage for this?

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8944175 [details] [diff] [review]
fix

Review of attachment 8944175 [details] [diff] [review]:
-----------------------------------------------------------------

(Fix seems reasonable; r=me)

Comment 9

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Assuming you see the same results, maybe include this as a second crashtest,
> and then we'll have some coverage for this?

Yep, it does assert when run as a crashtest locally.
I'll include it in the crashtest patch, thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef7ac1c276b
Report at least OverflowIncomplete reflow status when we skip reflowing OverflowContainer children and have a next-in-flow.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c8e64328ce
Crashtests.

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/cef7ac1c276b
https://hg.mozilla.org/mozilla-central/rev/54c8e64328ce

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you want to request uplift to beta (59?)

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8944175 [details] [diff] [review]
fix

Approval Request Comment
[Feature/Bug causing the regression]:bug 399412
[User impact if declined]:crash
[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]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:the change is within a code block that is very rarely executed and it's a somewhat trivial change; though it's kinda hard to predict effects from changes like this
[String changes made/needed]:none

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8944175 [details] [diff] [review]
fix

Avoids an assertion, has some test coverage. Let's try this for 59 beta 4.

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68e0f9396d90
https://hg.mozilla.org/releases/mozilla-beta/rev/0c63b98288a7