518758 - Rendering artifacts in Google Wave

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

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.

Comment 1

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

Attachment: fix

I don't think we can test this with existing infrastructure. This patch applies to the 1.9.1 branch.

Comment 2

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

Google would really like to us to get this on 1.9.1...

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 402755 [details] [diff] [review]
fix

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

Is it worth checking rgnRects for null here?

Comment 4

[Markus Stange [:mstange]]

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

Comment 5

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

Attachment: fix v2

With the null check

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 7

[Markus Stange [:mstange]]

Attachment: testcase for manual testing

Clicking the button should not cause any red to show up.

Comment 8

[Markus Stange [:mstange]]

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.

Comment 9

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

I spent a few hours trying to create a testcase and failed before I noticed that Markus had already figured it out. How embarrassing.

Comment 10

[Samuel Sidler (old account; do not CC)]

Is this patch ready for 1.9.1? Please request approval, if so.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 402780 [details] [diff] [review]
fix v2

Approved for 1.9.1.6, a=dveditz for release-drivers

Comment 12

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

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db9c10d2cffd

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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.