We should add general assertions that we don't do frame construction in the middle of reflow or painting, etc. Patch coming.

Compiled, but not yet tested.

FWIW, it was bug 310833 that finally prompted me to write this.

bz, do you know of any good cases off the top of your head so that I can verify that these assertions fire when they should? (If not, I can add some code in my tree to do so, but that could be a little painful.)

Hmm.... I'd actually made an attempt to make sure they _wouldn't_ get hit. ;) You might remove the check for mChangeNestCount in PresShell::IsSafeToFlush and see where that gets you, maybe? Perhaps even by sticking random layout or frame flushes in various places? Or opening some XUL menus, or context menus -- I seem to recall those liking to try and flush reflow. Also, you probably want a FrameC entry point around nsCSSFrameConstruction::AttributeChanged and another one around pending restyle processing, at least.

(In reply to comment #4) > Also, you probably want a FrameC entry point around > nsCSSFrameConstruction::AttributeChanged That does a restyle asynchronously. > and another one around pending restyle > processing, at least. I checked that everything that restyle processing calls (sometimes frame construction, sometimes reflow, sometimes repaint) has the entry points marked, but since they're different, I'd rather not mark too high up.

> That does a restyle asynchronously. In most cases... there are the XUL menu hacks in there. :( OK on the rest.

With this patch, the assertions actually fire. A little too often, though.

Here's the current state of the patch, which I'm putting aside for now. There are also issues like that reflow destroys and creates frames when we rewrap lines. Then again, maybe I should only be checking those specific frame construction entry points and not the nsFrame.cpp assertions...

Comment on attachment 218473 [details] [diff] [review] patch >Index: base/nsCSSFrameConstructor.cpp Do we want to have an entry point in AttributeChanged too? Or do XUL menus make that fire all the time? What about ContentStatesChanged? >Index: base/nsPresContext.h Can we assert when painting happens in the middle of reflow or frame construction? Or are those triggered too much? >+ NS_ASSERTION(mPresContext->mLayoutPhaseCount[eLayoutPhase_Paint] == 0, >+ "reflowing in the middle of a paint"); >+ // Is this a problem?: Yes, since painting holds weak refs to frames via the display list and reflow can destroy frames (e.g. in-flows). Or did the comment refer to reflowing during frame construction? That one's a problem just because reflow on an inconsistent frame tree is likely to crash. >+ // The nsXBLService::LoadBindings call in ConstructFrameInternal >+ // makes us hit this one too often to be an NS_ASSERTION, >+ // despite how scary it is. Hmm... How come LoadBindings reenters frame construction? File a followup bug on this please, cc me? Also, won't this be hit in situations where we call ReconstructDocElementHierarchy() from frame construction? For example, WipeContainingBlock() can call ReconstructDocElementHierarchy() if it finds no blocks (say the testcases from bug 291902 or bug 296086). Similarly, ReframeContainingBlock() calls ReconstructDocElementHierarchy() in some cases. Do we just need a temp exit at those spots? >Index: base/nsPresShell.cpp >@@ -2941,24 +2942,27 @@ PresShell::ResizeReflow(nscoord aWidth, >+ // XXXldb Set mIsReflowing (and unset it later)? File followup? >@@ -3380,24 +3384,27 @@ PresShell::StyleChangeReflow() >+ // XXXldb Set mIsReflowing (and unset it later)? Same.

(In reply to comment #10) > (From update of attachment 218473 [details] [diff] [review] [edit]) > >Index: base/nsCSSFrameConstructor.cpp > > Do we want to have an entry point in AttributeChanged too? Or do XUL menus > make that fire all the time? What about ContentStatesChanged? What's the problem with those now that restyles are async?

AttributeChanged is not fully async, unfortunately, because of XUL menus. ContentStatesChanged should be ok, yeah.

Actually, even if it were still synchronous we wouldn't want it. I remember making sure all the codepaths through ProcessOneRestyle / ProcessPendingRestyles, etc., trigger the right entry points. But some of those are frame construction and some are reflow, and the patch already has the right entry points in both cases.

Patch addressing bz's comments.

Checked in to trunk. Still need to file followup bugs.

Filed bug 334429 on mIsReflowing and bug 334430 on nsXBLService::LoadBindings reentry into frame construction.

Hey David, I'm having a hard time using thunderbird with these new assertions. I get 7 or 8 I think when I bring up the compose window. The account manager triggered 20 of them before it would come up. The options dialog triggers them about 10 times. ###!!! ASSERTION: constructing frames in the middle of reflow: 'mPresContext->mL ayoutPhaseCount[eLayoutPhase_Reflow] == 0', file c:\build\trees\tbirddbg\mozilla \dist\include\layout\nsPresContext.h, line 831 I'm going to end up commenting off the assertions so I can use my debug build, but that is just masking issues on my end. Are there things we need to be changing or fixing in the front ends to avoid these frame construction warnings? Obvsiously the assertion is indicating that something bad is happening.

I changed the assertion on frame construction within reflow into a warning and filed bug 336756 on changing it back.