Closed Bug 1225368 Opened 9 years ago Closed 8 years ago

"Assertion failure: !sz.IsFrozen()" in nsGridContainerFrame::Tracks::CollectGrowable

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: assertion, testcase)

Attachments

(7 files)

Attached file testcase
Assertion failure: !sz.IsFrozen(), at layout/generic/nsGridContainerFrame.cpp:290
Attached file stack
Attached file testcase #2
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
Attached file testcase #2b
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.
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
Attached patch fixSplinter Review
Attachment #8690443 - Flags: review?(dholbert)
Attached patch reftestsSplinter Review
Both the assertion and hang are covered and various other similar situations.

FWIW, the rendering of this reftest is identical in Chrome.
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+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/d58f495ddcfb
https://hg.mozilla.org/mozilla-central/rev/07c30e16ddbd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: