Closed
Bug 804089
Opened 13 years ago
Closed 13 years ago
"ASSERTION: DidReflow() was never called" with flexbox and MathML
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: jruderman, Assigned: dholbert)
References
Details
(Keywords: assertion, reproducible, testcase)
Attachments
(3 files, 1 obsolete file)
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
Reporter | ||
Comment 1•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → dholbert
Depends on: css3-flexbox
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 2•13 years ago
|
||
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...
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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...
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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...
Assignee | ||
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
Status: NEW → ASSIGNED
Flags: in-testsuite+
Comment 13•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•