ComputeRepaintRegionForCopy breaks when scrolling inside a clipped area

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
25 days ago

People

(Reporter: roc, Assigned: roc)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---
Bug Flags:
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

See testcase.

Open the testcase, click inside the scrollable area, and then use down-arrow to scroll down to the bottom. The text "Hello" should scroll into view, but it doesn't appear to. If you force a repaint (e.g. by resizing the window), it appears.

I created this testcase to try to confirm my suspicions in https://bugzilla.mozilla.org/show_bug.cgi?id=424915#c23 but now I'm not sure that's actually the reason for this bug.
I think you forgot to upload the testcase.
That looks more like a patch to me ;)
Created attachment 314977 [details]
oops^2
Attachment #314965 - Attachment is obsolete: true
wanted1.9 since it's a regression from FF2.
Flags: wanted1.9+
Keywords: regression, testcase
Created attachment 315727 [details] [diff] [review]
fix

Three things in this patch:

-- Make nsDisplayClip track which frame is doing the clipping (so later we can tell if it's moving or not)

-- When we decide we don't need to traverse the descendants of the moving frame, add "summary display items" to represent what could have been painted by those descendants. This prevents various display list optimizations throwing out information we need later.

-- Make AddItemsToRegion handle the case of non-moving frames clipping moving frames.
Attachment #315727 - Flags: superreview?(dbaron)
Attachment #315727 - Flags: review?(dbaron)
Whiteboard: [needs review]
Comment on attachment 315727 [details] [diff] [review]
fix

>     if (aBuilder->GetRootMovingFrame() == this &&
>         !PresContext()->GetRenderedPositionVaryingContent()) {
>       // No position-varying content has been rendered in this prescontext.
>       // Therefore there is no need to descend into analyzing the moving frame's
>       // descendants looking for such content, because any bitblit will
>-      // not be copying position-varying graphics.
>+      // not be copying position-varying graphics. However, to keep things
>+      // sane we still need display items representing the frame subtree.
>+      // We need to add these summaries to every list that the child could
>+      // contribute to. This avoids display list optimizations optimizing
>+      // away entire lists because they appear to be empty.

I'm a little surprised this is the only place where we optimize away lists such that nsDisplaySummary items are needed.  Are there really no other such optimizations, or do they really not have the same problems?

r+sr=dbaron
Attachment #315727 - Flags: superreview?(dbaron)
Attachment #315727 - Flags: superreview+
Attachment #315727 - Flags: review?(dbaron)
Attachment #315727 - Flags: review+
AFAIK it's the only place where we decide not to traverse a subtree that's visible.
Comment on attachment 315727 [details] [diff] [review]
fix

This fixes a gaping hole in our code that manages invalidation due to scrolling. I don't know of any real sites that are being affected, but IMHO it's a major 1.9 regression.

The patch is quite safe because it's designed to just make us invalidate more. It might slow down our scrolling in some cases but I've tested troublesome pages like GMail and they have not regressed.
Attachment #315727 - Flags: approval1.9?
Comment on attachment 315727 [details] [diff] [review]
fix

a=beltzner, this probably should have been a blocker
Attachment #315727 - Flags: approval1.9? → approval1.9+
checked in
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
I backed this out to see if it would fix test failures. I doubt it's responsible, but I'm desperate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This did not cause the test failures. I'll reland it.
Whiteboard: [needs landing]
Relanded
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]

Comment 15

11 years ago
Roc, is there any chance this could have caused crash a5985130-0ed5-11dd-a2cf-001cc45a2ce4 (browser area went solid black, then crashed)?
I suppose it's possible. Have you got a bug filed for that crash?
Component: Layout: View Rendering → Layout: Web Painting
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.