854263 - "Assertion failure: childDesiredSize.height >= aItem.GetBorderPaddingSizeInAxis(aAxisTracker.GetCrossAxis()) (Child should ask for at least enough space for border/padding), at nsFlexContainerFrame.cpp:2011", with <embed>, border or padding, flex containe

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 Load attached testcase in a debug build.

ACTUAL RESULTS:
Fatal assertion:
{
Assertion failure: childDesiredSize.height >= aItem.GetBorderPaddingSizeInAxis(aAxisTracker.GetCrossAxis()) (Child should ask for at least enough space for border/padding), at layout/generic/nsFlexContainerFrame.cpp:2011
}

This is the same assertion as in bug 801268, except here, we're hitting it without any absurdly-large sizes.

Comment 1

[Daniel Holbert [:dholbert]]

Looks like we start out with an actual "nsFrame" instance here, which uses the trivial nsFrame::Reflow implementation, which sets aDesiredSize to 0,0.  Then later, we get an "actual" frame, after we've figured out that our <embed> element doesn't have an appropriate plugin.

It seems a bit silly that nsFrame::Reflow doesn't request any space for its computed border/padding.  But I suppose it makes sense, given that nsFrame also uses the inherited (no-op) BuildDisplayList impl -- I guess it really wants to be a trivial, non-rendered zero-sized  frame (w/ border & padding not painted, and hence no visual space allocated for them).

This could be a bit complicated if we wanted to, in general, gracefully handle frames that arbitrarily decide not to request any space for their border/padding.  I think we can safely assume that frames will generally be good about that, though, unless they've got a 100%-trivial reflow impl like nsFrame does.

So, I'm tempted to do the following:
 (a) reduce the severity of this assertion to NS_ASSERTION, since it can also be triggered by huge nscoord values  and we don't want it aborting our builds. 
 (b) Add a special-case for frames that have nonzero computed border/padding and yet zero desired height.  Possibly assert that these frames are instances of nsFrame (i.e. GetType() is null), to be sure that it's the only one that falls into this special case.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix: allow for other frame classes that don't request enough space for border/padding

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v2: gracefully handle other frame classes that don't request enough space for border/padding

(Sorry, forgot to qref in a few tweaks before posting)

Comment 4

[Jesse Ruderman]

I think the assertion should be downgraded further, to NS_WARN_IF_FALSE, unless you plan to fix bug 765861.  NS_ASSERTION means "this is a bug", not "this is weird".

Comment 5

[Daniel Holbert [:dholbert]]

Well, it does indicate a bug, as long as we don't have nscoord overflow happening.

I think it should ultimately be an assertion that gets disabled when bug 765861's "crazy nscoord math" flag gets toggled.  For now, I'll make it NS_WARN_IF_FALSE with a comment to that effect.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v3: assertion softened to NS_WARN_IF_FALSE

(Softened assertion to a warning, per Jesse's feedback, with a comment indicating that we should upgrade it to an assertion once bug 765861 is fixed.)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 729891 [details] [diff] [review]
fix v3: assertion softened to NS_WARN_IF_FALSE

I think you ought to be more consistent about either using aAxisTracker or not; this code is a mix of both.  Given that it's fine not to given that all but the first few lines of the function are for vertical-cross-axis only, I think you're better off tilting towards not.

r=dbaron with that

Comment 8

[Daniel Holbert [:dholbert]]

Oh, good point. I should be checking aAxisTracker.GetCrossComponent(childDesiredSize), not childDesiredSize.height.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Oh, good point. I should be checking
> aAxisTracker.GetCrossComponent(childDesiredSize), not
> childDesiredSize.height.

...but more importantly, as you said at the end of comment 7: at this point in the code, we know we've got a vertical cross axis (i.e. we're a horizontal flex container), so we can drop the charade of using aAxisTracker to extract the cross component, and just directly reference childDesiredSize.height consistently.

I'll do that, as you suggest.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 729891 [details] [diff] [review]
fix v3: assertion softened to NS_WARN_IF_FALSE

>+  nscoord crossAxisBorderPadding =
>     aItem.GetBorderPaddingSizeInAxis(aAxisTracker.GetCrossAxis());

(Since we know at this point that our cross axis is vertical, we might as well use aItem.GetBorderPadding().TopBottom() here, too. We don't have a direct GetBorderPadding() accessor on FlexItem currently, just because we haven't needed one, but it's trivial to add one. I'll add one & invoke it here so we can just call TopBottom() on it.)

Comment 11

[Daniel Holbert [:dholbert]]

Pushed, with review comments addressed (i.e. not using aAxisTracker at all in this chunk of code): https://hg.mozilla.org/integration/mozilla-inbound/rev/1370c07c5d3d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1370c07c5d3d