1442844 - FinishAndStoreOverflow's aNewSize parameter isn't always used

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Matt Woodrow (:mattwoodrow)]

I was debugging display list building issues on Gmail, and found a fairly common case where FinishAndStoreOverflow is called with aNewSize.height == 0 (called from nsBlockFrame::Reflow).

FinishAndStoreOverflow calls SetSize(aNewSize), does some overflow calculations, and then SetSize(oldSize) (aOldSize is nullptr).

The expectation of this code appears to be that the caller will then call SetSize again with the value of aNewSize, but this never happens.

So it appears that we're calling FinishAndStoreOverflow with a height of 0, but we never actually intend to resize to that.

This causes problems for retained-dl, as the SetSize call to 0 (and the one changing back again) register us as being modified, and adds area to the display list building dirty rect.

It also seems that this would potentially be an issue for the overflow area calculations we do that depend on this 0 size (effects, clip-rect, perspective, transform).

David, do you have any ideas on what the expected behaviour is here?

I can fairly easily suppress the retained-dl modifications for this pair of SetSize calls, but that might be ignoring a real issue.

I've got the stack for this, think I accidentally deleted the rr recording though. Can probably generate a new one if necessary.

Comment 1

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

I guess the interesting bit from the stack would be who the caller of nsBlockFrame::Reflow is, since they're the one that's supposed to SetSize.

I suspect the answer will just be that we should just work around it... but as you point out, this also means the transform-related overflow calculations in FinishAndStoreOverflow will also be wrong.

Comment 2

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

It seems like the interesting bit here is the cases where nsBlockFrame::ReflowBlockFrame calls ReflowBlock but doesn't call PlaceBlock.

Comment 3

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

* I think it would be interesting to know which case of exiting from nsBlockFrame::ReflowBlockFrame is happening such that it hits ReflowBlock but not PlaceBlock

* I think suppressing the RDL stuff that happens within FinishAndStoreOverflow may be useful because of the pattern of retrying reflow multiple times (with different sizes) before completing (as in the replacedBlock case in nsBlockFrame::ReflowBlockFrame)... however, it's probably actually not very useful because the reflow of descendants will be completed, so the work there won't get suppressed and we'll lose anyway.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Debugged this properly tonight.

We're not actually exiting nsBlockFrame::ReflowBlockFrame without calling PlaceBlock, we're just calling ReflowBlock twice.

The first call to ReflowBlock has the 0 height, and triggers display item invalidation twice from FinishAndStoreOverflow.

The second call to ReflowBlock, along with the PlaceBlock have the real height, and don't trigger display item invalidation (and my original breakpoints) since the new size and the old size match.

Looks like that should cover all the potential issues with incorrect overflow areas, and we just want to suppress the display item invalidation from the temporary SetSize calls in FinishAndStoreOverflow.

It looks like doing this makes a fair difference on gmail (though hard to be confident, it's not the best reproducible test case), so I think it's worth it. Would be nice if we could avoid invalidating descendants too somehow.

Comment 5

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't mark display items invalid when temporarily setting the frame size in FinishAndStoreOverflow.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Gmail seems to hit this fairly often. It appears that there aren't too many descendants, so just blocking the size change seems to help a lot (plus the table invalidation cleanups in bug 1370575).

It'd be nice if there was a way to avoid to duplicate reflow though, possibly by remembering what result we got last time.

Comment 8

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

Comment on attachment 8963873 [details] [diff] [review]
Don't mark display items invalid when temporarily setting the frame size in FinishAndStoreOverflow.

r=dbaron

I wonder if there's something you can look at in the display item data structures (or somewhere else) where you can check whether the rect *actually* changed over the whole reflow process, so that at the end you can figure out which of the things marked for display item rebuild actually have new rects, and skip the ones that don't.  Maybe worth a followup?  (And potentially undoing this?)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/670c69dba51c
Don't mark display items invalid when temporarily setting the frame size in FinishAndStoreOverflow. r=dbaron

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/670c69dba51c