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

RESOLVED FIXED in Firefox 45

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla45
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(7 attachments)

(Reporter)

Description

3 years ago
Created attachment 8688234 [details]
testcase

Assertion failure: !sz.IsFrozen(), at layout/generic/nsGridContainerFrame.cpp:290
(Reporter)

Comment 1

3 years ago
Created attachment 8688235 [details]
stack
(Assignee)

Comment 2

3 years ago
Created attachment 8690412 [details]
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
(Assignee)

Comment 3

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

Comment 4

3 years ago
Created attachment 8690420 [details]
(WARNING: HANGS YOUR BROWSER) testcase #3

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

3 years ago
Created attachment 8690443 [details] [diff] [review]
fix
Attachment #8690443 - Flags: review?(dholbert)
(Assignee)

Comment 6

3 years ago
Created attachment 8690445 [details] [diff] [review]
reftests

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+
(Assignee)

Updated

3 years ago
Flags: in-testsuite+

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d58f495ddcfb
https://hg.mozilla.org/mozilla-central/rev/07c30e16ddbd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.