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

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tsmith, Assigned: mats)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

59 Branch
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox58 wontfix, firefox59 fixed, firefox60 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Posted file 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)
Flags: in-testsuite?
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).
See Also: → 1402766
Posted file 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.
Posted patch fix (obsolete) — Splinter Review
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)
Posted patch fixSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8219653e495985f33eb08d6a3134e84246f15de4
Assignee: nobody → mats
Attachment #8944142 - Attachment is obsolete: true
Attachment #8944175 - Flags: review?(dholbert)
Posted patch crashtestSplinter Review
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...
Blocks: 399412
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Posted file 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?
Flags: needinfo?(mats)
Comment on attachment 8944175 [details] [diff] [review]
fix

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

(Fix seems reasonable; r=me)
Attachment #8944175 - Flags: review?(dholbert) → review+
(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!
Flags: needinfo?(mats)
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/cef7ac1c276b
https://hg.mozilla.org/mozilla-central/rev/54c8e64328ce
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Do you want to request uplift to beta (59?)
Flags: needinfo?(mats)
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
Flags: needinfo?(mats)
Attachment #8944175 - Flags: approval-mozilla-beta?
Comment on attachment 8944175 [details] [diff] [review]
fix

Avoids an assertion, has some test coverage. Let's try this for 59 beta 4.
Attachment #8944175 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.