363253 - Move scrollframe attribute-setting out of reflow to a post-reflow callback. (was: ASSERTION: wasDirty lied: 'mDirtyRoots.IndexOf(f) == -1')

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andrew Schultz]

A current CVS seamonkey debug build asserts opening a new window via javascript:window.open("about:blank").  This is a reflow branch landing regression.  It gets triggered during the bloat tests.

###!!! ASSERTION: wasDirty lied: 'mDirtyRoots.IndexOf(f) == -1', file /build/andrew/moz-debug/mozilla/layout/base/nsPresShell.cpp, line 3500

Comment 1

[Alex Vincent [:WeirdAl]]

I'm also hitting this assertion on a XULRunner trunk build, Windows.  OS -> All.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: assertion stack trace, XULRunner

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Yeah, I think this is the fault of the manipulation that we do to the scrollbars in the middle of reflow.  Not entirely sure what to do about it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Could we do the attribute-changing from a reflow callback instead of doing it synchronously?

Comment 5

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

It's tricky because we need to know whether the thumb current position is being changed by nsGfxScrollFrameInner::LayoutScrollbars or for some other reason.

We may be able to do this by updating the scrollbars with an implementation of nsIReflowCallback instead of just posting attribute changes. I'll have a go.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, nsIReflowCallback was what I meant.

Comment 7

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

Attachment: fix

I think this should be OK.

Comment 8

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

Well, er, except it doesn't seem to have fixed the assertion...

Comment 9

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

Attachment: stack trace for assertion on Mac

Here's my stack trace in gdb...

We should probably still take my patch, even though it's unrelated to this bug.

Comment 10

[Boris Zbarsky [:bzbarsky]]

That patch does fix some instances of that assert that I was seeing.

For the rest, it seems to me that PresShell::StyleChangeReflow and PresShell::ResizeReflow should clear mDirtyRoots at some point, no?

I also think we should add an assert that the frame we're adding to mDirtyRoots is not currently in reflow; that should let us catch the scrollbar sort of problem earlier.

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Comment on attachment 254012 [details] [diff] [review]
fix

Does nsGfxScrollFrameInner::ReflowFinished need to return true if there
is neither an mHScrollbarBox nor an mVScrollbarBox?

GfxScrollFrameInner::SetScrollbarEnabled looks like it has the aMaxPos
test backwards.

r+sr=dbaron

Comment 12

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

(In reply to comment #11)
> Does nsGfxScrollFrameInner::ReflowFinished need to return true if there
> is neither an mHScrollbarBox nor an mVScrollbarBox?

No. Fixed.

> GfxScrollFrameInner::SetScrollbarEnabled looks like it has the aMaxPos
> test backwards.

Indeed. Fixed.

Checked in. Leaving this bug open because the assertion is still firing.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Are those cases covered by comment 10?

Comment 14

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

I don't know.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I was going to change them, but maybe we should just wait on bug 370952 for that.

Comment 16

[Olli Pettay [:smaug]]

Did this cause new crashes?
TB shows new entries: TB29962130 , TB29962100 , TB29962084 .

nsGfxScrollFrameInner::IsLTR  [mozilla/layout/generic/nsgfxscrollframe.cpp, line 2097]
nsGfxScrollFrameInner::GetScrolledRect  [mozilla/layout/generic/nsgfxscrollframe.cpp, line 2511]
nsGfxScrollFrameInner::FireScrollPortEvent  [mozilla/layout/generic/nsgfxscrollframe.cpp, line 1524]
nsGfxScrollFrameInner::AsyncScrollPortEvent::Run  [mozilla/layout/generic/nsgfxscrollframe.cpp, line 1895]
NS_ProcessNextEvent_P  [mozilla/xpcom/build/nsthreadutils.cpp, line 225]
nsBaseAppShell::Run  [mozilla/widget/src/xpwidgets/nsbaseappshell.cpp, line 153]

Comment 17

[Steve England [:stevee]]

It also looks like this improved Txul and Ts for Linux:
- http://tinyurl.com/3bax6q (Txul ~770ms to ~660ms)
- http://tinyurl.com/39hleo (Ts ~2360ms to ~2290ms)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Steve, see bug 372729.

Comment 19

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

(In reply to comment #16)
> Did this cause new crashes?
> TB shows new entries: TB29962130 , TB29962100 , TB29962084 .
> 
> nsGfxScrollFrameInner::IsLTR  [mozilla/layout/generic/nsgfxscrollframe.cpp,
> line 2097]
> nsGfxScrollFrameInner::GetScrolledRect 
> [mozilla/layout/generic/nsgfxscrollframe.cpp, line 2511]
> nsGfxScrollFrameInner::FireScrollPortEvent 
> [mozilla/layout/generic/nsgfxscrollframe.cpp, line 1524]
> nsGfxScrollFrameInner::AsyncScrollPortEvent::Run 
> [mozilla/layout/generic/nsgfxscrollframe.cpp, line 1895]
> NS_ProcessNextEvent_P  [mozilla/xpcom/build/nsthreadutils.cpp, line 225]
> nsBaseAppShell::Run  [mozilla/widget/src/xpwidgets/nsbaseappshell.cpp, line
> 153]

Possible, since I moved the revocation of the scrollport event to nsGfxScrollFrameInner::Destroy(), but as far as I can tell that does get called whenever the frame is destroyed, so I'm not sure what the problem is...

Comment 20

[Boris Zbarsky [:bzbarsky]]

roc, can you think of any way this would have improved Txul, even with bug 372729 fixed?

If yes, then I need to seriously look into why checking in the patch for bug 267833 brings Txul to the level it was at before this bug... otherwise, I am suspecting it's a bug in how Txul does measurements.

Comment 21

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

No, I can't really think of anything.

If we were reflowing scrollframes in multiple passes, this could be coalescing scrollbar updates, but I don't think we do that post-reflow-branch, certainly not in XUL.

The code for updating scrollbars is a little more efficient and direct now, but that can hardly be responsible for a measurable performance change.

Whatever whacky hypotheses I can come up with would affect Tp as well as Txul, anyway, and Tp didn't change.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ok.  I'm going to declare it a bug in Tp measurement, then... :(

Comment 23

[Jesse Ruderman]

Since the patch here caused regressions, I think it would ease regression-tracking if this bug were marked as FIXED and the patch for the remaining instances of the assertion happened in another bug.

Comment 24

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

Indeed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Did you file this other bug?  And move the relevant comments there?  If so, what's the bug #?

Comment 26

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

Okay, I just filed bug 374167.