Closed Bug 1225592 Opened 4 years ago Closed 4 years ago

css grid repeat: "Assertion failure: mSizes.Length() >= explicitGridOffset + aFunctions.mMinSizingFunctions.Length()"

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase
Assertion failure: mSizes.Length() >= explicitGridOffset + aFunctions.mMinSizingFunctions.Length(), at layout/generic/nsGridContainerFrame.cpp:1918
Attached file stack
Assignee: nobody → mats
Attached patch fixSplinter Review
The style system change fixes the problem alone, but it doesn't hurt to make
the layout code robust against this kind of error.
Attachment #8689705 - Flags: review?(dholbert)
Attached patch crashtestSplinter Review
(can't push to Try right now due to bug 1226354)
Comment on attachment 8689705 [details] [diff] [review]
fix

Review of attachment 8689705 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me, with readability nits (below) addressed as you see fit.

::: layout/generic/nsGridContainerFrame.cpp
@@ +1927,5 @@
>    MOZ_ASSERT(aFunctions.mMinSizingFunctions.Length() ==
>                 aFunctions.mMaxSizingFunctions.Length());
>    uint32_t i = 0;
> +  uint32_t len = std::min<uint32_t>(explicitGridOffset, mSizes.Length());
> +  for (; i < len; ++i) {

"len" is a confusing name here. It sounds like it's the array-length, but it's not. (unless something's gone wrong)

Really, "len" here is our clamped explicit-grid-offset. So maybe call it "clampedExplicitGridOffset"?  A bit longer, but clearer what it represents (and clearer why that in particular is what we're iterating up to here).

(The current formulation, with "len", was initially confusing to me, because:
 - We walk across the array, doing work on mSizes[i], up to i == len.
 - Then we proceed *further* and potentially do work at higher indexes within mSizes, beyond the 'len' where we just stopped.
 - So, if I superficially interpret "len" as "array-length", this seems like we're walking out of mSizes' array-bounds. But really we're not, because len isn't really a length.)

@@ +1933,5 @@
>                           aFunctions.mAutoMinSizing,
>                           aFunctions.mAutoMaxSizing);
>    }
> +  len = std::min<uint32_t>(i + aFunctions.mMinSizingFunctions.Length(),
> +                           mSizes.Length());

Similarly, here, it's not clear what "len" is the length of. (It might be mSizes.Length(), but it might not be.)

Maybe call it "numTracksWithSizingFuncs" or something like that, if that's appropriate?

::: layout/style/nsRuleNode.cpp
@@ +7653,4 @@
>          AppendGridLineNames(item->mValue, aResult);
>          item = item->mNext;
>  
> +        if (!item || numTracks == nsStyleGridLine::kMaxLine - 1) {

It took me a little bit of staring at this line & the "for" expression to fully sanity-check that this comparison is indeed the right one to make (and that it's not off-by-one in one way or another).

In particular, the things that took some thought:
 - At this point, is numTracks is the number of tracks *including* what's currently in  |item|, or *excluding* it? (excluding.)
 - numTracks and kMaxLine are counts of two different things (tracks vs. lines)
 - -1 is to convert between those things.
 - Does this comparison let us reach kMaxLine total lines, or not-quite-reach it? (It lets us reach but not exceed it.)

SUGGESTION: since "numTracks" *only* exists to serve this comparison, it might reduce cognitive load slightly if you just directly tracked the line-count instead of the item-count. e.g.:

for (int32_t numGridLines = 1; ; ++numGridLines) {
  AppendGridLineNames(item->mValue, aResult);
  item = item->mNext;

  if (!item || numGridLines == nsStyleGridLine::kMaxLine) {
    break;
  }

(It might seem a bit odd to start at 1, but note that the very first thing we do is call AppendGridLineNames -- for the very first line.  And IIRC, a grid with 0 tracks is still considered to have 1 line.)

ALTERNATE SUGGESTION: if you'd prefer to keep this as-is with numTracks, please add a comment next to the comparison saying e.g.
  // If we hit kMaxLine grid-lines, then stop adding tracks.
Attachment #8689705 - Flags: review?(dholbert) → review+
I made it 1-based as you suggested and named it "line", so the comparison is now:

         if (!item || line == nsStyleGridLine::kMaxLine) {
           break;
         }
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/427053469dd9
https://hg.mozilla.org/mozilla-central/rev/3a60b22eb391
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(In reply to Mats Palmgren (:mats) from comment #7)
> I made it 1-based as you suggested and named it "line"

Thanks!
Duplicate of this bug: 1228213
You need to log in before you can comment on or make changes to this bug.