Closed Bug 1431781 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect)

59 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(5 files, 1 obsolete file)

Attached 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
See Also: → 1431232
Attached 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.
Attached 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)
Attached 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)
Attached 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
Attached 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.