804089 - "ASSERTION: DidReflow() was never called" with flexbox and MathML

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With
  user_pref("layout.css.flexbox.enabled", true);

###!!! ASSERTION: DidReflow() was never called: '!(childFrame->GetStateBits() & NS_FRAME_IN_REFLOW)', file layout/mathml/nsMathMLContainerFrame.cpp, line 530

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

This is probably because the flex container frame does some trial-reflows on its children, to measure them, before it does the final reflow and calls FinishReflowChild().

Perhaps I need to call FinishReflowChild() after each of those trial-reflows? I'd rather not, because I don't want to actually reposition / resize the children at that point...

Comment 3

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Note: We also trigger this warning before the assertion-failure:
WARNING: it is wrong to fire stretch more than once on a frame: file /mozilla/layout/mathml/nsMathMLmoFrame.cpp, line 612

Comment 4

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Yeah -- so basically:
 * Flex container does a trial-reflow of its <mo> child
    (a) That calls WillReflow, which sets the NS_FRAME_IN_REFLOW bit on the MRow frame
    (b) that calls Stretch(), which does 2 important things:
        (i)  it checks for the NS_MATHML_STRETCH_DONE bit, and sets that bit if it's unset.
        (ii) it calls Place(), which calls FinishReflowChild(), which calls DidReflow() on the MRow frame, which clears NS_FRAME_IN_REFLOW.

Then we do this all again, for the "real" reflow, except this time when we call Stretch(), we see that STRETCH_DONE bit is already set, so we warn and bail out.  As a result, we never clear the NS_FRAME_IN_REFLOW bit on the MRow, and we freak out and assert about that.

Comment 5

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Attachment: possible fix

karlt says in #developers:
> from a quick look, i'm not seeing any reason why
> calling Stretch twice would be so bad

Here's a patch to disable the early-return, letting us proceed with the second Stretch() call.  This fixes the assertion for me, and doesn't cause any obvious issues...

Comment 6

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Having said that, maybe it's technically-invalid for flex containers to be calling Reflow() multiple times on a given kid before issuing it a DidReflow() call...?  Looking at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#39 , it looks like I might be breaking those rules by calling Reflow() multiple times before calling DidReflow().  (via the wrappers ReflowChild / FinishReflowChild)

Comment 7

[Frédéric Wang (:fredw)]

From what I remember, I would also agree with Karl. However, the "trick" a few lines above the assertion is really weird. Perhaps that was the reason why this assertion is here.

Comment 8

[Frédéric Wang (:fredw)]

BTW, I wanted to say that somewhere but according to Dave Barton, using the flexbox model for MathML was the key change to clean Webkit's MathML implementation and make it enabled in Chrome Canary... I'm wondering if we could use this model in the future to fix various box issues we have in MathML...

Comment 9

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

I'm feeling more and more strongly that I should be calling DidReflow here, per comment 6.  It seems odd for us to already have NS_FRAME_IN_REFLOW turned on when we hit a child's WillReflow() method and set that bit again.  This feeling was reinforced by the (#ifdef'd off) debugging assertion to this effect in nsFrame::WillReflow():
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4151
There may be other state (e.g. this bug's MathML state) that we clean up in DidReflow & that we're expecting to be clean when we hit our next WillReflow call.

So -- I think nsFlexContainerFrame should be calling DidReflow after each of its "trial" child-reflows.

Since it's a nsContainerFrame, it could do this via the "FinishReflowChild()" wrapper-method, but I think we instead want to directly call DidReflow() ourselves, since the other things that FinishReflowChild() does are things that we want to avoid (set the frame's bounds, mess with its view, etc) until our final reflow when we do actually want all that stuff.

Comment 10

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

Attachment: fix v2

This adds a DidReflow() call after the trial/measuring reflow that we use to get the "auto" flex-basis & min-size for vertical-flexbox flex items.

Crashtest included. (I verified that it fails (asserts) in a crashtest run w/o the fix, and passes w/ the fix)

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Comment on attachment 675774 [details] [diff] [review]
fix v2

r=dbaron

Comment 12

[Daniel Holbert [:dholbert] (blocking needinfo/review, due to AFK on parental leave 12/9 - 1/6)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc3eb4a6931

Comment 13

[Phil Ringnalda (:philor)]

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