Closed Bug 1229999 Opened 4 years ago Closed 4 years ago

[css-grid] Assertion failure: "invalid start line" with grid, rel & abs pos

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: aStart >= nsStyleGridLine::kMinLine && aStart <= nsStyleGridLine::kMaxLine (invalid start line), at layout/generic/nsGridContainerFrame.h:139
Attached file stack
Attached patch fixSplinter Review
The problem is that ResolveAutoPosition is a bit too permissive:
http://hg.mozilla.org/mozilla-central/annotate/e02b17a2b5b8/layout/generic/nsGridContainerFrame.h#l169

The kTranslatedMaxLine there is large enough that even with a maximum
number of implicit tracks before line 1, it still allows auto-placed
items up to kMaxLine in the untranslated grid.  The problem is that
with no, or just a few, implicit lines before line 1, it allows
lines that would be > kMaxLine in the untranslated grid.

This isn't much of a problem - we can deal with kTranslatedMaxLine
lines just fine, and the layout is correct.  It's just that we clamp
definite lines numbers and auto-line numbers a bit differently.

The assertion occurs because before we resolve lines for abs.pos.
children (after auto-placement) we untranslate the grid and that leads
to some lines being > kMaxLine which when we create the LineRange
for the abs.pos. item in this case asserts in the ctor:
http://hg.mozilla.org/mozilla-central/annotate/e02b17a2b5b8/layout/generic/nsGridContainerFrame.h#l129

This patch seems like the most correct solution since it treats auto-placed
items equally with definite-placed items w.r.t clamping.  There's a small
runtime cost here though.

An alternative solution would be to just relax this assertion, but only
for abs.pos. children.
Assignee: nobody → mats
Attachment #8695897 - Flags: review?(dholbert)
Attached patch reftestSplinter Review
Blocks: 1147423
Summary: Assertion failure: "invalid start line" with grid, rel & abs pos → [css-grid] Assertion failure: "invalid start line" with grid, rel & abs pos
Comment on attachment 8695897 [details] [diff] [review]
fix

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

Commit message nit:
> Clamp auto-placed lines the where kMaxLine is in the translated grid. r=dholbert

"the where" seems like a typo.

::: layout/generic/nsGridContainerFrame.h
@@ +166,5 @@
>        MOZ_ASSERT(IsAuto(), "Why call me?");
>        mStart = aStart;
>        mEnd += aStart;
>        // Clamping per http://dev.w3.org/csswg/css-grid/#overlarge-grids :
> +      uint32_t translatedMax = aExplicitGridOffset + nsStyleGridLine::kMaxLine;

Can you extend the comment slightly to explain the new behavior? (Right now the comment sounds like one can find an explanation for this aExplicitGridOffset addition in the #overlarge-grids spec section; but I don't think it's really clear.)

Maybe just add a parenthetical, so we end up with e.g.:
  // Clamping to kMaxLine (in the explicit grid), per
  // http://dev.w3.org/csswg/css-grid/#overlarge-grids :

Also: might be worth indicating in this function's documentation that |aStart| is a line-index in the *implicit* (untranslated) grid. (It is, right?)  This would help make the aExplicitGridOffset parameter & addition more obviously-correct, too.  (and the old kTranslatedMaxLine comparison obviously-bogus, since it's from a different coordinate space)
Attachment #8695897 - Flags: review?(dholbert) → review+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/ef6fa56c99ca
https://hg.mozilla.org/mozilla-central/rev/d01ad4aa08d5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.