1225118 - "ASSERTION: huh?" in AlignJustifySelf with display:grid, huge padding

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: huh?: 'contentSize >= 0', file layout/generic/nsGridContainerFrame.cpp, line 965

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Mats Palmgren (inactive)]

LOL, why am I not surprised? :-)

Comment 3

[Mats Palmgren (inactive)]

(gdb) list
967               // instead?  Do all fragments stretch? (bug 1144096).
968               bp.ApplySkipSides(aRS.frame->GetLogicalSkipSides());
969               nscoord bpInAxis = aAxis == eLogicalAxisBlock ? bp.BStartEnd(wm)
970                                                             : bp.IStartEnd(wm);
971               nscoord contentSize = size - bpInAxis;
972               NS_ASSERTION(contentSize >= 0, "huh?");
973               const nscoord unstretchedContentSize = contentSize;
974               contentSize += gap;
975               nscoord max = aAxis == eLogicalAxisBlock ? aRS.ComputedMaxBSize()
976                                                        : aRS.ComputedMaxISize();
(gdb) p size
$2 = 1073741824
(gdb) p/x size
$3 = 0x40000000
(gdb) p bpInAxis
$4 = 1073741944
(gdb) p/x bpInAxis
$5 = 0x40000078
(gdb) p bp
$6 = {
  mWritingMode = {
    mWritingMode = 0 '\000'
  }, 
  mMargin = {
    <mozilla::gfx::BaseMargin<int, nsMargin>> = {
      top = 60, 
      right = 1073741884, 
      bottom = 60, 
      left = 60
    }, <No data fields>}
}
(gdb) p/x bp
$7 = {
  mWritingMode = {
    mWritingMode = 0x0
  }, 
  mMargin = {
    <mozilla::gfx::BaseMargin<int, nsMargin>> = {
      top = 0x3c, 
      right = 0x4000003c, 
      bottom = 0x3c, 
      left = 0x3c
    }, <No data fields>}
}

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

nscoord overflow - resistance is futile!

Normally I don't care what the rendering is for tests like this, but I guess
we should at least return a non-negative content size here.

Comment 5

[Mats Palmgren (inactive)]

Attachment: crashtest

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8687996 [details] [diff] [review]
fix

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +974,5 @@
>            didResize = gap > 0;
>            if (didResize) {
> +            auto& size = (aAxis == eLogicalAxisBlock ? aContentSize->BSize(wm)
> +                                                     : aContentSize->ISize(wm));
> +            size = std::max(0, contentSize);

I'm not sure that this intervention (clamping contentSize to 0 when we set |size|) is the right thing to do here, particularly if we leave "gap" as something nonzero and perform the "offset += gap" step below it.  That offset+=gap step is premised on the assumption that we just adjusted the size.

IMO it makes marginally more sense (and allows for better branch-prediction & fewer assignments) if we just add a contentSize check to this clause -- so, change the didResize check to:
  if (didResize && MOZ_LIKELY(contentSize >= 0)) {

The idea being, regardless of "didResize", we only actually *do resize* if we're left with a nonnegative size.  Does that make sense?

Also: whatever intervention we use, it's probably worth adding a comment to indicate that this is a *bizarre edge case* & not a normal circumstance. e.g.:
  // (contentSize can only be negative if there's absurdly large sizes involved)

(Also, side-note: your variable "size" here actually shadows a variable with the same name that's declared earlier in this case statement:
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp?rev=5ae04c1c341c#951
But per my main point here RE the didResize check, I don't think we need to modify the contents of this clause anyway.)

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix

Yeah, that's better, thanks.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8688059 [details] [diff] [review]
fix

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

Thanks!

Maybe tweak commit message to mention grid and/or "justify-content:stretch", too. (Current message, "Deal with nscoord overflow a little better", is perhaps a bit generic.)

r=me in any case.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Maybe tweak commit message to mention grid and/or "justify-content:stretch",

er, meant to say "justify-self" (not "justify-content")

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b25b71b35630
https://hg.mozilla.org/integration/mozilla-inbound/rev/b741d265470e

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b25b71b35630
https://hg.mozilla.org/mozilla-central/rev/b741d265470e