478465 - Display list code clobbers NS_FRAME_IN_REFLOW

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

NS_FRAME_IN_REFLOW and NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO have the same value, with the latter having a comment about only being set during painting. But on the testcase Jesse attached in bug 67752 comment 57, I get asserts about NS_FRAME_IN_REFLOW not being set when it should be, and the reason for that is this stack:

#0  0x0227fedd in nsIFrame::RemoveStateBits (this=0xf692780, aBits=1) at nsIFrame.h:1046
#1  0x0229dd79 in UnmarkFrameForDisplay (aFrame=0x1505b94) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/base/nsDisplayList.cpp:137
#2  0x022a22f4 in nsDisplayListBuilder::LeavePresShell (this=0xbfff928c, aReferenceFrame=0xf692780, aDirtyRect=@0xbfff950c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/base/nsDisplayList.cpp:202
#3  0x022c6637 in nsLayoutUtils::ComputeRepaintRegionForCopy (aRootFrame=0xf692780, aMovingFrame=0xf69eac0, aDelta=@0xbfff9598, aCopyRect=@0xbfff95e8, aRepaintRegion=0xbfff96ac) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/base/nsLayoutUtils.cpp:1271
#4  0x022db9f4 in PresShell::ComputeRepaintRegionForCopy (this=0x1512c00, aRootView=0x108b9c90, aMovingView=0x854f370, aDelta=@0xbfff9630, aCopyRect=@0xbfff95e8, aRepaintRegion=0xbfff96ac) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/base/nsPresShell.cpp:4966
#5  0x02805b17 in nsViewManager::CanScrollWithBitBlt (this=0x89ef1c0, aView=0x854f370, aDelta=@0xbfff96fc, aUpdateRegion=0xbfff96ac) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/view/src/nsViewManager.cpp:1690
#6  0x02801a1b in nsScrollPortView::Scroll (this=0x1088fe50, aScrolledView=0x854f370, aTwipsDelta=@0xbfff979c, aPixDelta=@0xbfff9794, aP2A=60) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/view/src/nsScrollPortView.cpp:528
#7  0x02802534 in nsScrollPortView::ScrollToImpl (this=0x1088fe50, aX=0, aY=1140) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/view/src/nsScrollPortView.cpp:668
#8  0x02802c36 in nsScrollPortView::ScrollTo (this=0x1088fe50, aDestinationX=0, aDestinationY=1140, aUpdateFlags=0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/view/src/nsScrollPortView.cpp:255
#9  0x0234e9ea in nsGfxScrollFrameInner::ScrollToRestoredPosition (this=0x1505bd8) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/generic/nsGfxScrollFrame.cpp:1528
#10 0x0235572f in nsHTMLScrollFrame::Reflow (this=0x1505b94, aPresContext=0xf70e000, aDesiredSize=@0xbfff9bb4, aReflowState=@0xbfff9ae8, aStatus=@0xbfffa104) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/generic/nsGfxScrollFrame.cpp:828

This clobbers the NS_FRAME_IN_REFLOW bit on frames that are in fact currently in reflow...

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ouch.

I'm not sure what to do here. I think we might be able to scavenge the NS_FRAME_INDEPENDENT_SELECTION state bit, and reassign it to NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or we could just remove views and scavenge two state bits!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Is it possible to do the ScrolltoRestoredPosition off a reflow callback?  Or not so much?

Alternately, can we just use a debug-only boolean member for NS_FRAME_IN_REFLOW?  The only non-assertion use seems to be in MathML code, and if we can find a different solution for that...

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> Is it possible to do the ScrolltoRestoredPosition off a reflow callback?  Or
> not so much?

Yeah, that should work.

> Alternately, can we just use a debug-only boolean member for
> NS_FRAME_IN_REFLOW?  The only non-assertion use seems to be in MathML code, and
> if we can find a different solution for that...

A use in nsHTMLReflowState looks real. The MathML use could be switched to a MathML state bit.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The nsHTMLReflowState use is just an optimization, as far as I can tell.  But yes, I agree that it's a real use; I missed that one.

If we can restore off a reflow callback then that's what we should probably do.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Not sure whether there's a sane place I could assert that we don't build display lists while reflowing....

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

mUpdateScrollbarAttributes isn't used anywhere?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er...  It should be!  Specifically, this line:

+  if (NS_SUBTREE_DIRTY(mOuter))

should be:

+  if (NS_SUBTREE_DIRTY(mOuter) || !mUpdateScrollbarAttributes)

and the setting of mUpdateScrollbarAttributes to false should move to after that check.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: With that fixed

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4ffbac1f19eb