Closed Bug 379904 Opened 13 years ago Closed 13 years ago

[FIX]ResizeReflow can still cause wasDirty asserts

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Fix (obsolete) — Splinter Review
If we have a dirty root, and ResizeReflow, and that reflows the dirty root, we need to remove it from mDirtyRoots.  Otherwise we'll assert when it comes time to mark it dirty again.
Attachment #263950 - Flags: superreview?(dbaron)
Attachment #263950 - Flags: review?(dbaron)
Comment on attachment 263950 [details] [diff] [review]
Fix

This is a potential problem with a reflow of any root, at any time, so I don't think we should fix it in ResizeReflow.

(I think the only way to fix it that's not O(N^2) is to just no-op when we process the roots rather than asserting -- I guess we should change the assertion into a runtime check.)
Attachment #263950 - Flags: superreview?(dbaron)
Attachment #263950 - Flags: superreview-
Attachment #263950 - Flags: review?(dbaron)
Attachment #263950 - Flags: review-
This is only a problem if either:

1)  Frames are marked dirty during reflow

or

2)  Some reflow roots are reflow, but not others

#1 shouldn't be happening, as I understand.  #2 can only happen with ResizeReflow and if ProcessReflowCommands doesn't process all the roots.  We could also add a loop like this to the latter case.

I'm not sure what you mean by "just no-op when we process the roots rather than asserting"; do you mean the assert in FrameNeedsReflow?  Or something else?
I think we should:
 * add an assertion that frames aren't marked dirty in the middle of reflow
 * remove the wasDirty lied assertion and the code to remove extra mDirtyRoots (and just let the duplicates get put in, and then skipped by ProcessReflowCommands when they're not dirty)

What's wrong with occasionally having frames in mDirtyRoots an extra time?  We won't even reflow them an extra time, since we check NS_SUBTREE_DIRTY in ProcessReflowCommands.  I don't think it's worth O(N^2) algorithms to ensure that it doesn't happen.

(Sorry, I was thinking there was a dirtyness assertion in ProcessReflowCommands, but it seems somebody (I?) removed that long ago.  That's what I was suggesting removing.)
roc, do the tree body frame changes make sense?  That code gets hit in reflow via GetMinSize() calling EnsureView().
Attachment #263950 - Attachment is obsolete: true
Attachment #264785 - Flags: superreview?(dbaron)
Attachment #264785 - Flags: review?(roc)
I think you should add a call to UpdateScrollbars to nsTreeBodyFrame::ReflowFinished. That's currently happening in SetView before your patch, but not after.
Will do, sure.  Do you want an updated patch?
Comment on attachment 264785 [details] [diff] [review]
Patch to that effect

I know you're good for it :-)
Attachment #264785 - Flags: review?(roc) → review+
Comment on attachment 264785 [details] [diff] [review]
Patch to that effect

sr=dbaron
Attachment #264785 - Flags: superreview?(dbaron) → superreview+
Checked in with the change roc asked for.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.