1278461 - [css-grid] Assertion failure: "NS_STATE_GRID_DID_PUSH_ITEMS lied" with -moz-column, grid, display:contents

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: foundOwnPushedChild || !items.IsEmpty() || mDidPushItemsBitMayLie (NS_STATE_GRID_DID_PUSH_ITEMS lied), at layout/generic/nsGridContainerFrame.cpp:5435

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jet Villegas (inactive)]

To Brad for a look.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: fixGridAssert1.patch

Mats, this is the smallest intervention I could find to deal with this testcase. I couldn't find a way to prevent setting the NS_STATE_GRID_DID_PUSH_ITEMS bit or to unset it only in the right conditions, so I instead use the existing mDidPushItemsBitMayLie variable.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25223d6962cc

Comment 4

[Mats Palmgren (inactive)]

It's not clear to me why we fix the prev-in-flow's state here.
Do we actually assert, during reflow of 'this', something about the the prev-in-flow's state?
Or is this just setting the mDidPushItemsBitMayLie bit for a potential future reflow of
prev-in-flow?

Can you elaborate on the series of steps of pushing/pulling items, reflowing
nsGridContainerFrames etc, that leads up to this state where we will assert?
What does the frame trees look like, and what are the DID_PUSH_ITEMS bits set
to in each step?

Comment 5

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #4)
> Or is this just setting the mDidPushItemsBitMayLie bit for a potential
> future reflow of
> prev-in-flow?
It is this. In the testcase, a fragmented grid is reflowed, and the first fragment nsGridContainerFrame sets the bit. Without this patch, then the second fragment nsGridContainerFrame takes all of the overflow children BUT leaves the bit set. The testcase then makes a dynamic style change to "display:contents", and the first fragment is reflowed without touching the bit, triggering the assert.

It would be a better solution to ensure that the NS_STATE_GRID_DID_PUSH_ITEMS bit is cleared on appropriate nsGridContainerFrames when the style is changed, before triggering reflow. I tried to isolate an appopriate point to do this, but I couldn't find an existing nsGridContainerFrame virtual function that would work for both this testcase and the existing tests. The closest candidate I found was nsGridContainerFrame::DrainSelfOverflowList, which is definitely called during the dynamic style change and solved the problem for this testcase, but clearing the bit there triggered a different assert when running layout/reftests/css-grid/grid-fragmentation-dyn1-016.html. There's probably a way to thread the needle, but I wasn't able to find it. If the approach in the proposed patch is unacceptable, I'll go back to this investigation and keep trying. My goal then would be to try to avoid creating another virtual function in nsGridContainerFrame.

I'll briefly make an argument for the proposed patch: If we're going to have a flag called mDidPushItemsBitMayLie, and we currently set it when we remove some but not all of the possible pushed items (in nsGridContainerFrame::RemoveFrame), then the usage in this proposed patch is consistent with that.

Comment 6

[Mats Palmgren (inactive)]

> then the second fragment nsGridContainerFrame takes all of the overflow children BUT leaves the bit set

This is a correct behavior, the bit reflects that we "pushed an item", so the child
frame may be on the Overflow list of the container that pushed, or on the Principal
or Overflow lists of any of the pushing container's continuations.

> The testcase then makes a dynamic style change to "display:contents", and the first fragment is reflowed without touching the bit, triggering the assert.

OK, so we got a call to RemoveFrame from that, right?
Why didn't the mDidPushItemsBitMayLie assignment there help?
Can we just set mDidPushItemsBitMayLie on 'this' and all its prev-in-flows there?

> It would be a better solution to ensure that the NS_STATE_GRID_DID_PUSH_ITEMS

This means we need to walk the child lists of continuations to figure out
if the bit should be set or not, which is expensive.  It's better to do it
at the start of Reflow, so that it coalesces multiple child removals etc.
Also it's more robust if DID_PUSH_ITEMS only means "we did push some item
in the past, but it may have been removed since then" rather than relying
on it to be exact at all times.  I'm aiming for it to be correct in the case
of pulling/pushing items due to reflows though, but care less about dynamic
changes like restyle.

Comment 7

[Mats Palmgren (inactive)]

Just to make sure I understand what happens in the test...
We fragment the grid container in two columns, the "x" item
ends up in the 2nd column and since we pushed it the first
container fragment has the DID_PUSH_ITEMS set.  Then we set
display:contents on "x" and its frame is destroyed, we get
a call to RemoveFrame (with the 2nd container fragment as
'this') and set mDidPushItemsBitMayLie on 'this' there?
Then we reflow the 1st container fragment but it can't find
any pushed items on its next-in-flows so it asserts?

Comment 8

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #6)
> > The testcase then makes a dynamic style change to "display:contents", and the first fragment is reflowed without touching the bit, triggering the assert.
> 
> OK, so we got a call to RemoveFrame from that, right?
> Why didn't the mDidPushItemsBitMayLie assignment there help?
> Can we just set mDidPushItemsBitMayLie on 'this' and all its prev-in-flows
> there?

Indeed, RemoveFrame is called on the second fragment nsGridContainerFrame, but not on the first. I will extend the setting to all prev-in-flows and that should also solve the problem.

> Just to make sure I understand what happens in the test...
> We fragment the grid container in two columns, the "x" item
> ends up in the 2nd column and since we pushed it the first
> container fragment has the DID_PUSH_ITEMS set.  Then we set
> display:contents on "x" and its frame is destroyed, we get
> a call to RemoveFrame (with the 2nd container fragment as
> 'this') and set mDidPushItemsBitMayLie on 'this' there?
> Then we reflow the 1st container fragment but it can't find
> any pushed items on its next-in-flows so it asserts?

Yes that seems to be what is happening. I'll follow your proposed approach (RemoveFrame set of the mDidPushItemsBitMayLie flag goes back through prev-in-flow containers) to solve it.

Comment 9

[Brad Werth [:bradwerth]]

Attachment: fixGridAssert2.patch

RemoveFrame propegates the mDidPushItemsBitMayLie bit up the prev-in-flow chain.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd82226b234

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8793515 [details] [diff] [review]
fixGridAssert2.patch

Thanks!  Does this also fix the other bug filed yesterday?

Comment 11

[Brad Werth [:bradwerth]]

To be determined. I can't replicate bug 1304184 with current mozilla-central (prior to this patch), so I'm building the specific revision mentioned in the other bug and testing again.

Comment 12

[Brad Werth [:bradwerth]]

Since this is r+ and probably-related bug 1304184 needs more time to replicate and remediate, I'm putting this in for checkin now.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Can you please include the testcase as a crashtest?

Comment 14

[Brad Werth [:bradwerth]]

Confirmed that this fix also fixes bug 1304184 which has a similar pathology. I will add both testcases as crashtests in next patch.

Comment 15

[Brad Werth [:bradwerth]]

Attachment: fixGridAssert3.patch

Adds the testcase requested in comment 13, and also a testcase for bug 1304184, which this patch also fixes.

Comment 16

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #15)
> Adds the testcase requested in comment 13, and also a testcase for bug
> 1304184, which this patch also fixes.

s/testcase/crashtest

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d343d42820d9
Prevent an assert from tripping when removing an overflowed frame during a partial reflow. r=mats

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d343d42820d9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d343d42820d9