310985 - add assertions to verify that we don't do frame construction in the middle of reflow, etc.

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Compiled, but not yet tested.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> That does a restyle asynchronously.

In most cases... there are the XUL menu hacks in there.  :(

OK on the rest.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

AttributeChanged is not fully async, unfortunately, because of XUL menus.

ContentStatesChanged should be ok, yeah.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Patch addressing bz's comments.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Scott MacGregor]

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. 

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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