1349650 - [css-grid] Assertion failure: mIter.isSome() || mArray->Length() == aGridItemCount (grid item count mismatch)

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase asserts in mozilla-central rev e03e0c60462.

Assertion failure: mIter.isSome() || mArray->Length() == aGridItemCount (grid item count mismatch), at /home/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:474
#01: nsGridContainerFrame::Reflow at layout/generic/nsGridContainerFrame.cpp:6196
#02: nsBlockReflowContext::ReflowBlock at layout/generic/nsBlockReflowContext.cpp:307
#03: nsBlockFrame::ReflowBlockFrame at layout/generic/nsBlockFrame.cpp:3470
#04: nsBlockFrame::ReflowLine at layout/generic/nsBlockFrame.cpp:2831
#05: nsBlockFrame::ReflowDirtyLines at layout/generic/nsBlockFrame.cpp:2370
#06: nsBlockFrame::Reflow at layout/generic/nsBlockFrame.cpp:1250
#07: nsContainerFrame::ReflowChild at layout/generic/nsContainerFrame.cpp:900
#08: nsCanvasFrame::Reflow at layout/generic/nsCanvasFrame.cpp:722
#09: nsContainerFrame::ReflowChild at layout/generic/nsContainerFrame.cpp:900
#10: nsHTMLScrollFrame::ReflowScrolledFrame at layout/generic/nsGfxScrollFrame.cpp:556
#11: nsHTMLScrollFrame::ReflowContents at layout/generic/nsGfxScrollFrame.cpp:686
#12: nsHTMLScrollFrame::Reflow at layout/generic/nsGfxScrollFrame.cpp:1041
#13: nsContainerFrame::ReflowChild at layout/generic/nsContainerFrame.cpp:943
#14: mozilla::ViewportFrame::Reflow at layout/generic/ViewportFrame.cpp:330
#15: mozilla::PresShell::DoReflow at layout/generic/ReflowOutput.h:282

Comment 1

[Mats Palmgren (inactive)]

I can't reproduce the bug in a Linux64 debug build from the exact same rev
as yours (which is currently m-c tip), using a fresh profile.

Comment 2

[Mats Palmgren (inactive)]

Daniel, can you reproduce this assertion?

Comment 3

[Daniel Holbert [:dholbert]]

I can't reproduce either. I'm also on Linux64, using a debug mozilla-inbound build from this evening. (revision 3df85eb27d47)

Jesse, what OS are you on? Could you double-check that you're still seeing the assert? And does it reproduce 100% of the time on your system? (and/or any other factors you can think of that might explain why you're seeing it & we're not?)

Comment 4

[Daniel Holbert [:dholbert]]

Sorry, my clownshoes are showing -- I forgot that the build I tested in had "--disable-debug" in its mozconfig, so of course assertions were disabled.

I can reproduce this in a Linux64 debug build that I just downloaded from treeherder -- the "target.tar.bz2" tarball here:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=01125b5142e587fb8d8bd40c8dbaf6aaef71a9f9&filter-searchStr=linux%20x64%20debug%20tc%20B&selectedJob=85835007

(Direct link: https://queue.taskcluster.net/v1/task/MuZ5TB9zRb2g0XjQ-uSi2g/runs/0/artifacts/public/build/target.tar.bz2 )

Mats, if your tested local build is definitely a debug build & you still can't reproduce, maybe try that ^^ tarball as an extra sanity check?

Comment 5

[Daniel Holbert [:dholbert]]

Ah, the plot thickens -- my *locally-built* enable-debug/disable-optimize build does not reproduce the issue.

The main distinction between my just-tested local build & the downloaded tarball from comment 4 is "disable-optimize" (local bulid) vs "enable-optimize" (treeherder build -- technically unspecified, but this is the default).

So maybe this depends on some subtly-different initialization / out-of-order instructions that only happen in enable-optimize builds...?

Comment 6

[Mats Palmgren (inactive)]

Thanks Daniel, I indeed have disable-optimize in my .mozconfig.
I'll rebuild with enable-optimize and see if that helps.

Comment 7

[Jesse Schwartzentruber (:truber)]

Thanks Daniel. I only try taskcluster builds usually (mozilla-central.latest.firefox.linux64-debug). I didn't realize those are optimized.

Comment 8

[Mats Palmgren (inactive)]

Nope, that build didn't reproduce it either.  Hmm, it might be a gcc/clang difference...

Comment 9

[Mats Palmgren (inactive)]

Yep, now I can reproduce it.  gcc + enable-optimize did it.

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix

It's just a bogus assertion.
When mArray is used it contains all grid container children, including
placeholders.  aGridItemCount always excludes placeholders.

Comment 11

[Mats Palmgren (inactive)]

Attachment: crashtest

Comment 12

[Mats Palmgren (inactive)]

(BTW, I'm amazed how much faster enable-optimize makes the build, even with enable-debug.
It feels like an order of magnitude or so.  It does make it less debuggable though,
with gdb frequently displaying <optimized out> when trying to print variables.
It seems worth investigating if adding -Og and/or -ggdb helps with that though...)

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #9)
> Yep, now I can reproduce it.  gcc + enable-optimize did it.

Glad to hear it. I can't reproduce with *clang* + enable-optimize, FWIW. (I left that building overnight to test in the morning, & just tried it.  But I guess that's just because clang & gcc perform different optimizations.

Comment 14

[Daniel Holbert [:dholbert]]

D'oh, I just now noticed the "CLANG_CRASH_BUG" exclusion-ifdef around the condition -- that definitely explains why I'm not hitting it. :)  (I'm using clang 3.8.1 on this machine, which satisfies the CLANG_CRASH_BUG condition.)

Comment 15

[Daniel Holbert [:dholbert]]

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

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

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3670c556728b
[css-grid] Fix a bogus assertion.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e9870067e4
[css-grid] Crashtest.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3670c556728b
https://hg.mozilla.org/mozilla-central/rev/18e9870067e4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

I'm going to assume that this isn't worth backporting. Feel free to set things back to affected and make the requests if you feel otherwise, though.

Comment 19

[Daniel Holbert [:dholbert]]

I agree. It's a debug-only change, so there's no user benefit that we'd gain from uplifting this.

Comment 20

[Daniel Holbert [:dholbert]]

...though actually, this is in a piece of code that I want to move around over in Bug 812687 (and I'll want to uplift the patches over there).

So it would make for an easier/cleaner uplift over there, if we uplifted this bug's patches over here.

I think we don't need to bother with uplift approval over here, though -- this bug only changes the inside of a MOZ_ASSERT expression, so it's debug-only and hence NPOTB.

Comment 21

[Daniel Holbert [:dholbert]]

Uplifted to aurora:
  https://hg.mozilla.org/releases/mozilla-aurora/rev/08002a9d9e3ae337bdd23bf01432ba64251b880e
  https://hg.mozilla.org/releases/mozilla-aurora/rev/f09f78db26a594b76139cee19a2487f9509d92ee

...and beta:
  https://hg.mozilla.org/releases/mozilla-beta/rev/27126f140a2cf381f070afe905cf03659be8cd98
  https://hg.mozilla.org/releases/mozilla-beta/rev/5ec9b3104a4cc38b1d28e6f49f0c0dc61642314e

...per comment 20. (I'd like it to as easy as possible for bug 812687 to be uplifted, and this bug's NPOTB changes are a requirement for that).