Rendering artifacts in Google Wave

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({verified1.9.1})

1.9.1 Branch
x86
Mac OS X
verified1.9.1
Points:
---

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

In drawRect nsChildView.mm doesn't clip out areas that are covered by its child NSViews. On trunk we do subtract those areas from our dirty region, so we could potentially get into trouble in situations where we draw some --- but not all --- content outside our dirty region in the area occupied by a child NSView. Fortunately this isn't too big a deal on trunk because compositor-phase-1 eliminated most child views and I think Cocoa often (but not always) removes child view areas from our getRectsToDraw.

In 1.9.1 things are considerably worse. There, nsViewManager assumes that the platform paint handling will clip out the areas occupied by child widgets. nsChildView.mm does not do this. Things mostly work because Cocoa usually removes those areas from getRectsToDraw, but when count >= MAX_RECTS_IN_REGION we fluff the paint region out to the bounding-box and can easily end up with the clip region including an area occupied by a descendant NSView. Then nsViewManager kicks in and removes the child NSView's area from the paint region, so we don't paint (or only partly paint) that area, and that garbage reaches the screen.

This bug is affecting Google Wave quite badly, probably because it's so complex and it seems to use a number of overflow:auto elements. I've seen dirty regions with over 150 rects in them.
Created attachment 402755 [details] [diff] [review]
fix

I don't think we can test this with existing infrastructure. This patch applies to the 1.9.1 branch.
Attachment #402755 - Flags: review?(mstange)
Google would really like to us to get this on 1.9.1...
blocking1.9.1: --- → ?
Comment on attachment 402755 [details] [diff] [review]
fix

+  nsRegionRectSet* rgnRects = nsnull;
+  rgn->GetRects(&rgnRects);

Is it worth checking rgnRects for null here?
Attachment #402755 - Flags: review?(mstange) → review+
(In reply to comment #0)
> and can easily end up with
> the clip region including an area occupied by a descendant NSView. Then
> nsViewManager kicks in and removes the child NSView's area from the paint
> region, so we don't paint (or only partly paint) that area, and that garbage
> reaches the screen.

Note that this is only problematic in this special case with the large number of rects. Usually this doesn't matter because the child views paint their own stuff on top of the garbage, so it never reaches the screen. But if we're simplifying the region in the superview, and not doing so in the subviews (because they have a lower number of rects), then we fail to cover up the garbage.
Created attachment 402780 [details] [diff] [review]
fix v2

With the null check
Attachment #402755 - Attachment is obsolete: true
If we can't get automated tests, can we get a testcase, at least? Did Google have a deadline in mind for inclusion? It's a little late for 3.5.4, IMO, but we should try to get it for 3.5.5
blocking1.9.1: ? → .5+
status1.9.1: ? → wanted
Created attachment 404456 [details]
testcase for manual testing

Clicking the button should not cause any red to show up.
Let's make this bug's status reflect reality:

This bug is fixed on trunk and 1.9.2 by bug 339548, specifically by http://hg.mozilla.org/mozilla-central/rev/a1161f2b4595

The patch in this bug is basically a partial backport of that patch.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
status1.9.2: --- → beta1-fixed
Depends on: 339548
Resolution: --- → FIXED
Version: Trunk → 1.9.1 Branch
I spent a few hours trying to create a testcase and failed before I noticed that Markus had already figured it out. How embarrassing.
Is this patch ready for 1.9.1? Please request approval, if so.
Attachment #402780 - Flags: approval1.9.1.7?
Attachment #402780 - Flags: approval1.9.1.7? → approval1.9.1.6?
Attachment #402780 - Flags: approval1.9.1.6? → approval1.9.1.6+
Comment on attachment 402780 [details] [diff] [review]
fix v2

Approved for 1.9.1.6, a=dveditz for release-drivers
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db9c10d2cffd
status1.9.1: wanted → .6-fixed
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.6pre) Gecko/20091125 Shiretoko/3.5.6pre using manual testcase.
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.