399352 - assertions due to GetFrameForPoint while frames are dirty: "ASSERTION: cannot call on a dirty frame not currently being reflowed"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Running trunk Firefox for the past few weeks, I see frequent assertions:

    ###!!! ASSERTION: cannot call on a dirty frame not currently being reflowed: '!NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /builds/trunk/mozilla/layout/generic/
    nsIFrame::GetUsedBorder() const (/builds/trunk/mozilla/layout/generic/nsFrame.cpp:554)
    nsIFrame::GetPaddingRect() const (/builds/trunk/mozilla/layout/generic/nsFrame.cpp:640)
    ApplyOverflowClipping (/builds/trunk/mozilla/layout/generic/nsFrame.cpp:1007)
    nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsRect const&, nsDisplayListSet const&, unsigned int) (/builds/trunk/mozilla/layout/generic/nsFrame.cpp:1356)
    nsBoxFrame::BuildDisplayListForChildren(nsDisplayListBuilder*, nsRect const&, nsDisplayListSet const&) (/builds/trunk/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1309)
    nsRootBoxFrame::BuildDisplayList(nsDisplayListBuilder*, nsRect const&, nsDisplayListSet const&) (/builds/trunk/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp:248)
    nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsRect const&, nsDisplayListSet const&, unsigned int) (/builds/trunk/mozilla/layout/generic/nsFrame.cpp:1374)
    ViewportFrame::BuildDisplayList(nsDisplayListBuilder*, nsRect const&, nsDisplayListSet const&) (/builds/trunk/mozilla/layout/generic/nsViewportFrame.cpp:109)
    nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsRect const&, nsDisplayList*) (/builds/trunk/mozilla/layout/generic/nsFrame.cpp:1165)
    nsLayoutUtils::GetFrameForPoint(nsIFrame*, nsPoint, int) (/builds/trunk/mozilla/layout/base/nsLayoutUtils.cpp:760)
    PresShell::HandleEvent(nsIView*, nsGUIEvent*, nsEventStatus*) (/builds/trunk/mozilla/layout/base/nsPresShell.cpp:5482)
    nsViewManager::HandleEvent(nsView*, nsPoint, nsGUIEvent*, int) (/builds/trunk/mozilla/view/src/nsViewManager.cpp:1296)
    ...


From #developers:
    <roc> I think so
    <roc> otherwise we have to flush before processing any mouse event
    <dbaron> roc, (I'm seeing a lot of assertions about GetUsedBorder being called when frames are dirty within GetFrameForPoint.)
    <roc> which means we flush on every mouse move
    <roc> which sounds bad
    <dbaron> roc, so I suppose we should change that assertion to only check that it's not NS_FRAME_FIRST_REFLOW?
    <dbaron> roc, although we might fail for even that...
    <roc> if that sounds reasonable to you, yeah
    <roc> but yeah
    <roc> well
    <roc> frames are zero-sided before they're first reflowed
    <roc> so maybe we won't hit that
    <roc> what do you think?
    <dbaron> the problem is that the function will return some mix of old and new data
    <roc> yeah
    <dbaron> since it will have the new style data
    <roc> Ideally we would process restyling, frame construction and reflow as an atomic unit
    <roc> actually
    <dbaron> I'm not so sure flushing on mouse move is that horrible
    <roc> ok
    <roc> I'm not sure either
    <roc> I'm just scared
    <dbaron> what was the "actuall"?
    <dbaron> y
    <roc> we no longer have style-only flushes
    <roc> oh, we do
    <roc> never mind
    <dbaron> Anyway, I suppose I shouldn't be worrying about this now...
    <roc> I'm worried about mouse movement affecting Tp by adding extra flushes
    <roc> which we would not be able to pick up in tests, but could affect the user experience a lot
    <roc> in impossible-to-reproduce ways
    <dbaron> yeah
    <dbaron> I guess I should at least file a bug on this, since I couldn't find one
    <roc> is it true that styles won't change unless/until we process a restyle event?
    <roc> dbaron: another consideration is the desire for interruptible reflow
    <roc> we'd like to be able to stop reflow, process some input events, and start it again
    <roc> that's going to be pointless if mouse events force a flush

Problems caused by this issue are a regression in this cycle, since before GetUsedBorder, GetUsedPadding, etc, we computed all of this from the style data.  I suspect there probably are some real problems here since it means we're mixing new style data with old frame coordinates, which means the coordinates we're using don't correspond with what's painted.  Then again, maybe it's not a regression since in the old world we'd be using only new style data, but we'd still have the old stuff painted.

Comment 1

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

Er, I cut off the first line of the IRC discussion, which was:

<dbaron> roc, should we be allowed to do GetFrameForPoint when frames are dirty?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Should fix this, and also assertions during painting.

Copying over discussion from bug 366021 regarding assertion during painting:

--- Comment #10 David Baron [:dbaron]  2008-01-03 22:12:24 PST

Disabling during painting seems wrong.  I agree that we should disable it
during event handling (there's another bug discussing that), but why does it
need to be disabled during painting?

This bug, on the other hand, is about the assertion firing during reflow, which
your patch wouldn't fix.  Maybe it belongs on a different bug?

--- Comment #12 Robert O'Callahan (:roc)  2008-01-05 01:40:12 PST

We aren't always able to fully flush reflows before painting. See
http://mxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#5894

[This will still be true even in a post-compositor world, assuming we do
interruptible reflow as well. For example if you have some translucent chrome
floating over a content window and we need to repaint that chrome, we'll have
to paint the content window without flushing its reflows, otherwise chrome
event processing would be blocked by the reflow flush, basically eliminating
the point of interruptible reflow. In general we need to be able to paint and
handle events in a dirty frame tree.]

I'm not really sure what we should do about this. Perhaps we should just
convert those assertions to warnings.

--- Comment #13 [reply] David Baron [:dbaron]  2008-01-10 11:41:48 PST

Well, we could make frames have two style context pointers, one reflecting what
should be used in the next reflow, and one reflecting what was used in the last
one.


If we know why painting currently can trigger the assertion I think we should disable the assertion there too. I mentally just filter out all these assertions today since they trigger so often, which seems to defeat the point of having the assertion at all.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(the last paragraph was from me, not part of davids comment)

Comment 5

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

Comment on attachment 299907 [details] [diff] [review]
Patch v1

How about making sDisableGetUsedXAssertions a static member of class nsLayoutUtils instead of using extern?  With that, r+sr=dbaron.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we need tests for this. The only thing that would be worth testing here is the assertion-supression code, and if that regresses the worst thing that would happen is that we'd annoy developers by firing too many assertions.