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

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

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.

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

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.

Yeah, nsIReflowCallback was what I meant.

I think this should be OK.

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

Here's my stack trace in gdb... We should probably still take my patch, even though it's unrelated to this bug.

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

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

Are those cases covered by comment 10?

I don't know.

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

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]

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)

Steve, see bug 372729.

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

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.

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.

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

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.

Indeed.

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

Okay, I just filed bug 374167.