If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Andrew Schultz, Assigned: roc)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
All
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
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
Created attachment 248057 [details]
assertion stack trace, XULRunner
(Reporter)

Updated

11 years ago
Blocks: 300030
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.
Created attachment 254012 [details] [diff] [review]
fix

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...
Created attachment 254015 [details]
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.
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 don't know.
I was going to change them, but maybe we should just wait on bug 370952 for that.
Depends on: 370952

Updated

11 years ago
Depends on: 372729

Comment 16

11 years ago
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.

Updated

11 years ago
Depends on: 373122

Updated

11 years ago
Blocks: 371269
Ok.  I'm going to declare it a bug in Tp measurement, then... :(

Updated

11 years ago
Depends on: 373586

Updated

11 years ago
Depends on: 374090

Updated

11 years ago
Depends on: 374083

Updated

11 years ago
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')

Comment 23

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED
Did you file this other bug?  And move the relevant comments there?  If so, what's the bug #?
Okay, I just filed bug 374167.

Updated

11 years ago
Depends on: 375831

Updated

11 years ago
Depends on: 375822
Flags: in-testsuite?

Updated

11 years ago
Blocks: 373564

Updated

11 years ago
Depends on: 378273
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.