Closed
Bug 363253
Opened 19 years ago
Closed 18 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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ajschult784, Assigned: roc)
References
()
Details
(Keywords: assertion, regression, testcase)
Attachments
(3 files)
2.00 KB,
text/plain
|
Details | |
43.74 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
10.72 KB,
text/plain
|
Details |
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•19 years ago
|
||
I'm also hitting this assertion on a XULRunner trunk build, Windows. OS -> All.
OS: Linux → All
Comment 2•19 years ago
|
||
Reporter | ||
Updated•19 years ago
|
Blocks: reflow-refactor
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•18 years ago
|
||
Could we do the attribute-changing from a reflow callback instead of doing it synchronously?
Flags: blocking1.9?
Assignee | ||
Comment 5•18 years ago
|
||
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•18 years ago
|
||
Yeah, nsIReflowCallback was what I meant.
Assignee | ||
Comment 7•18 years ago
|
||
I think this should be OK.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #254012 -
Flags: superreview?(dbaron)
Attachment #254012 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•18 years ago
|
||
Well, er, except it doesn't seem to have fixed the assertion...
Assignee | ||
Comment 9•18 years ago
|
||
Here's my stack trace in gdb...
We should probably still take my patch, even though it's unrelated to this bug.
![]() |
||
Comment 10•18 years ago
|
||
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
Attachment #254012 -
Flags: superreview?(dbaron)
Attachment #254012 -
Flags: superreview+
Attachment #254012 -
Flags: review?(dbaron)
Attachment #254012 -
Flags: review+
Assignee | ||
Comment 12•18 years ago
|
||
(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•18 years ago
|
||
Are those cases covered by comment 10?
Assignee | ||
Comment 14•18 years ago
|
||
I don't know.
![]() |
||
Comment 15•18 years ago
|
||
I was going to change them, but maybe we should just wait on bug 370952 for that.
Depends on: 370952
Comment 16•18 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]
Comment 17•18 years ago
|
||
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•18 years ago
|
||
Steve, see bug 372729.
Assignee | ||
Comment 19•18 years ago
|
||
(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•18 years ago
|
||
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.
Assignee | ||
Comment 21•18 years ago
|
||
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•18 years ago
|
||
Ok. I'm going to declare it a bug in Tp measurement, then... :(
Updated•18 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•18 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.
Assignee | ||
Comment 24•18 years ago
|
||
Indeed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
![]() |
||
Comment 25•18 years ago
|
||
Did you file this other bug? And move the relevant comments there? If so, what's the bug #?
Assignee | ||
Comment 26•18 years ago
|
||
Okay, I just filed bug 374167.
Updated•18 years ago
|
Flags: in-testsuite?
![]() |
||
Updated•18 years ago
|
Flags: blocking1.9?
You need to log in
before you can comment on or make changes to this bug.
Description
•