379904 - [FIX]ResizeReflow can still cause wasDirty asserts

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

roc, do the tree body frame changes make sense?  That code gets hit in reflow via GetMinSize() calling EnsureView().

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you should add a call to UpdateScrollbars to nsTreeBodyFrame::ReflowFinished. That's currently happening in SetView before your patch, but not after.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Will do, sure.  Do you want an updated patch?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 264785 [details] [diff] [review]
Patch to that effect

I know you're good for it :-)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 264785 [details] [diff] [review]
Patch to that effect

sr=dbaron

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in with the change roc asked for.