Closed Bug 363253 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: roc)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files)

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.
OS: Linux → 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.
Blocks: 363391
Could we do the attribute-changing from a reflow callback instead of doing it synchronously?
Flags: blocking1.9?
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.
Attached patch fixSplinter Review
I think this should be OK.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #254012 - Flags: superreview?(dbaron)
Attachment #254012 - Flags: review?(dbaron)
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.
Blocks: 366320
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.
Blocks: 371038
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
Attachment #254012 - Flags: superreview?(dbaron)
Attachment #254012 - Flags: superreview+
Attachment #254012 - Flags: review?(dbaron)
Attachment #254012 - Flags: review+
(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 was going to change them, but maybe we should just wait on bug 370952 for that.
Depends on: 370952
Depends on: 372729
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.
Depends on: 373122
Blocks: 371269
Ok.  I'm going to declare it a bug in Tp measurement, then... :(
Depends on: 373586
Depends on: 374090
Depends on: 374083
Summary: ASSERTION: wasDirty lied: 'mDirtyRoots.IndexOf(f) == -1' opening a new window → Move scrollframe attribute-setting out of reflow to a post-reflow callback. (was: ASSERTION: wasDirty lied: 'mDirtyRoots.IndexOf(f) == -1')
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Did you file this other bug?  And move the relevant comments there?  If so, what's the bug #?
Depends on: 375831
Depends on: 375822
Flags: in-testsuite?
Blocks: 373564
Depends on: 378273
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.