862947 - Assertion failure: "the root frame can't be a flex item, since being a flex item requires that you have a parent"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: !frame->IsFlexItem() (the root frame can't be a flex item, since being a flex item requires that you have a parent), at layout/generic/nsHTMLReflowState.cpp:1816

Comment 1

[Daniel Holbert [:dholbert]]

This assertion was added in bug 851379. Marking as blocking that bug.

So this is for this chunk of code:

> 1799 void
> 1800 nsHTMLReflowState::InitConstraints(nsPresContext* aPresContext,
> 1806 {
> 1807   DISPLAY_INIT_CONSTRAINTS(frame, this,
> 1808                            aContainingBlockWidth, aContainingBlockHeight,
> 1809                            aBorder, aPadding);
> 1810 
> 1811   // If this is the root frame, then set the computed width and
> 1812   // height equal to the available space
> 1813   if (nullptr == parentReflowState) {
> 1814     MOZ_ASSERT(!frame->IsFlexItem(),
> 1815                "the root frame can't be a flex item, since being a flex item "
> 1816                "requires that you have a parent");

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1800

...and parentReflowState is indeed null, *but* frame->GetParent() is non-null (and does in fact point to a flex container), so we *are* a flex item, and we're *not* the root frame.  So either that "If this is the root frame" comment is misleading, or the parentReflowState null-check is an invalid root-frame check.

We're in this situation because the nsDeckFrame's child (a block frame) receives a call to nsFrame::BoxReflow, as part of calculating its minimum size, and BoxReflow instantiates a reflow state for the *parent* (the deck frame in this case) using this nsHTMLReflowState constructor:
> 406   // Initialize a <b>root</b> reflow state with a rendering context to
> 407   // use for measuring things.
> 408   nsHTMLReflowState(nsPresContext*           aPresContext,
> 409                     nsIFrame*                aFrame,
> 410                     nsRenderingContext*     aRenderingContext,
> 411                     const nsSize&            aAvailableSpace,
> 412                     uint32_t                 aFlags = 0);
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.h#406

And that constructor sets its parentReflowState pointer to null (because it doesn't have access to a parent reflow state).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: possible fix

Haven't tested it yet, but I think this should fix this.

This clarifies the preexisting comment to say "reflow root" (i.e. a reflow state that's created with no parent), instead of "root frame".

(We can get these reflow states for the root frame, and also when reflow states are created for measuring purposes, as is the case in BoxReflow as noted above.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Fixed two typos in previous WIP, and added a crashtest.

Description of code-change:  The asserting code turned out to be a spot where we need to account for flex items -- so it needs the logic for "if this is a flex item then use the container height instead of the width, and convert NS_AUTOHEIGHT to 0".

I refactored out the existing instance of that logic into a helper-function, and invoke the helper function from both spots now.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 740541 [details] [diff] [review]
fix v1

r=mats

Drop the "Get" prefix perhaps?

Comment 5

[Daniel Holbert [:dholbert]]

Sounds good -- fixed that & landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4c26b1674a6e

Thanks!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c26b1674a6e