Closed
Bug 1225368
Opened 9 years ago
Closed 9 years ago
"Assertion failure: !sz.IsFrozen()" in nsGridContainerFrame::Tracks::CollectGrowable
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, testcase)
Attachments
(7 files)
Assertion failure: !sz.IsFrozen(), at layout/generic/nsGridContainerFrame.cpp:290
Reporter | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Here's an equivalent simplified testcase for debugging purposes.
FTR, what happens here is:
Just before starting Step 2.5:
http://hg.mozilla.org/mozilla-central/annotate/abbd213422a5/layout/generic/nsGridContainerFrame.cpp#l2394
(gdb) p plan[0]
$5 = {
mBase = 0,
mLimit = 0,
mPosition = 0,
mState = 17
}
(gdb) p plan[1]
$6 = {
mBase = 0,
mLimit = 1073741824,
mPosition = 0,
mState = 17
}
First item (of two with span=2) in step2Items:
(gdb) p space
$9 = 18000
after running DistributeToTrackLimits:
(gdb) p plan[0]
$12 = {
mBase = 0,
mLimit = 0,
mPosition = 0,
mState = 273 <--- frozen
}
(gdb) p plan[1]
$13 = {
mBase = 18000,
mLimit = 1073741824,
mPosition = 0,
mState = 17
}
Second item in step2Items:
(gdb) p space
$14 = 18000
ASSERT in CollectGrowable b/c track 2 is frozen (both items covers
the same tracks)
Assignee: nobody → mats
Assignee | ||
Comment 3•9 years ago
|
||
So I think this assertion is bogus and that we should subtract
the size of frozen items here like we currently do. That is,
for the span=2 item which contributes 300px we want to reduce
that number with the base size of all the tracks it covers,
also the frozen ones, to see how much is left that should
contribute towards growing the other (non-frozen, growable)
tracks.
Here's an example to show that the layout is correct (after
you downgrade the assertion to be non-fatal, or try it in
a non-debug build). The first column gets a width=30px from
the <x> element (span=1) and becomes frozen. Then we reduce
300px by 30px to 270px to distribute to the 2nd column so
that the spanned elements will have a total width of 300px.
Assignee | ||
Comment 4•9 years ago
|
||
There *is* a bug here though, which is that CollectGrowable add
also the frozen tracks to aGrowableTracks, which might cause
GrowTracksToLimit to spin forever since it expects those tracks
to be non-frozen on entry.
http://hg.mozilla.org/mozilla-central/annotate/abbd213422a5/layout/generic/nsGridContainerFrame.cpp#l335
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8690443 -
Flags: review?(dholbert)
Assignee | ||
Comment 6•9 years ago
|
||
Both the assertion and hang are covered and various other similar situations.
FWIW, the rendering of this reftest is identical in Chrome.
Comment 7•9 years ago
|
||
Comment on attachment 8690443 [details] [diff] [review]
fix
Review of attachment 8690443 [details] [diff] [review]:
-----------------------------------------------------------------
r=me, with documentation clarified as discussed below:
::: layout/generic/nsGridContainerFrame.cpp
@@ +273,5 @@
> /**
> * Collect the tracks which are growable (matching aSelector) and return
> * aAvailableSpace minus the sum of mBase's in aPlan for the tracks
> + * in aRange, or 0 if this subtraction goes below 0 or there are no
> + * growable tracks.
One nit -- it's not intuitively clear what the return value represents here. The documentation does precisely describe what will be returned (mathematically/logically), but it's non-obvious *why* this is a useful thing to return & what meaning the caller should draw from this returned value.
Could you start this comment with something like the following, explaining the high-level idea first:
* Collect the tracks which are growable (matching aSelector) into
* aGrowableTracks, and return the amount of space that can be used
* to grow those tracks. Specifically, we return aAvailableSpace minus
* the sum of [...etc etc]
That way, it's clear what the point of the returned value is, and then the details about edge cases make more sense in light of that.
Attachment #8690443 -
Flags: review?(dholbert) → review+
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite+
Comment 9•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d58f495ddcfb
https://hg.mozilla.org/mozilla-central/rev/07c30e16ddbd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•