"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

RESOLVED FIXED in mozilla22

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({assertion})

Trunk
mozilla22
x86_64
Linux
assertion
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 728804 [details]
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 729299 [details] [diff] [review]
fix: allow for other frame classes that don't request enough space for border/padding
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 729309 [details] [diff] [review]
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)
Attachment #729299 - Attachment is obsolete: true
Attachment #729309 - Flags: review?(dbaron)

Comment 4

5 years ago
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".
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 729891 [details] [diff] [review]
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.)
Attachment #729891 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
Attachment #729309 - Attachment is obsolete: true
Attachment #729309 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
Blocks: 801268
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
Attachment #729891 - Flags: review?(dbaron) → review+
(Assignee)

Comment 8

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

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
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.)
(Assignee)

Comment 11

5 years ago
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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/1370c07c5d3d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.