Closed Bug 804089 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jruderman, Assigned: dholbert)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, reproducible, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file 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
Attached file stack
Keywords: reproducible
OS: Mac OS X → All
Hardware: x86_64 → All
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...
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
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.
Attached patch possible fix (obsolete) — Splinter Review
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...
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)
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.
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...
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.
Attached patch fix v2Splinter Review
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)
Attachment #674039 - Attachment is obsolete: true
Attachment #675774 - Flags: review?(dbaron)
Comment on attachment 675774 [details] [diff] [review]
fix v2

r=dbaron
Attachment #675774 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/dbc3eb4a6931
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.